Plots in an editor #4364

timtmok · 2024-08-14T21:10:32Z

Address #2270

This adds an action to display a plot from the Plots pane in an editor. It works on static and dynamic plots. It is an editor just for plots and hosts a React container to reuse the components from the Plots pane. So, it could easily display web plots.

A new editor and editor input has been created. The plot id is used for the editor title. It's not very descriptive and perhaps should be replaced with something that makes sense for a user. The plots service now tracks another list of plots for editors. The original plot client is cloned as a new object. It uses the same plot id as the original plot. The service now has an openEditor() method that opens the current plot in an editor.

The entire feature is hidden behind an experimental flag. It is in the Application > Experimental category in the preferences. It is off by default until the rest of the editor actions are implemented. See the issue for linked follow-up work.

QA Notes

It's likely buggy with how the plot behaves with being moved into a new window and splitting the editor. It shares the sizing policy with the Plots pane so changing it there will re-render the editor in the new size.

nstrayer

Looking good and impressive work!

Left a few comments on code patterns etc..
I feel like we should consider the phrasing of this feature too, I'm not sure how much the user knows that the tabs are represented by "editors." I left a few comments on the code itself about this but I think just a bit more description will go a long way to alleviating potential confusion.

- Probably should have the editor tab show something more meaningful than a random id string.

I noticed that we are using the grab cursor even though there's no scrolling available. Is this a choice to make it clear there's no right-click action menu stuff or just a left-over from the reused components?
On the product side a thing I am curious about is if we want to add the same controls that one gets in the plots pane to the editor? E.g. placing the actions toolbar in the editor pane. I think 90% of the usecases will be just looking at the plot larger, but I could imagine people wanting to be able to export etc from there as well.
This would also have implications on state transfer, e.g. aspect ratio being preserved across transition from the plots pane to the editor tab.
I'm also good to defer this stuff for a future PR as it could add a lot of work and delay people getting access to this.

nstrayer · 2024-08-15T14:10:49Z

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

+	constructor() {
+		super({
+			id: PlotsEditorAction.ID,
+			title: localize2('positronPlots.openEditor', 'Open Plot in Editor'),


I wonder if the phrase "Editor tab" is clearer. To me the whole of positron is an "editor."

"Editor tab" is better phrasing. It's good to set this right because I hadn't noticed this all this time how misleading it can be!

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

nstrayer · 2024-08-15T14:18:13Z

src/vs/workbench/contrib/positronPlotsEditor/browser/editorPlotsContainer.tsx

+}
+
+export const EditorPlotsContainer = (props: EditorPlotsContainerProps) => {
+	usePositronPlotsContext();


This doesn't do anything, right? If there's some sort of effect in the hook we should probably name it as such but it doesn't look like it and things seem to work fine when I remove it.

I'll fix that. I forgot to remove it when I refactored how to display the plot.

nstrayer · 2024-08-15T14:18:30Z

src/vs/workbench/contrib/positronPlotsEditor/browser/editorPlotsContainer.tsx

+
+export const EditorPlotsContainer = (props: EditorPlotsContainerProps) => {
+	usePositronPlotsContext();
+	const render = (plotClient?: IPositronPlotClient) => {


Does plotClient need to be optional here?

I'll fix it!

nstrayer · 2024-08-15T14:19:40Z

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.contribution.ts

+			`${Schemas.positronPlotsEditor}:**/**`,
+			{
+				id: PositronPlotsEditorInput.EditorID,
+				label: localize('positronPlotsEditor', 'Plots Editor'),


I think something along the line of "Editor plot tab" or something is clearer. We're not editing plots. It's unfortunately verbose though.

Good to clarify the name. It's not that much longer so it should look fine wherever it might show up in the UI.

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx

nstrayer · 2024-08-15T14:24:14Z

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx

+			<PositronPlotsContextProvider
+				commandService={this._commandService}
+				configurationService={this._configurationService}
+				contextKeyService={this._contextKeyService}
+				contextMenuService={this._contextMenuService}
+				hoverService={this._hoverService}
+				keybindingService={this._keybindingService}
+				languageRuntimeService={this._languageRuntimeService}
+				positronPlotsService={this._positronPlotsService}
+				notificationService={this._notificationService}
+			>


I think we should create some sort of one-service-context-provider-to-rule-them-all to avoid every area of the react code having their own bespoke one. Not for this PR but just as a comment so I remember later!

nstrayer · 2024-08-15T14:27:34Z

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx

+	render = (plotClient?: IPositronPlotClient) => {
+		if (plotClient instanceof PlotClientInstance) {
+			return <DynamicPlotInstance
+				key={plotClient.id}
+				width={this._width}
+				height={this._height}
+				plotClient={plotClient}
+				zoom={ZoomLevel.Fill} />;
+		}
+		if (plotClient instanceof StaticPlotClient) {
+			return <StaticPlotInstance
+				key={plotClient.id}
+				plotClient={plotClient}
+				zoom={ZoomLevel.OneHundred} />;
+		}
+
+		return null;
+	};
+
+	renderPlot = (plotClient: IPositronPlotClient) => {
+		if (plotClient instanceof PlotClientInstance) {
+			const dynamicPlot = plotClient as PlotClientInstance;
+			dynamicPlot.render(this._height, this._width, 1);
+		}
+	};


Looks like some left-over code here.

nstrayer · 2024-08-15T14:28:11Z

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditorInput.ts

+
+	constructor(
+		readonly resource: URI,
+		// @IPositronPlotsService private readonly _positronPlotsService: IPositronPlotsService


Is there a reason this is commented out and not deleted?

Just more leftover code!

jmcphers

Because this approach uses only one comm, we need to avoid disposing/closing the comm until all instances of the plot in the UI have been closed.

As it stands, cleaning up the plot from the Plots pane causes it to stop working in the editor tab; attempting to resize the editor tab results in a progress bar drawn right in the middle of the plot that never dismisses since the backend can't render the plot any more.

I also observed a fair bit of cross-talk between the plot in the editor and the plot in the Plots pane -- sometimes rendering it in one place caused both places to show a progress bar and/or the resized image to show up in both places. Might be expected at this stage?

Another issue is that you lose your editor plots on reload. You don't need to solve this right away, but we should have a plan to do it. See #3172 for context.

timtmok · 2024-08-19T13:31:18Z

Makes sense to address the plot clients and the comm. I think it's good to use one comm so that any messages can update multiple plot clients. I did notice that the Runtimes view shows new runtime clients when opening a plot in an editor. But all those plot runtime clients are closed once the plot is closed.

timtmok · 2024-08-27T13:58:37Z

Pushing a rebase on main and moving the PR into draft.

It looks like it's best to create a new comm backed by the same plot in the backend. It will require updating Ark as well but this should simplify the frontend. It turns out sharing a comm on the frontend makes it far more complicated with render queues, throttling, and managing the render events.

jmcphers · 2024-08-27T15:46:54Z

If we move this complexity to the backend we will also need to implement for Python and all future language packs. I'd rather have the front end handle all of the complexity around events/throttling since it will keep the logic in one place and make things simpler at the API level.

I'd imagine this could work by adding a service that can manage the plot comms, adding and removing UI instances of the plot (e.g. for the editor placement, the Plots pane, the Save dialog, etc.) and maintaining a single render queue but only delivering events/RPC results to the UI instance that owned the request. When the UI instance refcount drops to zero, we can dispose the comm.

Willing to be convinced otherwise, lmk if you want to discuss realtime!

Renders static and dynamic plots in the editor

Co-authored-by: Nick Strayer <[email protected]> Signed-off-by: Tim Mok <[email protected]>

Opens another comm to a plot in the backend Implement Python plots in editor for the frontend

This reverts commit 5da4fa8.

Fix rebase from main

timtmok · 2024-08-30T19:25:37Z

This is now at a point where the PlotClientInstance has been refactored with the calls to the comm pulled out into PositronPlotCommProxy. This allows sharing the comm while the plot clients can request renders with their own parameters.

The plots service tracks how many clients are still using the comm and will dispose the comm when no more clients are using it. It still uses a new plot client list for editors. I do think it's worth following up with another change to go back to one list by updating IPositronPlotMetadata with a location. This should make it easy to save the plot state for editors and restore it when the UI reloads.

Use editor input dispose to determine the editor is finished with the plot client

jmcphers

Looks good! I left some notes re: indentation but there are a bunch more places where the indentation got garbled (maybe an editor setting?).

I was able to verify that the newest set of changes address the lifecycle management issues; i.e. removing a plot from the Plots pane doesn't cause it to break in the Editor, and closing the Editor cleans up the comm correctly if it is the last remaining reference.

There are still some drawing issues; the progress bar doesn't seem to draw consistently in the editor pane, and when it does, it draws behind the plot.

There's also some crosstalk w/r/t the sizing policy; if you change the sizing policy in the Plots pane, it redraws the plot in the Editor pane too, which feels a little jarring. No need to try to address that now; we will eventually just want the editor plot tab to have its own toolbar where these things can be controlled/surfaced.

jmcphers · 2024-09-03T17:46:08Z

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

@@ -209,6 +118,9 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien
 	onDidRenderUpdate: Event<IRenderedPlot>;
 	private readonly _renderUpdateEmitter = new Emitter<IRenderedPlot>();

+	/**
+ * Event that fires when the plot wants to display itself.


Nit: indentation

jmcphers · 2024-09-03T17:46:28Z

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

+
+export class PositronPlotCommProxy extends Disposable {
+	/**
+ * The currently active render request, if any.


Nit: indentation

jmcphers · 2024-09-03T17:46:38Z

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

+	private _currentRender?: DeferredRender;
+
+	/**
+ * The underlying comm


Nit: indentation

timtmok · 2024-09-03T18:21:06Z

I noticed the progress misplacement as well. I opened #4562 to address it later.

timtmok · 2024-09-03T18:29:53Z

I expect #4358 to address the sizing policy. It should allow different sizing polices for the editors. Now that the plot client has the comm messages separated, it will be easier for each client to have its own render parameters.

timtmok · 2024-09-04T18:39:13Z

I noticed that the comm events weren't hooked up in the comm proxy. It was preventing the plot client from disposing when the comm closed.

The editor won't close when the comm closes. I think a follow up can address this. It can do the same thing as the data explorer and show a message that the plot is gone.

nstrayer

This is awesome and I'm excited to have this!

I like the new shape of things and everything works as expected.

Just had a few nits and general comments about react code organization/patterns that can be addressed now or later (or ignored if they're off base!)

Main stopper is the feature flag default, but should be good to go after that!

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

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

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.contribution.ts

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx

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

src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.contribution.ts

Co-authored-by: Nick Strayer <[email protected]> Signed-off-by: Tim Mok <[email protected]>

nstrayer

Looking good -- this has shaped up real nicely!

nstrayer · 2024-09-09T16:04:46Z

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

-							onPressed={copyPlotHandler} />}
-						{enableZoomPlot && <ZoomPlotMenuButton actionHandler={zoomPlotHandler} zoomLevel={props.zoomLevel} />}
-						{enableSizingPolicy &&
+						{(enableSizingPolicy || enableSavingPlots || enableZoomPlot || enablePopoutPlot) ? <ActionBarSeparator /> : null}


super-nit: I would maybe extract the long or logic here to a well-named variable to make it clearer why something is being rendered or not.

nstrayer · 2024-09-09T16:07:05Z

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

@@ -59,7 +59,7 @@ export interface ActionBarsProps {
 export const ActionBars = (props: PropsWithChildren<ActionBarsProps>) => {
 	// Hooks.
 	const positronPlotsContext = usePositronPlotsContext();
-	const [enableEditorPlot, setEnableEditorPlots] = React.useState<boolean>(false);
+	const [useEditorPlots, setUseEditorPlots] = React.useState<boolean>(positronPlotsEditorEnabled(props.configurationService));


Good call on the initial value to avoid a potentially unnecessary render.

timtmok requested review from jmcphers and nstrayer August 14, 2024 21:10

nstrayer requested changes Aug 15, 2024

View reviewed changes

timtmok force-pushed the feature/plots-editor branch from 5e8ba94 to 5640edd Compare August 15, 2024 18:29

timtmok requested a review from nstrayer August 15, 2024 19:57

jmcphers requested changes Aug 15, 2024

View reviewed changes

timtmok marked this pull request as draft August 27, 2024 13:31

timtmok force-pushed the feature/plots-editor branch from 5640edd to 0739250 Compare August 27, 2024 13:56

timtmok force-pushed the feature/plots-editor branch from 710130b to 9e63df7 Compare August 29, 2024 21:33

timtmok and others added 15 commits August 30, 2024 14:10

Basic plots editor

7120f74

Display plot in editor

5d83e99

Renders static and dynamic plots in the editor

Update open plot editor icon

4841985

Set editor name to plot id

6ba2915

Show plot editor button when applicable

ac60398

Add feature flag

36e073c

Move setting to Application/Experimental

db00b39

Apply suggestions from code review

0f89206

Co-authored-by: Nick Strayer <[email protected]> Signed-off-by: Tim Mok <[email protected]>

Review changes

738df01

Create plot client in backend

8cb23bb

Opens another comm to a plot in the backend Implement Python plots in editor for the frontend

Revert "Create plot client in backend"

0613f67

This reverts commit 5da4fa8.

Shared plot comm

d0e7d0d

Initial comm cleanup when removing plots

6433e28

Handle comm close on editor close

2b9ee3b

Fix unregisterPlotClient

6104ec2

timtmok force-pushed the feature/plots-editor branch from 16ca5c6 to 6104ec2 Compare August 30, 2024 18:11

timtmok added 2 commits August 30, 2024 14:21

Clean up preview calls

c620eb9

Fix rebase from main

Fix disposing editor's plot client

b72be8f

timtmok marked this pull request as ready for review August 30, 2024 19:18

timtmok requested review from jmcphers and seeM August 30, 2024 19:18

Fix plot client disposed too early

e1ad439

Use editor input dispose to determine the editor is finished with the plot client

jmcphers previously approved these changes Sep 3, 2024

View reviewed changes

Formatting

36e08bc

timtmok dismissed jmcphers’s stale review via 36e08bc September 3, 2024 18:26

Reconnect comm events

dd16e43

nstrayer requested changes Sep 5, 2024

View reviewed changes

timtmok and others added 4 commits September 6, 2024 12:06

Apply suggestions from code review

1736e56

Co-authored-by: Nick Strayer <[email protected]> Signed-off-by: Tim Mok <[email protected]>

Review changes

4c92b34

Fix rendering plot element

7f16e39

Refactor need for useCallback hook

b695fbc

timtmok requested a review from nstrayer September 6, 2024 20:26

nstrayer approved these changes Sep 9, 2024

View reviewed changes

timtmok merged commit 475768a into main Sep 10, 2024
2 checks passed

timtmok deleted the feature/plots-editor branch September 10, 2024 16:12

github-actions bot locked and limited conversation to collaborators Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plots in an editor #4364

Plots in an editor #4364

timtmok commented Aug 14, 2024

nstrayer left a comment

nstrayer Aug 15, 2024

timtmok Aug 15, 2024

nstrayer Aug 15, 2024

timtmok Aug 15, 2024

nstrayer Aug 15, 2024

timtmok Aug 15, 2024

nstrayer Aug 15, 2024

timtmok Aug 15, 2024

nstrayer Aug 15, 2024

nstrayer Aug 15, 2024

nstrayer Aug 15, 2024

timtmok Aug 15, 2024

jmcphers left a comment

timtmok commented Aug 19, 2024

timtmok commented Aug 27, 2024 •

edited

Loading

jmcphers commented Aug 27, 2024

timtmok commented Aug 30, 2024

jmcphers left a comment

jmcphers Sep 3, 2024

jmcphers Sep 3, 2024

jmcphers Sep 3, 2024

timtmok commented Sep 3, 2024

timtmok commented Sep 3, 2024

timtmok commented Sep 4, 2024

nstrayer left a comment

nstrayer left a comment

nstrayer Sep 9, 2024

nstrayer Sep 9, 2024

Plots in an editor #4364

Plots in an editor #4364

Conversation

timtmok commented Aug 14, 2024

QA Notes

nstrayer 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

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

jmcphers left a comment

Choose a reason for hiding this comment

timtmok commented Aug 19, 2024

timtmok commented Aug 27, 2024 • edited Loading

jmcphers commented Aug 27, 2024

timtmok commented Aug 30, 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

timtmok commented Sep 3, 2024

timtmok commented Sep 3, 2024

timtmok commented Sep 4, 2024

nstrayer left a comment

Choose a reason for hiding this comment

nstrayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtmok commented Aug 27, 2024 •

edited

Loading