Add UI modal dialog for saving plots #2589

timtmok · 2024-04-01T15:46:09Z

Intent

Address #2169

Approach

For static plots, present a file dialog to select the save location. It saves the data uri that is shown in the plots pane.

For dynamic plots, a dialog is presented to select the save location, width, height, and DPI. A preview is available if the user wants to see what it looks like before saving. At save, the plot is rendered and saved.

The modal dialog uses callbacks as was changed with the recent refactoring with modals.

The file formats was mostly scaffolding for future support and this continues that with a bit of hardcoding to PNG.

QA Notes

See https://docs.google.com/document/d/1CqA8Ypji8FhiqZ5MujvilfV58btoNfIbsjK67qFtvd8/edit?usp=sharing for the spec.

The dialog does validate the values before generating a preview and saving. This wasn't tried on Windows so there may be bugs with the path validation that may need fixing.

The file format is PNG only at the moment

use a progress bar when rendering move to a grid layout to provide more space to the preview

Use save callback

timtmok · 2024-04-01T17:16:42Z

Fixing this build failure. I messed up something that I didn't catch locally during code cleanup.

src/vs/base/browser/ui/positronComponents/progressBar.tsx

...vs/workbench/browser/positronComponents/positronModalDialog/components/okCancelActionBar.tsx

src/vs/workbench/contrib/positronPlots/browser/components/actionBars.tsx

petetronic · 2024-04-01T17:45:45Z

src/vs/workbench/contrib/positronPlots/browser/components/actionBars.tsx

@@ -117,6 +125,7 @@ export const ActionBars = (props: PropsWithChildren<ActionBarsProps>) => {
 						<ActionBarButton iconId='positron-right-arrow' disabled={disableRight} tooltip={positronShowNextPlot} ariaLabel={positronShowNextPlot} onPressed={showNextPlotHandler} />

 						{enableZoomPlot && <ZoomPlotMenuButton actionHandler={zoomPlotHandler} zoomLevel={props.zoomLevel} />}
+						{enableSavingPlots && <ActionBarButton iconId='positron-save' tooltip={localize('positronSavePlot', "Save plot")} ariaLabel={localize('positronSavePlot', "Save plot")} onPressed={savePlotHandler} />}


Nit, for consistency, should localize('positronSavePlot', "Save plot") be saved in a variable, like positronClearAllPlots was on line 37?

We no longer need to preload all our calls to localize in modal dialogs. This was fixed in my modal dialog rework a couple of weeks ago.

So now, this code can be inlined:

/** * Localized strings. */ const positronShowPreviousPlot = localize('positronShowPreviousPlot', "Show previous plot"); const positronShowNextPlot = localize('positronShowNextPlot', "Show next plot"); const positronClearAllPlots = localize('positronClearAllPlots', "Clear all plots");

That's good to know. I thought it was for ease of re-use and clarity.

petetronic · 2024-04-01T17:55:15Z

Nice Tim! Do you want to log a follow-up task to handle if the target file already exists when saving?

petetronic · 2024-04-01T18:01:38Z

Also, for another follow up issue, do you want to add support for saving SVG based plots (presumably a rendering to PNG as an MVP)? Alternatively, if we don't support this in 1.0, the Save button should be disabled, perhaps with a tool-tip explaining the plot format is not supported for saving.

softwarenerd · 2024-04-01T19:50:28Z

src/vs/workbench/contrib/positronPlots/browser/components/actionBars.tsx

@@ -106,6 +111,9 @@ export const ActionBars = (props: PropsWithChildren<ActionBarsProps>) => {
 	const zoomPlotHandler = (zoomLevel: number) => {
 		props.zoomHandler(zoomLevel);
 	};
+	const savePlotHandler = async () => {


One of these should be the implemetation:

const savePlotHandler = async () => positronPlotsContext.positronPlotsService.savePlot(); const savePlotHandler = async () => { await positronPlotsContext.positronPlotsService.savePlot(); };

softwarenerd · 2024-04-01T19:53:04Z

src/vs/workbench/contrib/positronPlots/browser/components/actionBars.tsx

@@ -117,6 +125,7 @@ export const ActionBars = (props: PropsWithChildren<ActionBarsProps>) => {
 						<ActionBarButton iconId='positron-right-arrow' disabled={disableRight} tooltip={positronShowNextPlot} ariaLabel={positronShowNextPlot} onPressed={showNextPlotHandler} />

 						{enableZoomPlot && <ZoomPlotMenuButton actionHandler={zoomPlotHandler} zoomLevel={props.zoomLevel} />}
+						{enableSavingPlots && <ActionBarButton iconId='positron-save' tooltip={localize('positronSavePlot', "Save plot")} ariaLabel={localize('positronSavePlot', "Save plot")} onPressed={savePlotHandler} />}


We no longer need to preload all our calls to localize in modal dialogs. This was fixed in my modal dialog rework a couple of weeks ago.

So now, this code can be inlined:

/** * Localized strings. */ const positronShowPreviousPlot = localize('positronShowPreviousPlot', "Show previous plot"); const positronShowNextPlot = localize('positronShowNextPlot', "Show next plot"); const positronClearAllPlots = localize('positronClearAllPlots', "Clear all plots");

softwarenerd · 2024-04-01T19:56:52Z

src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts

+
+				if (plot instanceof StaticPlotClient) {
+					// if it's a static plot, save the image to disk
+					const staticPlot = plot as StaticPlotClient;


This is unnecessary. plot is an instance of StaticPlotClient.

timtmok · 2024-04-02T13:24:11Z

Nice Tim! Do you want to log a follow-up task to handle if the target file already exists when saving?

I think it's good now from using the fileDialogService.showSaveDialog(). It warns if that the file already exists. It's something that came for free. But it can be disabled there and re-implemented to be shown just before writing the file.

Fix focusable elements in modal Inline string localization Fix action bar button order and separator visibility

timtmok · 2024-04-02T15:45:27Z

Also, for another follow up issue, do you want to add support for saving SVG based plots (presumably a rendering to PNG as an MVP)? Alternatively, if we don't support this in 1.0, the Save button should be disabled, perhaps with a tool-tip explaining the plot format is not supported for saving.

I think this can be added in without too much more work. Most of the existing code prior to this PR accounts for other formats.

timtmok · 2024-04-02T15:47:43Z

I also fixed up the button order on the action bar. The save button wasn't in a consistent location due to the zoom button.

The tab order in the modal is also better now since the input number elements weren't being included properly in the focusable elements. There still seems to be a bug with the Save and Cancel buttons that don't seem to be included in the tab order. They should work as far as I can tell.

timtmok · 2024-04-02T16:42:20Z

Fixed up the compile error. The change didn't get staged for commit for some reason.

jmcphers

This could be merged as-is but I think we could make the error & plot state management better! Left a few notes.

jmcphers · 2024-04-02T23:47:43Z

src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts

@@ -299,13 +299,62 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien
 		return deferred.promise;
 	}

+	/**
+	 * Requests that the plot be rendered at a specific size, but does not
+	 * store the rendered plot to _lastRender. This is useful for previewing


This duplicates a lot of the code in render(). Consider consolidating?

One way to avoid this duplication would be to create a way to clone the plot client object so that you have two clients running against the same comm -- one for your preview window, another for the plots pane.

Since the backend is not stateful this is totally legit. It would also fix the other issue re: unnecessary re-rendering since the client for the preview window could also cache previous renders.

jmcphers · 2024-04-02T23:48:00Z

src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts

+					showSavePlotModalDialog(this._layoutService, this._keybindingService, this._fileDialogService, plot, this.savePlotAs, suggestedPath);
+				} else {
+					// if it's a webview plot, do nothing
+					return;


Should we show a warning in this case?

jmcphers · 2024-04-02T23:50:27Z

src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx

+	const acceptHandler = async () => {
+		if (validateInput()) {
+			setRendering(true);
+			const plotResult = await generatePreview();


If an exception is thrown while generating the plot prior to saving it, we should show that error to the user in an error dialog.

jmcphers · 2024-04-02T23:52:12Z

src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx

+
+	const acceptHandler = async () => {
+		if (validateInput()) {
+			setRendering(true);


In my testing, this caused the plot to be unnecessarily rendered a second time even though I'd already done a preview render at the correct size -- maybe we can avoid rendering in this case?

jmcphers · 2024-04-02T23:53:05Z

src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx

+		}
+		setRendering(true);
+		try {
+			const plotResult = await generatePreview();


We should catch errors thrown during preview and show them to the user -- probably in the error area already established.

(A very common error here would e.g. be the "figure margins too large" error you get in R when you try to draw a plot that's too small for its contents)

timtmok · 2024-04-04T19:12:05Z

I've created a couple of issues for followup:

#2657
#2662

Preview before saving dynamic plots Use a progress bar when rendering Save plot validation Use save callback

timtmok added 6 commits April 1, 2024 11:06

basic ability to save plots

fcc61d4

Preview before saving dynamic plots

0638711

Add DPI to save dialog

ae23f99

save plot dialog layout update

bb4cca6

use a progress bar when rendering move to a grid layout to provide more space to the preview

Save plot validation

78cd48f

Refactoring

84dcbd8

Use save callback

timtmok requested review from jmcphers and softwarenerd April 1, 2024 15:46

Cleanup

e9f4870

timtmok force-pushed the plot-saving branch from b7fa5e5 to e9f4870 Compare April 1, 2024 17:19

petetronic reviewed Apr 1, 2024

View reviewed changes

src/vs/base/browser/ui/positronComponents/progressBar.tsx Outdated Show resolved Hide resolved

petetronic reviewed Apr 1, 2024

View reviewed changes

...vs/workbench/browser/positronComponents/positronModalDialog/components/okCancelActionBar.tsx Outdated Show resolved Hide resolved

petetronic reviewed Apr 1, 2024

View reviewed changes

src/vs/workbench/contrib/positronPlots/browser/components/actionBars.tsx Outdated Show resolved Hide resolved

petetronic reviewed Apr 1, 2024

View reviewed changes

softwarenerd reviewed Apr 1, 2024

View reviewed changes

timtmok added 2 commits April 2, 2024 09:25

Minor fixes

cc70d6f

More fixes

4216161

Fix focusable elements in modal Inline string localization Fix action bar button order and separator visibility

Remove unnecessary id

ac75f59

wesm changed the title ~~Plot saving~~ Add UI modal dialog for saving plots Apr 2, 2024

jmcphers approved these changes Apr 2, 2024

View reviewed changes

timtmok merged commit 675b950 into main Apr 4, 2024
1 check passed

timtmok deleted the plot-saving branch April 4, 2024 19:13

nstrayer pushed a commit that referenced this pull request Apr 17, 2024

Add UI modal dialog for saving plots (#2589)

54b4130

Preview before saving dynamic plots Use a progress bar when rendering Save plot validation Use save callback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI modal dialog for saving plots #2589

Add UI modal dialog for saving plots #2589

timtmok commented Apr 1, 2024

timtmok commented Apr 1, 2024

petetronic Apr 1, 2024

softwarenerd Apr 1, 2024

timtmok Apr 2, 2024

petetronic commented Apr 1, 2024

petetronic commented Apr 1, 2024 •

edited

Loading

softwarenerd Apr 1, 2024

softwarenerd Apr 1, 2024

softwarenerd Apr 1, 2024

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

jmcphers left a comment

jmcphers Apr 2, 2024

jmcphers Apr 2, 2024

jmcphers Apr 2, 2024

jmcphers Apr 2, 2024

jmcphers Apr 2, 2024

jmcphers Apr 2, 2024

timtmok commented Apr 4, 2024

Add UI modal dialog for saving plots #2589

Add UI modal dialog for saving plots #2589

Conversation

timtmok commented Apr 1, 2024

Intent

Approach

QA Notes

timtmok commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetronic commented Apr 1, 2024

petetronic commented Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

timtmok commented Apr 2, 2024

jmcphers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtmok commented Apr 4, 2024

petetronic commented Apr 1, 2024 •

edited

Loading