Add saving to plots in editor tabs #5801
Conversation
Create an abstract action to work with plots shown in the view or editor
|
E2E Tests 🚀 ? |
There was a problem hiding this comment.
Working well for me on Mac Desktop! I tried saving a plot from the Plots pane, from an Editor and via the command.
Some code questions and one UX idea for future consideration:
It would be so cool if we could focus or visually highlight the plot currently highlighted by the quick pick, to make it easier to distinguish which plot will be saved.
Some cases:
- if a user happens to have multiple plots open in editors, since they are identified by plot IDs, it can be hard to quickly tell which plot is being selected
- if a user wants to save a plot, but their Plots pane is not visible
For example, if a user uses the arrow keys to highlight the plot starting with ID d425, we could focus that editor and apply some visual highlighting (maybe outlining the editor with a coloured line?).
| /** | ||
| * Executes the action on the quick pick item. | ||
| * | ||
| * @param quickPick quick pick item to execute on | ||
| * @param plotsService the plots service | ||
| * @param editorService the editor service | ||
| * @param notificationService the notification service | ||
| */ | ||
| abstract executeQuickPick(quickPick: IQuickPickItem, plotsService: IPositronPlotsService, | ||
| editorService: IEditorService, notificationService: INotificationService): Promise<void>; |
There was a problem hiding this comment.
editorService seems to be unused
| /** | |
| * Executes the action on the quick pick item. | |
| * | |
| * @param quickPick quick pick item to execute on | |
| * @param plotsService the plots service | |
| * @param editorService the editor service | |
| * @param notificationService the notification service | |
| */ | |
| abstract executeQuickPick(quickPick: IQuickPickItem, plotsService: IPositronPlotsService, | |
| editorService: IEditorService, notificationService: INotificationService): Promise<void>; | |
| /** | |
| * Executes the action on the quick pick item. | |
| * | |
| * @param quickPick quick pick item to execute on | |
| * @param plotsService the plots service | |
| * @param notificationService the notification service | |
| */ | |
| abstract executeQuickPick(quickPick: IQuickPickItem, plotsService: IPositronPlotsService, | |
| notificationService: INotificationService): Promise<void>; |
| } | ||
|
|
||
| override executeQuickPick(quickPick: IQuickPickItem, plotsService: IPositronPlotsService, | ||
| editorService: IEditorService, notificationService: INotificationService): Promise<void> { |
There was a problem hiding this comment.
editorService seems to be unused
| editorService: IEditorService, notificationService: INotificationService): Promise<void> { | |
| notificationService: INotificationService): Promise<void> { |
There was a problem hiding this comment.
Good catch! Too many editors open and I couldn't see the unused service
| const quickPickItems = this.getItems(plotsService, editorService); | ||
| // no need to show the quick pick if there is only one option or editor plots are disabled | ||
| if (quickPickItems.length === 1 || !editorPlotsEnabled) { | ||
| this.executeQuickPick(quickPickItems[0], plotsService, editorService, notificationService); |
There was a problem hiding this comment.
editorService seems to be unused
| this.executeQuickPick(quickPickItems[0], plotsService, editorService, notificationService); | |
| this.executeQuickPick(quickPickItems[0], plotsService, notificationService); |
| } | ||
| } | ||
|
|
||
| return Promise.resolve(); |
There was a problem hiding this comment.
From a quick parse it doesn't look like we have explicitly async code in this file -- do we need the promise pattern in this file?
There was a problem hiding this comment.
I thought I needed this but I don't see a reason for it. I remember rationalizing that it was required but I don't remember why. 🤷
That would be great! I haven't thought much about how to identify the editors easily and that makes the most sense. I think I'd also want a better tab title but just need to figure out which part of the code snippet to put in the title (multi-line execution makes it hard to see which line of code generated the plot). Maybe we really need to assign numbers to the plots just so it's easier to remember over a plot id. |
Address #4361
Refactors the command and action to work with editor tabs or the Plots view. Presents a quick pick when using the command palette if there is more than one plot across the plots view and editor tabs.
This introduces an abstract plot action so that other commands can also have the quick pick functionality with the command palette.
Screen.Recording.2024-12-18.at.12.45.14.PM.mov
@:plots
QA Notes
A new button will be available in the editor action bar for saving the editor tab plot.