Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add saving to plots in editor tabs #5801

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

timtmok
Copy link
Contributor

@timtmok timtmok commented Dec 18, 2024

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.

Create an abstract action to work with plots shown in the view or editor
Copy link

github-actions bot commented Dec 18, 2024

E2E Tests 🚀  ?
This PR will run tests tagged with: @critical @plots

@timtmok timtmok marked this pull request as ready for review December 18, 2024 17:47
Copy link
Member

@sharon-wang sharon-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

image

Comment on lines +41 to +50
/**
* 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>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

editorService seems to be unused

Suggested change
/**
* 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

editorService seems to be unused

Suggested change
editorService: IEditorService, notificationService: INotificationService): Promise<void> {
notificationService: INotificationService): Promise<void> {

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

editorService seems to be unused

Suggested change
this.executeQuickPick(quickPickItems[0], plotsService, editorService, notificationService);
this.executeQuickPick(quickPickItems[0], plotsService, notificationService);

}
}

return Promise.resolve();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants