From b41cf05d16771929188aff6c4a40c88137c156c6 Mon Sep 17 00:00:00 2001 From: Tim Mok Date: Mon, 15 Apr 2024 11:49:24 -0400 Subject: [PATCH] Add other plot save formats (#2729) Support multiple formats for plot saving Add SVG, PDF, JPEG support Add format selector in dialog Browse now sets directory save location Add field for file name Validate path at save time Show error if saving failed --- .../positron/positron_ipykernel/plot_comm.py | 4 + .../positron/positron_ipykernel/plots.py | 26 ++- .../positron_ipykernel/tests/test_plots.py | 2 +- positron/comms/plot-backend-openrpc.json | 8 + .../modalDialogs/savePlotModalDialog.css | 20 ++- .../modalDialogs/savePlotModalDialog.tsx | 156 ++++++++++++++---- .../browser/positronPlotsService.ts | 14 +- .../common/languageRuntimePlotClient.ts | 20 ++- .../common/positronPlotComm.ts | 5 +- 9 files changed, 198 insertions(+), 57 deletions(-) diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/plot_comm.py b/extensions/positron-python/python_files/positron/positron_ipykernel/plot_comm.py index fdeaa8dcd67..57a160b890e 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/plot_comm.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/plot_comm.py @@ -59,6 +59,10 @@ class RenderParams(BaseModel): description="The pixel ratio of the display device", ) + format: str = Field( + description="The requested plot format", + ) + class RenderRequest(BaseModel): """ diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/plots.py b/extensions/positron-python/python_files/positron/positron_ipykernel/plots.py index 9ecae76f816..03b061db47f 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/plots.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/plots.py @@ -26,6 +26,13 @@ DEFAULT_HEIGHT_IN = 4.8 BASE_DPI = 100 +MIME_TYPE = { + "png": "image/png", + "svg": "image/svg+xml", + "pdf": "application/pdf", + "jpeg": "image/jpeg", +} + class PositronDisplayPublisherHook: def __init__(self, target_name: str, session_mode: SessionMode): @@ -112,12 +119,16 @@ def handle_msg(self, msg: CommMessage[PlotBackendMessageContent], raw_msg: JsonR width_px = request.params.width or 0 height_px = request.params.height or 0 pixel_ratio = request.params.pixel_ratio or 1.0 + format = request.params.format or "png" if width_px != 0 and height_px != 0: - format_dict = self._resize_pickled_figure(pickled, width_px, height_px, pixel_ratio) - data = format_dict["image/png"] - output = PlotResult(data=data, mime_type="image/png").dict() - figure_comm.send_result(data=output, metadata={"mime_type": "image/png"}) + format_dict = self._resize_pickled_figure( + pickled, width_px, height_px, pixel_ratio, [format] + ) + mime_type = MIME_TYPE[format] + data = format_dict[mime_type] + output = PlotResult(data=data, mime_type=mime_type).dict() + figure_comm.send_result(data=output, metadata={"mime_type": mime_type}) else: logger.warning(f"Unhandled request: {request}") @@ -170,7 +181,7 @@ def _resize_pickled_figure( new_width_px: int = 614, new_height_px: int = 460, pixel_ratio: float = 1.0, - formats: list = ["image/png"], + formats: list = ["png"], ) -> dict: # Delay importing matplotlib until the kernel and shell has been # initialized otherwise the graphics backend will be reset to the gui @@ -215,11 +226,12 @@ def _resize_pickled_figure( # Render the figure to a buffer # using format_display_data() crops the figure to smaller than requested size - figure.savefig(figure_buffer, format="png") + figure.savefig(figure_buffer, format=formats[0]) figure_buffer.seek(0) image_data = base64.b64encode(figure_buffer.read()).decode() + key = MIME_TYPE[formats[0]] - format_dict = {"image/png": image_data} + format_dict = {key: image_data} plt.close(figure) diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_plots.py b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_plots.py index 8f978066790..6462d49cdb5 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_plots.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_plots.py @@ -162,7 +162,7 @@ def test_hook_call(hook: PositronDisplayPublisherHook, images_path: Path) -> Non def render_request(comm_id: str, width_px: int = 500, height_px: int = 500, pixel_ratio: int = 1): return json_rpc_request( "render", - {"width": width_px, "height": height_px, "pixel_ratio": pixel_ratio}, + {"width": width_px, "height": height_px, "pixel_ratio": pixel_ratio, "format": "png"}, comm_id=comm_id, ) diff --git a/positron/comms/plot-backend-openrpc.json b/positron/comms/plot-backend-openrpc.json index ae607cc8cc9..5a80970a01e 100644 --- a/positron/comms/plot-backend-openrpc.json +++ b/positron/comms/plot-backend-openrpc.json @@ -30,6 +30,14 @@ "schema": { "type": "number" } + }, + { + "name": "format", + "description": "The requested plot format", + "schema": { + "type": "string" + }, + "required": false } ], "result": { diff --git a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.css b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.css index f0c7fabe429..e6314346abc 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.css +++ b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.css @@ -4,9 +4,10 @@ .plot-preview-input { height: 100%; - grid-template-rows: 1fr 2fr 4px 9fr; + grid-template-rows: 1fr 1fr 2fr 4px 9fr; grid-template-areas: "browse" + "file" "plot-input" "preview-progress" "preview"; @@ -28,6 +29,17 @@ grid-area: input; } +.plot-preview-input .file { + display: flex; + flex-direction: row; + column-gap: 10px; + align-items: flex-end; +} + +.plot-preview-input .file button { + margin-top: 4px; +} + .plot-input div.error { padding-top: 4px; grid-column: 1 / span 3; @@ -40,10 +52,14 @@ width: auto; } -.plot-preview-input .labeled-text-input input { +.plot-preview-input .plot-input .labeled-text-input input { width: 100px; } +.plot-preview-input .file .labeled-text-input input { + width: 200px; +} + .plot-preview-input .preview-progress { grid-area: preview-progress; display: flex; diff --git a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx index 1903bcdfdb2..b52a3165ad4 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx +++ b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx @@ -7,7 +7,7 @@ import * as React from 'react'; import { localize } from 'vs/nls'; import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService'; import { IRenderedPlot, PlotClientInstance } from 'vs/workbench/services/languageRuntime/common/languageRuntimePlotClient'; -import { IFileDialogService } from 'vs/platform/dialogs/common/dialogs'; +import { IDialogService, IFileDialogService } from 'vs/platform/dialogs/common/dialogs'; import { URI } from 'vs/base/common/uri'; import { ProgressBar } from 'vs/base/browser/ui/positronComponents/progressBar'; import { LabeledTextInput } from 'vs/workbench/browser/positronComponents/positronModalDialog/components/labeledTextInput'; @@ -18,12 +18,23 @@ import { OKCancelActionBar } from 'vs/workbench/browser/positronComponents/posit import { PositronModalDialog } from 'vs/workbench/browser/positronComponents/positronModalDialog/positronModalDialog'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; import { PositronModalReactRenderer } from 'vs/workbench/browser/positronModalReactRenderer/positronModalReactRenderer'; +import { FileFilter } from 'electron'; +import { DropDownListBox } from 'vs/workbench/browser/positronComponents/dropDownListBox/dropDownListBox'; +import { DropDownListBoxItem } from 'vs/workbench/browser/positronComponents/dropDownListBox/dropDownListBoxItem'; +import { IFileService } from 'vs/platform/files/common/files'; export interface SavePlotOptions { uri: string; path: URI; } +export enum PlotFormat { + PNG = 'png', + SVG = 'svg', + PDF = 'pdf', + JPEG = 'jpeg', +} + const SAVE_PLOT_MODAL_DIALOG_WIDTH = 500; const SAVE_PLOT_MODAL_DIALOG_HEIGHT = 600; const BASE_DPI = 100; // matplotlib default DPI @@ -32,6 +43,8 @@ const BASE_DPI = 100; // matplotlib default DPI * Show the save plot modal dialog for dynamic plots. * @param layoutService the layout service for the modal * @param keybindingService the keybinding service to intercept shortcuts + * @param dialogService the dialog service to confirm the save + * @param fileService the file service to check if paths exist * @param fileDialogService the file dialog service to prompt where to save the plot * @param plotClient the dynamic plot client to render previews and the final image * @param savePlotCallback the action to take when the dialog closes @@ -40,6 +53,8 @@ const BASE_DPI = 100; // matplotlib default DPI export const showSavePlotModalDialog = ( layoutService: IWorkbenchLayoutService, keybindingService: IKeybindingService, + dialogService: IDialogService, + fileService: IFileService, fileDialogService: IFileDialogService, plotClient: PlotClientInstance, savePlotCallback: (options: SavePlotOptions) => void, @@ -58,7 +73,10 @@ export const showSavePlotModalDialog = ( renderer.render( { - const [path, setPath] = React.useState({ value: props.suggestedPath ?? URI.file(''), valid: true }); + const [directory, setDirectory] = React.useState({ value: props.suggestedPath ?? URI.file(''), valid: true }); + const [name, setName] = React.useState({ value: 'plot', valid: true }); + const [format, setFormat] = React.useState(PlotFormat.PNG); const [width, setWidth] = React.useState({ value: props.plotWidth, valid: true }); const [height, setHeight] = React.useState({ value: props.plotHeight, valid: true }); const [dpi, setDpi] = React.useState({ value: 100, valid: true }); @@ -89,13 +118,18 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { const [rendering, setRendering] = React.useState(false); const inputRef = React.useRef(null); + const filterEntries: FileFilter[] = []; + for (const filter in PlotFormat) { + filterEntries.push({ extensions: [filter.toLowerCase()], name: filter.toUpperCase() }); + } + React.useEffect(() => { setUri(props.plotClient.lastRender?.uri ?? ''); }, [props.plotClient.lastRender?.uri]); const validateInput = React.useCallback((): boolean => { - return path.valid && width.valid && height.valid && dpi.valid; - }, [path, width, height, dpi]); + return directory.valid && width.valid && height.valid && dpi.valid && name.valid; + }, [directory, width, height, dpi, name]); React.useEffect(() => { validateInput(); @@ -117,38 +151,58 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { }; const updatePath = (pathString: string) => { - const newPath = URI.file(pathString); - setPath({ value: newPath, valid: !!newPath }); + try { + const newPath = URI.file(pathString); + props.fileService.exists(newPath).then(exists => { + setDirectory({ + value: newPath, valid: exists, + errorMessage: exists ? undefined : localize('positron.savePlotModalDialog.pathDoesNotExist', "Path does not exist.") + }); + }); + } catch (error) { + setDirectory({ value: URI.file(''), valid: false, errorMessage: error.message }); + } }; const browseHandler = async () => { - const uri = await props.fileDialogService.showSaveDialog({ + const uri = await props.fileDialogService.showOpenDialog({ title: localize('positron.savePlotModalDialog.title', "Save Plot"), - filters: - [ - { - extensions: ['png'], - name: 'PNG', - }, - ], + defaultUri: directory.value, + openLabel: localize('positron.savePlotModalDialog.select', "Select"), + canSelectFiles: false, + canSelectFolders: true, + canSelectMany: false, }); - if (uri?.fsPath.length) { - setPath({ value: uri, valid: true }); + if (uri && uri.length > 0) { + updatePath(uri[0].fsPath); } }; const acceptHandler = async () => { if (validateInput()) { - setRendering(true); - const plotResult = await generatePreview(); - - if (plotResult) { - props.savePlotCallback({ uri: plotResult.uri, path: path.value }); + const fileExists = await props.fileService.exists(directory.value); + if (fileExists) { + const confirmation = await props.dialogService.confirm({ + message: localize('positron.savePlotModalDialog.fileExists', "The file already exists. Do you want to overwrite it?"), + primaryButton: localize('positron.savePlotModalDialog.overwrite', "Overwrite"), + cancelButton: localize('positron.savePlotModalDialog.cancel', "Cancel"), + }); + if (!confirmation.confirmed) { + return; + } } + setRendering(true); - setRendering(false); - props.renderer.dispose(); + generatePreview(format) + .then(async (plotResult) => { + const filePath = URI.joinPath(directory.value, `${name.value}.${format}`); + props.savePlotCallback({ uri: plotResult.uri, path: filePath }); + }) + .finally(() => { + setRendering(false); + props.renderer.dispose(); + }); } }; @@ -162,15 +216,15 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { } setRendering(true); try { - const plotResult = await generatePreview(); + const plotResult = await generatePreview(PlotFormat.PNG); setUri(plotResult.uri); } finally { setRendering(false); } }; - const generatePreview = async (): Promise => { - return props.plotClient.preview(height.value, width.value, dpi.value / BASE_DPI); + const generatePreview = async (format: PlotFormat): Promise => { + return props.plotClient.preview(height.value, width.value, dpi.value / BASE_DPI, format); }; const previewButton = () => { @@ -195,16 +249,46 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => {
localize( - 'positron.savePlotModalDialog.path', - "Path" + 'positron.savePlotModalDialog.directory', + "Directory" ))()} - value={path.value.fsPath} + value={directory.value.fsPath} onChange={e => updatePath(e.target.value)} onBrowse={browseHandler} readOnlyInput={false} - error={!path.valid} + error={!directory.valid} inputRef={inputRef} />
+
+ localize( + 'positron.savePlotModalDialog.name', + "Name" + ))()} + value={name.value} + onChange={e => setName({ value: e.target.value, valid: !!e.target.value })} + error={!name.valid} + /> +
+ +
+
localize( @@ -242,9 +326,9 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { />
- {!path.valid && (() => localize( - 'positron.savePlotModalDialog.noPathMessage', - "Specify a path." + {!directory.valid && (() => localize( + 'positron.savePlotModalDialog.invalidPathError', + "Invalid path: {0}", directory.errorMessage ))()}
@@ -259,6 +343,12 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { "DPI must be between 1 and 300." ))()}
+
+ {!name.valid && (() => localize( + 'positron.savePlotModalDialog.invalidNameError', + "Plot name cannot be empty." + ))()} +
diff --git a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts index 89eb57c9526..f2eaa68e6d4 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts +++ b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts @@ -24,14 +24,14 @@ import { WebviewPlotClient } from 'vs/workbench/contrib/positronPlots/browser/we import { IPositronNotebookOutputWebviewService } from 'vs/workbench/contrib/positronOutputWebview/browser/notebookOutputWebviewService'; import { IPositronIPyWidgetsService } from 'vs/workbench/services/positronIPyWidgets/common/positronIPyWidgetsService'; import { Schemas } from 'vs/base/common/network'; -import { IFileDialogService } from 'vs/platform/dialogs/common/dialogs'; +import { IDialogService, IFileDialogService } from 'vs/platform/dialogs/common/dialogs'; import { decodeBase64 } from 'vs/base/common/buffer'; import { SavePlotOptions, showSavePlotModalDialog } from 'vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog'; import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService'; -import { URI } from 'vs/base/common/uri'; import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; import { IClipboardService } from 'vs/platform/clipboard/common/clipboardService'; +import { localize } from 'vs/nls'; /** The maximum number of recent executions to store. */ const MaxRecentExecutions = 10; @@ -109,7 +109,8 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, @IWorkbenchLayoutService private readonly _layoutService: IWorkbenchLayoutService, @IKeybindingService private readonly _keybindingService: IKeybindingService, - @IClipboardService private _clipboardService: IClipboardService) { + @IClipboardService private _clipboardService: IClipboardService, + @IDialogService private readonly _dialogService: IDialogService) { super(); // Register for language runtime service startups @@ -682,7 +683,7 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe if (this._selectedPlotId) { const plot = this._plots.find(plot => plot.id === this._selectedPlotId); const workspaceFolder = this._workspaceContextService.getWorkspace().folders[0]?.uri; - const suggestedPath = workspaceFolder ? URI.joinPath(workspaceFolder, 'plot.png') : undefined; + const suggestedPath = workspaceFolder ?? undefined; if (plot) { let uri = ''; @@ -692,7 +693,7 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe this.showSavePlotDialog(uri); } else if (plot instanceof PlotClientInstance) { // if it's a dynamic plot, present options dialog - showSavePlotModalDialog(this._layoutService, this._keybindingService, this._fileDialogService, plot, this.savePlotAs, suggestedPath); + showSavePlotModalDialog(this._layoutService, this._keybindingService, this._dialogService, this._fileService, this._fileDialogService, plot, this.savePlotAs, suggestedPath); } else { // if it's a webview plot, do nothing return; @@ -712,7 +713,8 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe const data = matches[2]; htmlFileSystemProvider.writeFile(options.path, decodeBase64(data).buffer, { create: true, overwrite: true, unlock: true, atomic: false }) - .then(() => { + .catch((error: Error) => { + this._dialogService.error(localize('positronPlotsService.savePlotError.unknown', 'Error saving plot: {0}', error.message)); }); }; diff --git a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts index 97baccfb878..5b19883716f 100644 --- a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts +++ b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts @@ -76,6 +76,9 @@ interface RenderRequest { /** The pixel ratio of the device for which the plot was rendered */ pixel_ratio: number; + + /** The format of the plot */ + format: string; } /** @@ -246,9 +249,10 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien * @param height The plot height, in pixels * @param width The plot width, in pixels * @param pixel_ratio The device pixel ratio (e.g. 1 for standard displays, 2 for retina displays) + * @param format The format of the plot ('png', 'svg') * @returns A promise that resolves to a rendered image, or rejects with an error. */ - public render(height: number, width: number, pixel_ratio: number): Promise { + public render(height: number, width: number, pixel_ratio: number, format = 'png'): Promise { // Deal with whole pixels only height = Math.floor(height); width = Math.floor(width); @@ -269,7 +273,8 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien const request: RenderRequest = { height, width, - pixel_ratio + pixel_ratio, + format }; const deferred = new DeferredRender(request); @@ -309,7 +314,7 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien * @param pixel_ratio The device pixel ratio (e.g. 1 for standard displays, 2 for retina displays) * @returns A promise that resolves when the render request is scheduled, or rejects with an error. */ - public preview(height: number, width: number, pixel_ratio: number): Promise { + public preview(height: number, width: number, pixel_ratio: number, format: string): Promise { // Deal with whole pixels only height = Math.floor(height); width = Math.floor(width); @@ -318,7 +323,8 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien const request: RenderRequest = { height, width, - pixel_ratio + pixel_ratio, + format }; const deferred = new DeferredRender(request); @@ -384,7 +390,8 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien const renderRequest = request.renderRequest; this._comm.render(renderRequest.height, renderRequest.width, - renderRequest.pixel_ratio).then((response) => { + renderRequest.pixel_ratio, + renderRequest.format).then((response) => { // Ignore if the request was cancelled or already fulfilled if (!request.isComplete) { @@ -490,7 +497,8 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien const req = new DeferredRender({ height: Math.floor(height!), width: Math.floor(width!), - pixel_ratio: pixel_ratio! + pixel_ratio: pixel_ratio!, + format: this._currentRender?.renderRequest.format ?? 'png' }); this.scheduleRender(req, 0); diff --git a/src/vs/workbench/services/languageRuntime/common/positronPlotComm.ts b/src/vs/workbench/services/languageRuntime/common/positronPlotComm.ts index e263c113b06..95836a370f3 100644 --- a/src/vs/workbench/services/languageRuntime/common/positronPlotComm.ts +++ b/src/vs/workbench/services/languageRuntime/common/positronPlotComm.ts @@ -51,11 +51,12 @@ export class PositronPlotComm extends PositronBaseComm { * @param height The requested plot height, in pixels * @param width The requested plot width, in pixels * @param pixelRatio The pixel ratio of the display device + * @param format The requested plot format * * @returns A rendered plot */ - render(height: number, width: number, pixelRatio: number): Promise { - return super.performRpc('render', ['height', 'width', 'pixel_ratio'], [height, width, pixelRatio]); + render(height: number, width: number, pixelRatio: number, format: string): Promise { + return super.performRpc('render', ['height', 'width', 'pixel_ratio', 'format'], [height, width, pixelRatio, format]); }