From 257cbcdc6e238cefc177400fed1a5b2ec31f4ef1 Mon Sep 17 00:00:00 2001 From: Tim Mok Date: Thu, 23 Jan 2025 08:48:51 -0500 Subject: [PATCH] Plot editor tab sizing policies (#6043) Adds a command palette action to change the sizing policy for a plot in an editor tab or the plots view --- .../components/dynamicPlotInstance.tsx | 6 +- .../components/openInEditorMenuButton.tsx | 1 - .../components/sizingPolicyMenuButton.tsx | 9 +- .../modalDialogs/savePlotModalDialog.tsx | 2 +- .../browser/positronPlots.contribution.ts | 3 +- .../browser/positronPlotsActions.ts | 163 +++++++++++++++-- .../browser/positronPlotsService.ts | 167 ++++++++++++------ .../positronPlotsService.test.ts | 42 +++-- .../browser/positronPlotsEditor.tsx | 1 - .../browser/positronPlotsEditorInput.ts | 2 +- .../common/languageRuntimePlotClient.ts | 44 ++++- .../common/positronPlotCommProxy.ts | 4 +- .../positronPlots/common/positronPlots.ts | 23 ++- 13 files changed, 361 insertions(+), 106 deletions(-) diff --git a/src/vs/workbench/contrib/positronPlots/browser/components/dynamicPlotInstance.tsx b/src/vs/workbench/contrib/positronPlots/browser/components/dynamicPlotInstance.tsx index 6c393ee7d5d..64aac3851dc 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/components/dynamicPlotInstance.tsx +++ b/src/vs/workbench/contrib/positronPlots/browser/components/dynamicPlotInstance.tsx @@ -78,7 +78,7 @@ export const DynamicPlotInstance = (props: DynamicPlotInstanceProps) => { // Wait for the plot to render. const result = - await props.plotClient.render(plotSize, ratio); + await props.plotClient.renderWithSizingPolicy(plotSize, ratio); // Update the URI to the URI of the new plot. setUri(result.uri); @@ -94,7 +94,7 @@ export const DynamicPlotInstance = (props: DynamicPlotInstanceProps) => { }; // Render using the current sizing policy. - render(plotsContext.positronPlotsService.selectedSizingPolicy); + render(props.plotClient.sizingPolicy); // When the plot is rendered, update the URI. disposables.add(props.plotClient.onDidCompleteRender((result) => { @@ -102,7 +102,7 @@ export const DynamicPlotInstance = (props: DynamicPlotInstanceProps) => { })); // Re-render if the sizing policy changes. - disposables.add(plotsContext.positronPlotsService.onDidChangeSizingPolicy((policy) => { + disposables.add(props.plotClient.onDidChangeSizingPolicy((policy) => { render(policy); })); diff --git a/src/vs/workbench/contrib/positronPlots/browser/components/openInEditorMenuButton.tsx b/src/vs/workbench/contrib/positronPlots/browser/components/openInEditorMenuButton.tsx index 16151b0c1a6..204979b97f6 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/components/openInEditorMenuButton.tsx +++ b/src/vs/workbench/contrib/positronPlots/browser/components/openInEditorMenuButton.tsx @@ -55,7 +55,6 @@ export const OpenInEditorMenuButton = (props: OpenInEditorMenuButtonProps) => { const [actions, setActions] = useState([]); const openEditorPlotHandler = useCallback((groupType: number) => { - // props.plotsService.openEditor(groupType); props.commandService.executeCommand(PlotsEditorAction.ID, groupType); setDefaultEditorAction(groupType); }, [props.commandService]); diff --git a/src/vs/workbench/contrib/positronPlots/browser/components/sizingPolicyMenuButton.tsx b/src/vs/workbench/contrib/positronPlots/browser/components/sizingPolicyMenuButton.tsx index 9d30ca4d307..acabd9fa6e2 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/components/sizingPolicyMenuButton.tsx +++ b/src/vs/workbench/contrib/positronPlots/browser/components/sizingPolicyMenuButton.tsx @@ -43,7 +43,7 @@ export const SizingPolicyMenuButton = (props: SizingPolicyMenuButtonProps) => { // State. const [activePolicyLabel, setActivePolicyLabel] = - React.useState(props.plotsService.selectedSizingPolicy.getName(props.plotClient)); + React.useState(props.plotClient.sizingPolicy.getName(props.plotClient)); React.useEffect(() => { const disposables = new DisposableStore(); @@ -76,7 +76,7 @@ export const SizingPolicyMenuButton = (props: SizingPolicyMenuButtonProps) => { let policyDisposables = attachPolicy(props.plotsService.selectedSizingPolicy); // Update the active policy label when the selected policy changes. - disposables.add(props.plotsService.onDidChangeSizingPolicy(policy => { + disposables.add(props.plotClient.onDidChangeSizingPolicy(policy => { policyDisposables.dispose(); policyDisposables = attachPolicy(policy); })); @@ -106,7 +106,7 @@ export const SizingPolicyMenuButton = (props: SizingPolicyMenuButtonProps) => { enabled, checked: policy.id === selectedPolicy.id, run: () => { - props.plotsService.selectSizingPolicy(policy.id); + props.plotClient.sizingPolicy = policy; } }); } @@ -125,7 +125,7 @@ export const SizingPolicyMenuButton = (props: SizingPolicyMenuButtonProps) => { enabled: true, checked: customPolicy.id === selectedPolicy.id, run: () => { - props.plotsService.selectSizingPolicy(customPolicy.id); + props.plotClient.sizingPolicy = customPolicy; } }); } @@ -162,6 +162,7 @@ export const SizingPolicyMenuButton = (props: SizingPolicyMenuButtonProps) => { } else { // The user entered a valid size; set the custom policy. props.plotsService.setCustomPlotSize(result.size); + props.plotClient.sizingPolicy = new PlotSizingPolicyCustom(result.size); } } } diff --git a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx index 3ecf6a503e4..a7de658e848 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx +++ b/src/vs/workbench/contrib/positronPlots/browser/modalDialogs/savePlotModalDialog.tsx @@ -274,7 +274,7 @@ const SavePlotModalDialog = (props: SavePlotModalDialogProps) => { } size = { height: height.value, width: width.value }; } - return props.plotClient.render(size, dpi.value / BASE_DPI, format, true); + return props.plotClient.renderWithSizingPolicy(size, dpi.value / BASE_DPI, format, true); }; let intrinsicWidth = ''; diff --git a/src/vs/workbench/contrib/positronPlots/browser/positronPlots.contribution.ts b/src/vs/workbench/contrib/positronPlots/browser/positronPlots.contribution.ts index a0c475645a3..ce4dd18a81d 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/positronPlots.contribution.ts +++ b/src/vs/workbench/contrib/positronPlots/browser/positronPlots.contribution.ts @@ -18,7 +18,7 @@ import { IPositronPlotsService, POSITRON_PLOTS_VIEW_ID } from '../../../services import { IWorkbenchContributionsRegistry, Extensions as WorkbenchExtensions, IWorkbenchContribution } from '../../../common/contributions.js'; import { Extensions as ViewContainerExtensions, IViewsRegistry } from '../../../common/views.js'; import { registerAction2 } from '../../../../platform/actions/common/actions.js'; -import { PlotsActiveEditorCopyAction, PlotsActiveEditorSaveAction, PlotsClearAction, PlotsCopyAction, PlotsEditorAction, PlotsNextAction, PlotsPopoutAction, PlotsPreviousAction, PlotsRefreshAction, PlotsSaveAction } from './positronPlotsActions.js'; +import { PlotsActiveEditorCopyAction, PlotsActiveEditorSaveAction, PlotsClearAction, PlotsCopyAction, PlotsEditorAction, PlotsNextAction, PlotsPopoutAction, PlotsPreviousAction, PlotsRefreshAction, PlotsSaveAction, PlotsSizingPolicyAction } from './positronPlotsActions.js'; import { POSITRON_SESSION_CONTAINER } from '../../positronSession/browser/positronSessionContainer.js'; // Register the Positron plots service. @@ -72,6 +72,7 @@ class PositronPlotsContribution extends Disposable implements IWorkbenchContribu registerAction2(PlotsEditorAction); registerAction2(PlotsActiveEditorCopyAction); registerAction2(PlotsActiveEditorSaveAction); + registerAction2(PlotsSizingPolicyAction); } } diff --git a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsActions.ts b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsActions.ts index 9016575e6da..3848702ca98 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsActions.ts +++ b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsActions.ts @@ -14,17 +14,18 @@ import { ContextKeyExpr } from '../../../../platform/contextkey/common/contextke import { IsDevelopmentContext } from '../../../../platform/contextkey/common/contextkeys.js'; import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js'; import { INotificationService } from '../../../../platform/notification/common/notification.js'; -import { IQuickInputService, IQuickPickItem } from '../../../../platform/quickinput/common/quickInput.js'; +import { IQuickInputService, IQuickPick, IQuickPickItem } from '../../../../platform/quickinput/common/quickInput.js'; import { PLOT_IS_ACTIVE_EDITOR, POSITRON_EDITOR_PLOTS } from '../../positronPlotsEditor/browser/positronPlotsEditor.contribution.js'; import { PositronPlotsEditorInput } from '../../positronPlotsEditor/browser/positronPlotsEditorInput.js'; import { IEditorService } from '../../../services/editor/common/editorService.js'; -import { IPositronPlotsService } from '../../../services/positronPlots/common/positronPlots.js'; +import { IPositronPlotClient, IPositronPlotsService } from '../../../services/positronPlots/common/positronPlots.js'; +import { PlotClientInstance } from '../../../services/languageRuntime/common/languageRuntimePlotClient.js'; +import { ThemeIcon } from '../../../../base/common/themables.js'; export enum PlotActionTarget { VIEW = 'view', ACTIVE_EDITOR = 'activeEditor', } - export const POSITRON_PLOTS_ACTION_CATEGORY = nls.localize('positronPlotsCategory', "Plots"); const category: ILocalizedString = { value: POSITRON_PLOTS_ACTION_CATEGORY, original: 'Plots' }; @@ -34,6 +35,8 @@ const category: ILocalizedString = { value: POSITRON_PLOTS_ACTION_CATEGORY, orig * has a plot. */ abstract class AbstractPlotsAction extends Action2 { + quickPickService?: IQuickInputService; + constructor(descriptor: IAction2Options) { super(descriptor); } @@ -64,8 +67,8 @@ abstract class AbstractPlotsAction extends Action2 { const plotsService = accessor.get(IPositronPlotsService); const notificationService = accessor.get(INotificationService); const editorService = accessor.get(IEditorService); - const quickPick = accessor.get(IQuickInputService); const configurationService = accessor.get(IConfigurationService); + this.quickPickService = accessor.get(IQuickInputService); const editorPlotsEnabled = Boolean(configurationService.getValue(POSITRON_EDITOR_PLOTS)); @@ -84,7 +87,7 @@ abstract class AbstractPlotsAction extends Action2 { if (target) { this.executeTargetAction(target, plotsService, editorService, notificationService); } else { - const quickPicker = quickPick.createQuickPick(); + const quickPicker = this.quickPickService.createQuickPick(); quickPicker.items = quickPickItems; quickPicker.ignoreFocusOut = true; @@ -110,6 +113,15 @@ abstract class AbstractPlotsAction extends Action2 { } } + /** + * A filter to determine if the plot should be included in the action. + * + * @returns true if the plot should be included + */ + protected plotActionFilter(_plotClient: IPositronPlotClient): boolean { + return true; + } + /** * Gets the active editor plot id. * @@ -134,13 +146,14 @@ abstract class AbstractPlotsAction extends Action2 { const items: IQuickPickItem[] = []; if (plotsService.selectedPlotId) { - items.push( - { + const plotClient = plotsService.positronPlotInstances.find(p => p.id === plotsService.selectedPlotId); + if (plotClient && this.plotActionFilter(plotClient)) { + items.push({ id: PlotActionTarget.VIEW, label: localize('positronPlots.copyPlotsView', 'From Plots View'), ariaLabel: localize('positronPlots.copyPlotsView', 'From Plots View'), - } - ); + }); + } } editorService.editors.forEach(input => { @@ -148,11 +161,14 @@ abstract class AbstractPlotsAction extends Action2 { const name = input.getName(); const plotId = input.resource?.path.toString(); if (plotId) { - items.push({ - id: plotId, - label: localize('positronPlots.copyEditor', 'Editor: {0}', name), - ariaLabel: localize('positronPlots.copyEditor', 'Editor: {0}', name), - }); + const plotClient = plotsService.getEditorInstance(plotId); + if (plotClient && this.plotActionFilter(plotClient)) { + items.push({ + id: plotId, + label: localize('positronPlots.copyEditor', 'Editor: {0}', name), + ariaLabel: localize('positronPlots.copyEditor', 'Editor: {0}', name), + }); + } } } }); @@ -418,7 +434,7 @@ export class PlotsEditorAction extends Action2 { async run(accessor: ServicesAccessor, groupType?: number) { const plotsService = accessor.get(IPositronPlotsService); if (plotsService.selectedPlotId) { - plotsService.openEditor(groupType); + plotsService.openEditor(plotsService.selectedPlotId, groupType); } else { accessor.get(INotificationService).info(localize('positronPlots.noPlotSelected', 'No plot selected.')); } @@ -498,3 +514,120 @@ export class PlotsActiveEditorSaveAction extends Action2 { commandService.executeCommand(PlotsSaveAction.ID, PlotActionTarget.ACTIVE_EDITOR); } } + +/** Action to change the plot's sizing policy */ +export class PlotsSizingPolicyAction extends AbstractPlotsAction { + static ID = 'workbench.action.positronPlots.sizingPolicy'; + sizingPicker?: IQuickPick; + + constructor() { + super({ + id: PlotsSizingPolicyAction.ID, + title: localize2('positronPlots.sizingPolicy', 'Change Plot Sizing Policy'), + category, + f1: true, + }); + } + + override run(accessor: ServicesAccessor, target?: PlotActionTarget): Promise { + return super.run(accessor, target); + } + + override executeQuickPick(quickPick: IQuickPickItem, plotsService: IPositronPlotsService, editorService: IEditorService, notificationService: INotificationService): void { + const plotId = quickPick.id; + + if (!plotId) { + notificationService.info(localize('positronPlots.noPlotSelected', 'No plot selected.')); + return; + } + + const isView = plotId === PlotActionTarget.VIEW; + + if (isView) { + this.executeTargetAction(PlotActionTarget.VIEW, plotsService, editorService, notificationService); + } else { + const plotClient = plotsService.getEditorInstance(plotId); + if (plotClient instanceof PlotClientInstance) { + if (!this.quickPickService) { + return; + } + + this.getSizingPolicy(plotsService, editorService, () => { + if (!this.sizingPicker) { + return; + } + + const selectedItem = this.sizingPicker.selectedItems[0]; + if (selectedItem?.id) { + plotsService.setEditorSizingPolicy(plotId, selectedItem.id); + } + }); + } + } + } + + override executeTargetAction(target: PlotActionTarget, plotsService: IPositronPlotsService, editorService: IEditorService, notificationService: INotificationService): void { + this.getSizingPolicy(plotsService, editorService, () => { + if (!this.sizingPicker) { + return; + } + + const selectedItem = this.sizingPicker.selectedItems[0]; + if (selectedItem?.id) { + if (target === PlotActionTarget.VIEW) { + plotsService.selectSizingPolicy(selectedItem.id); + } else { + const plotId = this.getActiveEditorPlotId(editorService); + if (plotId) { + plotsService.setEditorSizingPolicy(plotId, selectedItem.id); + } + } + } + }); + } + + private getSizingPolicy(plotsService: IPositronPlotsService, editorService: IEditorService, onAccept: () => void): void { + if (!this.quickPickService) { + return; + } + + const sizingItems = this.createSizingItems(plotsService); + this.sizingPicker = this.quickPickService.createQuickPick(); + + this.sizingPicker.items = sizingItems; + this.sizingPicker.title = localize('positronPlots.action.selectSizingPolicy', 'Select a sizing policy'); + + this.sizingPicker.show(); + + this.sizingPicker.onDidAccept(() => { + onAccept(); + this.sizingPicker?.hide(); + }); + } + + private createSizingItems(plotsService: IPositronPlotsService, plotId?: string): IQuickPickItem[] { + const items: IQuickPickItem[] = []; + const plotClient = plotId ? plotsService.getEditorInstance(plotId) + : plotsService.positronPlotInstances.find(p => p.id === plotsService.selectedPlotId) as PlotClientInstance; + + if (!plotClient || !(plotClient instanceof PlotClientInstance)) { + throw new Error('Plot not found'); + } + + plotsService.sizingPolicies.forEach(policy => { + items.push({ + id: policy.id, + label: policy.getName(plotClient), + ariaLabel: policy.getName(plotClient), + iconClass: plotClient.sizingPolicy.id === policy.id ? ThemeIcon.asClassName(Codicon.positronCheckMark) + : ThemeIcon.asClassName(Codicon.blank), + }); + }); + + return items; + } + + override plotActionFilter(plotClient: IPositronPlotClient): boolean { + return plotClient instanceof PlotClientInstance; + } +} diff --git a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts index e645eed75a2..e11296f6177 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts +++ b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts @@ -5,7 +5,7 @@ import * as DOM from '../../../../base/browser/dom.js'; import { Disposable } from '../../../../base/common/lifecycle.js'; -import { IPositronPlotMetadata, PlotClientInstance } from '../../../services/languageRuntime/common/languageRuntimePlotClient.js'; +import { IPositronPlotMetadata, PlotClientInstance, PlotClientLocation } from '../../../services/languageRuntime/common/languageRuntimePlotClient.js'; import { ILanguageRuntimeMessageOutput, LanguageRuntimeSessionMode, RuntimeOutputKind } from '../../../services/languageRuntime/common/languageRuntimeService.js'; import { ILanguageRuntimeSession, IRuntimeClientInstance, IRuntimeSessionService, RuntimeClientType } from '../../../services/runtimeSession/common/runtimeSessionService.js'; import { HTMLFileSystemProvider } from '../../../../platform/files/browser/htmlFileSystemProvider.js'; @@ -92,9 +92,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe /** The list of sizing policies. */ private readonly _sizingPolicies: IPositronPlotSizingPolicy[] = []; - /** The emitter for the onDidChangeSizingPolicy event */ - private readonly _onDidChangeSizingPolicy = new Emitter(); - /** The emitter for the onDidChangeHistoryPolicy event */ private readonly _onDidChangeHistoryPolicy = new Emitter(); @@ -188,6 +185,10 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // Listen for plots being selected and update the selected plot ID this._register(this._onDidSelectPlot.event((id) => { this._selectedPlotId = id; + const selectedPlot = this._plots.find((plot) => plot.id === id); + if (selectedPlot instanceof PlotClientInstance) { + this._selectedSizingPolicy = selectedPlot.sizingPolicy; + } })); // Listen for plot clients being created by the IPyWidget service and register them with the plots service @@ -204,7 +205,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // When the storage service is about to save state, store the current history policy // and storage policy in the workspace storage. this._register(this._storageService.onWillSaveState(() => { - this._storageService.store( HistoryPolicyStorageKey, this._selectedHistoryPolicy, @@ -289,18 +289,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe } } - // See if there's a preferred sizing policy in storage, and select it if so - const preferredSizingPolicyId = this._storageService.get( - SizingPolicyStorageKey, - StorageScope.WORKSPACE); - if (preferredSizingPolicyId) { - const policy = this._sizingPolicies.find( - policy => policy.id === preferredSizingPolicyId); - if (policy) { - this._selectedSizingPolicy = policy; - } - } - // See if there's a preferred history policy in storage, and select it if so const preferredHistoryPolicy = this._storageService.get( HistoryPolicyStorageKey, @@ -404,7 +392,24 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe throw new Error(`Invalid sizing policy ID: ${id}`); } this._selectedSizingPolicy = policy; - this._onDidChangeSizingPolicy.fire(policy); + const selectedPlot = this._plots.find((plot) => this.selectedPlotId === plot.id); + if (selectedPlot instanceof PlotClientInstance) { + selectedPlot.sizingPolicy = policy; + } + } + + setEditorSizingPolicy(plotId: string, policyId: string): void { + const plot = this._editorPlots.get(plotId); + if (plot instanceof PlotClientInstance) { + const policy = this._sizingPolicies.find(policy => policy.id === policyId); + if (policy) { + plot.sizingPolicy = policy; + } else { + this._notificationService.error(localize('positronPlots.sizing.invalidSizingPolicy', 'Invalid sizing policy: {0}', policyId)); + } + } else { + this._notificationService.error(localize('positronPlots.sizing.invalidPlotType', 'Cannot set size for this plot type')); + } } /** @@ -425,7 +430,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe this._sizingPolicies.push(policy); this._selectedSizingPolicy = policy; this._customSizingPolicy = policy; - this._onDidChangeSizingPolicy.fire(policy); } /** @@ -446,7 +450,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // sizing policy. if (currentPolicy) { this._selectedSizingPolicy = new PlotSizingPolicyAuto(); - this._onDidChangeSizingPolicy.fire(this._selectedSizingPolicy); } } } @@ -495,11 +498,13 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe try { const metadata = JSON.parse(storedMetadata) as IPositronPlotMetadata; const commProxy = this.createCommProxy(client, metadata); - plotClients.push(this.createRuntimePlotClient(commProxy)); + plotClients.push(this.createRuntimePlotClient(commProxy, metadata)); registered = true; + } catch (error) { console.warn(`Error parsing plot metadata: ${error}`); } + } // If we don't have metadata, register the plot with a default metadata object if (!registered) { @@ -509,9 +514,10 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe session_id: session.sessionId, parent_id: '', code: '', + location: PlotClientLocation.View, }; const commProxy = this.createCommProxy(client, metadata); - plotClients.push(this.createRuntimePlotClient(commProxy)); + plotClients.push(this.createRuntimePlotClient(commProxy, metadata)); } } else { console.warn( @@ -533,6 +539,8 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // events. plotClients.forEach((client) => { this.registerPlotClient(client, false); + // Check if the plot also needs to be restored to an editor tab + this.restoreEditorPlot(client.metadata.id, session.sessionId, this._plotCommProxies.get(client.metadata.id)!); }); // Re-sort the plots by creation time since we may have added new ones that are @@ -586,17 +594,9 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe code, }; - // Save the metadata to storage so that we can restore it when - // the plot is reconnected. - this._storageService.store( - this.generateStorageKey(metadata.session_id, metadata.id), - JSON.stringify(metadata), - StorageScope.WORKSPACE, - StorageTarget.MACHINE); - // Register the plot client const commProxy = this.createCommProxy(event.client, metadata); - const plotClient = this.createRuntimePlotClient(commProxy); + const plotClient = this.createRuntimePlotClient(commProxy, metadata); this.registerPlotClient(plotClient, true); // Raise the Plots pane so the plot is visible. @@ -636,6 +636,51 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe } } + /** + * Check if the stored metadata has a plot for an editor and restore it. + */ + private restoreEditorPlot(plotId: string, sessionId: string, commProxy: PositronPlotCommProxy) { + const metadataKey = this.generateStorageKey(sessionId, plotId, PlotClientLocation.Editor); + const storedMetadata = this._storageService.get(metadataKey, StorageScope.WORKSPACE); + + if (storedMetadata) { + try { + const metadata = JSON.parse(storedMetadata) as IPositronPlotMetadata; + this.createEditorPlot(metadata, commProxy); + + this.openEditor(plotId, this.getPreferredEditorGroup(), metadata); + } catch (error) { + console.warn(`Error parsing plot metadata: ${error}`); + } + } + } + + private createEditorPlot(metadata: IPositronPlotMetadata, commProxy: PositronPlotCommProxy) { + const plot = this.createRuntimePlotClient(commProxy, metadata, PlotClientLocation.Editor); + plot.onDidClose(() => { + this._editorPlots.delete(metadata.id); + this._storageService.remove( + this.generateStorageKey(metadata.session_id, metadata.id, metadata.location), + StorageScope.WORKSPACE); + }); + this._editorPlots.set(metadata.id, plot); + } + + /** + * Save the metadata to storage so that we can restore it when + * the plot is reconnected. + * + * @param metadata the plot metadata + */ + private storePlotMetadata(metadata: IPositronPlotMetadata) { + this._storageService.store( + this.generateStorageKey(metadata.session_id, metadata.id, metadata.location), + JSON.stringify(metadata), + StorageScope.WORKSPACE, + StorageTarget.MACHINE + ); + } + /** * Creates a new plot client instance wrapper and registers it with the * service. @@ -656,14 +701,13 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // Remove the plot from our list when it is closed this._register(plotClient.onDidClose(() => { - this.unregisterPlotClient(plotClient); const index = this._plots.indexOf(plotClient); if (index >= 0) { this._plots.splice(index, 1); } // Clear the plot's metadata from storage this._storageService.remove( - this.generateStorageKey(plotClient.metadata.session_id, plotClient.metadata.id), + this.generateStorageKey(plotClient.metadata.session_id, plotClient.metadata.id, plotClient.metadata.location), StorageScope.WORKSPACE); })); @@ -726,7 +770,6 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe onDidSelectPlot: Event = this._onDidSelectPlot.event; onDidRemovePlot: Event = this._onDidRemovePlot.event; onDidReplacePlots: Event = this._onDidReplacePlots.event; - onDidChangeSizingPolicy: Event = this._onDidChangeSizingPolicy.event; onDidChangeHistoryPolicy: Event = this._onDidChangeHistoryPolicy.event; // Gets the individual plot instances. @@ -806,6 +849,14 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe this._onDidRemovePlot.fire(id); } + removeEditorPlot(id: string): void { + const plot = this._editorPlots.get(id); + if (plot) { + this.unregisterPlotClient(plot); + this._editorPlots.delete(id); + } + } + /** * Removes the currently selected plot from the service and fires an event * to update the the UI @@ -995,8 +1046,8 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe * @param runtimeId The ID of the runtime that owns the plot. * @param plotId The ID of the plot itself. */ - private generateStorageKey(sessionId: string, plotId: string): string { - return `positron.plot.${sessionId}.${plotId}`; + private generateStorageKey(sessionId: string, plotId: string, location = PlotClientLocation.View): string { + return `positron.plot.${sessionId}.${plotId}.${location}`; } /** @@ -1074,34 +1125,34 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe this._showPlotsPane(); } - public async openEditor(groupType?: number): Promise { - const plotClient = this._plots.find(plot => plot.id === this.selectedPlotId); + public async openEditor(plotId: string, groupType?: number, metadata?: IPositronPlotMetadata): Promise { + const plotClient = this._editorPlots.get(plotId) ?? this._plots.find(plot => plot.id === this.selectedPlotId); + + + if (!plotClient && !metadata) { + throw new Error('Cannot open plot in editor: plot not found'); + } if (plotClient instanceof WebviewPlotClient) { throw new Error('Cannot open plot in editor: webview plot not supported'); } - let plotId: string | undefined; if (plotClient instanceof StaticPlotClient) { - plotId = plotClient.id; this._editorPlots.set(plotClient.id, plotClient); } - if (plotClient instanceof PlotClientInstance) { - plotId = plotClient.id; + + // Create a new plot client instance for the editor + if (plotClient instanceof PlotClientInstance && plotClient.metadata.location === PlotClientLocation.View) { + metadata = metadata ?? plotClient.metadata; const commProxy = this._plotCommProxies.get(plotId); if (commProxy) { - const editorPlotClient = this.createRuntimePlotClient(commProxy); - this._editorPlots.set(editorPlotClient.id, editorPlotClient); + this.createEditorPlot(metadata, commProxy); } else { throw new Error('Cannot open plot in editor: plot comm not found'); } } - if (!plotId) { - throw new Error('Cannot open plot in editor: plot not found'); - } - - const preferredEditorGroup = this._storageService.getNumber('positronPlots.defaultEditorAction', StorageScope.WORKSPACE, ACTIVE_GROUP); + const preferredEditorGroup = this.getPreferredEditorGroup(); const selectedEditorGroup = groupType ?? preferredEditorGroup; const editorPane = await this._editorService.openEditor({ resource: URI.from({ @@ -1149,7 +1200,7 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe } private createCommProxy(client: IRuntimeClientInstance, metadata: IPositronPlotMetadata) { - const commProxy = new PositronPlotCommProxy(client, metadata); + const commProxy = new PositronPlotCommProxy(client); this._plotCommProxies.set(metadata.id, commProxy); this._register(commProxy.onDidClose(() => { @@ -1167,17 +1218,25 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe return commProxy; } - private createRuntimePlotClient(comm: PositronPlotCommProxy) { - const plotClient = new PlotClientInstance(comm, comm.metadata); - let plotClients = this._plotClientsByComm.get(comm.metadata.id); + private createRuntimePlotClient(comm: PositronPlotCommProxy, metadata: IPositronPlotMetadata, location: PlotClientLocation = PlotClientLocation.View) { + const sizingPolicy = this._sizingPolicies.find((policy) => policy.id === metadata.sizing_policy?.id) + ?? this._selectedSizingPolicy; + metadata.sizing_policy = { + id: sizingPolicy.id, + size: sizingPolicy instanceof PlotSizingPolicyCustom ? sizingPolicy.size : undefined + }; + const plotClient = new PlotClientInstance(comm, sizingPolicy ?? this._selectedSizingPolicy, { ...metadata, location: location }); + let plotClients = this._plotClientsByComm.get(metadata.id); if (!plotClients) { plotClients = []; - this._plotClientsByComm.set(comm.metadata.id, plotClients); + this._plotClientsByComm.set(metadata.id, plotClients); } plotClients.push(plotClient); + this.storePlotMetadata({ ...metadata, location }); + return plotClient; } diff --git a/src/vs/workbench/contrib/positronPlots/test/electron-sandbox/positronPlotsService.test.ts b/src/vs/workbench/contrib/positronPlots/test/electron-sandbox/positronPlotsService.test.ts index effa8622766..662257ae556 100644 --- a/src/vs/workbench/contrib/positronPlots/test/electron-sandbox/positronPlotsService.test.ts +++ b/src/vs/workbench/contrib/positronPlots/test/electron-sandbox/positronPlotsService.test.ts @@ -4,15 +4,20 @@ *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; +import * as sinon from 'sinon'; + import { raceTimeout } from '../../../../../base/common/async.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { PositronTestServiceAccessor, positronWorkbenchInstantiationService as positronWorkbenchInstantiationService } from '../../../../test/browser/positronWorkbenchTestServices.js'; -import { IPositronPlotMetadata } from '../../../../services/languageRuntime/common/languageRuntimePlotClient.js'; +import { IPositronPlotMetadata, PlotClientInstance } from '../../../../services/languageRuntime/common/languageRuntimePlotClient.js'; import { HistoryPolicy, IPositronPlotClient, IPositronPlotsService } from '../../../../services/positronPlots/common/positronPlots.js'; import { RuntimeClientType } from '../../../../services/runtimeSession/common/runtimeSessionService.js'; import { TestLanguageRuntimeSession } from '../../../../services/runtimeSession/test/common/testLanguageRuntimeSession.js'; import { startTestLanguageRuntimeSession } from '../../../../services/runtimeSession/test/common/testRuntimeSessionService.js'; +import { PositronPlotCommProxy } from '../../../../services/languageRuntime/common/positronPlotCommProxy.js'; +import { PlotSizingPolicyAuto } from '../../../../services/positronPlots/common/sizingPolicyAuto.js'; +import { PlotSizingPolicyFill } from '../../../../services/positronPlots/common/sizingPolicyFill.js'; suite('Positron - Plots Service', () => { @@ -106,26 +111,35 @@ suite('Positron - Plots Service', () => { }); test('sizing policy: change event', async () => { - let sizingPolicyChanged = 0; - - const didChangeSizingPolicy = new Promise((resolve) => { - const disposable = plotsService.onDidChangeSizingPolicy((e) => { - sizingPolicyChanged++; + const plotCommProxyStub = sinon.createStubInstance(PositronPlotCommProxy); + // Creates the properties on the stub instance before stubbing them + (plotCommProxyStub as any).onDidClose = null; + (plotCommProxyStub as any).onDidRenderUpdate = null; + (plotCommProxyStub as any).onDidShowPlot = null; + + sinon.stub(plotCommProxyStub, 'onDidClose').value(() => { }); + sinon.stub(plotCommProxyStub, 'onDidRenderUpdate').value(() => { }); + sinon.stub(plotCommProxyStub, 'onDidShowPlot').value(() => { }); + + const plotClientInstance = new PlotClientInstance(plotCommProxyStub as unknown as PositronPlotCommProxy, new PlotSizingPolicyAuto(), {} as IPositronPlotMetadata); + disposables.add(plotClientInstance); + + let sizingPolicyChanged = false; + const didClosePlot = new Promise((resolve) => { + const disposable = plotClientInstance.onDidChangeSizingPolicy(() => { + sizingPolicyChanged = true; resolve(); }); disposables.add(disposable); }); - // no event since 'auto' is the default - plotsService.selectSizingPolicy('auto'); - assert.strictEqual(plotsService.selectedSizingPolicy.id, 'auto'); + plotClientInstance.sizingPolicy = new PlotSizingPolicyFill(); - // event occurs when changing to 'fill' - plotsService.selectSizingPolicy('fill'); - assert.strictEqual(plotsService.selectedSizingPolicy.id, 'fill'); + await raceTimeout(didClosePlot, 100, () => assert.fail('onDidChangeSizingPolicy event did not fire')); + + assert.ok(sizingPolicyChanged, 'onDidChangeSizingPolicy event should fire'); - await raceTimeout(didChangeSizingPolicy, 100, () => assert.fail('onDidChangeSizingPolicy event did not fire')); - assert.strictEqual(sizingPolicyChanged, 1, 'onDidChangeSizingPolicy event should fire once for changing to "fill"'); + sinon.restore(); }); test('selection: select plot', async () => { diff --git a/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx b/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx index 0df2e760971..d8265e99ae7 100644 --- a/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx +++ b/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditor.tsx @@ -164,7 +164,6 @@ export class PositronPlotsEditor extends EditorPane implements IPositronPlotsEdi context: IEditorOpenContext, token: CancellationToken ): Promise { - this._plotClient?.dispose(); this._plotClient = this._positronPlotsService.getEditorInstance(input.resource.path); if (!this._plotClient) { throw new Error('Plot client not found'); diff --git a/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditorInput.ts b/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditorInput.ts index c2ede5e8b74..5ab5ddbe344 100644 --- a/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditorInput.ts +++ b/src/vs/workbench/contrib/positronPlotsEditor/browser/positronPlotsEditorInput.ts @@ -23,7 +23,7 @@ export class PositronPlotsEditorInput extends EditorInput { override dispose(): void { const plotClient = this._positronPlotsService.getEditorInstance(this.resource.path); if (plotClient) { - this._positronPlotsService.unregisterPlotClient(plotClient); + this._positronPlotsService.removeEditorPlot(plotClient.id); } super.dispose(); } diff --git a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts index ce017bd4af2..f716ea6fae5 100644 --- a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts +++ b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts @@ -7,8 +7,9 @@ import { Disposable } from '../../../../base/common/lifecycle.js'; import { Event, Emitter } from '../../../../base/common/event.js'; import { IPositronPlotClient } from '../../positronPlots/common/positronPlots.js'; import { IntrinsicSize, RenderFormat } from './positronPlotComm.js'; -import { IPlotSize } from '../../positronPlots/common/sizingPolicy.js'; +import { IPlotSize, IPositronPlotSizingPolicy } from '../../positronPlots/common/sizingPolicy.js'; import { DeferredRender, IRenderedPlot, PositronPlotCommProxy, RenderRequest } from './positronPlotCommProxy.js'; +import { PlotSizingPolicyCustom } from '../../positronPlots/common/sizingPolicyCustom.js'; export enum PlotClientLocation { /** The plot is in the editor */ @@ -53,6 +54,15 @@ export interface IPositronPlotMetadata { /** The ID of the runtime session that created the plot */ session_id: string; + + /** The sizing policy for the plot. This may not be present with older metadata. */ + sizing_policy?: { + id: string; + size?: IPlotSize; + }; + + /** The plot's location for display. */ + location?: PlotClientLocation; } /** @@ -130,6 +140,12 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien onDidSetIntrinsicSize: Event; private readonly _didSetIntrinsicSizeEmitter = new Emitter(); + /** + * Event that fires when the sizing policy is changed. + */ + onDidChangeSizingPolicy: Event; + private readonly _sizingPolicyEmitter = new Emitter; + /** * Creates a new plot client instance. * @@ -138,6 +154,7 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien */ constructor( private readonly _commProxy: PositronPlotCommProxy, + private _sizingPolicy: IPositronPlotSizingPolicy, public readonly metadata: IPositronPlotMetadata) { super(); @@ -166,6 +183,9 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien // Connect the intrinsic size emitter event this.onDidSetIntrinsicSize = this._didSetIntrinsicSizeEmitter.event; + // Connect the sizing policy emitter event + this.onDidChangeSizingPolicy = this._sizingPolicyEmitter.event; + // Listen to our own state changes this._register(this.onDidChangeState((state) => { this._state = state; @@ -192,6 +212,23 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien return this._commProxy.getIntrinsicSize(); } + get sizingPolicy() { + return this._sizingPolicy; + } + + set sizingPolicy(newSizingPolicy: IPositronPlotSizingPolicy) { + this._sizingPolicy = newSizingPolicy; + this.metadata.sizing_policy = { + id: newSizingPolicy.id, + size: newSizingPolicy instanceof PlotSizingPolicyCustom ? newSizingPolicy.size : undefined + }; + this._sizingPolicyEmitter.fire(newSizingPolicy); + } + + public renderWithSizingPolicy(size: IPlotSize | undefined, pixel_ratio: number, format = RenderFormat.Png, preview = false): Promise { + return this.render(size ? this._sizingPolicy.getPlotSize(size) : size, pixel_ratio, format, preview); + } + /** * Requests that the plot be rendered at a specific size. * @@ -375,4 +412,9 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien this.scheduleRender(req, 0); return req.promise; } + + override dispose(): void { + this._closeEmitter.fire(); + super.dispose(); + } } diff --git a/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts b/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts index 4c3896c82cd..959c1f6276e 100644 --- a/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts +++ b/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts @@ -7,7 +7,6 @@ import { Disposable } from '../../../../base/common/lifecycle.js'; import { DeferredPromise } from '../../../../base/common/async.js'; import { Event, Emitter } from '../../../../base/common/event.js'; import { IRuntimeClientInstance, RuntimeClientState } from './languageRuntimeClientInstance.js'; -import { IPositronPlotMetadata } from './languageRuntimePlotClient.js'; import { IntrinsicSize, PositronPlotComm, RenderFormat } from './positronPlotComm.js'; import { IPlotSize } from '../../positronPlots/common/sizingPolicy.js'; @@ -150,8 +149,7 @@ export class PositronPlotCommProxy extends Disposable { private readonly _didSetIntrinsicSizeEmitter = new Emitter(); constructor( - client: IRuntimeClientInstance, - public readonly metadata: IPositronPlotMetadata) { + client: IRuntimeClientInstance) { super(); this._comm = new PositronPlotComm(client, { render: { timeout: 30000 }, get_intrinsic_size: { timeout: 30000 } }); diff --git a/src/vs/workbench/services/positronPlots/common/positronPlots.ts b/src/vs/workbench/services/positronPlots/common/positronPlots.ts index fabf04c0f9f..6e7d5bbeb73 100644 --- a/src/vs/workbench/services/positronPlots/common/positronPlots.ts +++ b/src/vs/workbench/services/positronPlots/common/positronPlots.ts @@ -61,11 +61,6 @@ export interface IPositronPlotsService { */ readonly historyPolicy: HistoryPolicy; - /** - * Notifies subscribers when the sizing policy has changed. - */ - readonly onDidChangeSizingPolicy: Event; - /** * Notifies subscribers when the history policy has changed. */ @@ -121,6 +116,13 @@ export interface IPositronPlotsService { */ removePlot(id: string): void; + /** + * Remove an editor plot. + * + * @param id The ID of the plot to remove. + */ + removeEditorPlot(id: string): void; + /** * Removes the selected plot. */ @@ -136,6 +138,11 @@ export interface IPositronPlotsService { */ selectSizingPolicy(id: string): void; + /** + * Sets the sizing policy for the plot. + */ + setEditorSizingPolicy(plotId: string, policyId: string): void; + /** * Sets a custom plot size (and selects the custom sizing policy) */ @@ -184,12 +191,14 @@ export interface IPositronPlotsService { saveEditorPlot(plotId: string): void; /** - * Opens the currently selected plot in an editor. + * Opens the given plot in an editor. * + * @param plotId The id of the plot to open in an editor tab. * @param groupType Specify where the editor tab will be opened. Defaults to the preferred + * @param metadata The metadata for the plot. Uses the existing plot client if not provided. * editor group. */ - openEditor(groupType?: number): Promise; + openEditor(plotId: string, groupType?: number, metadata?: IPositronPlotMetadata): Promise; /** * Gets the preferred editor group for opening the plot in an editor tab.