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

Prompt user with dialog if triggering open with on unsaved file #115264

Closed
wants to merge 2 commits into from
Closed

Prompt user with dialog if triggering open with on unsaved file #115264

wants to merge 2 commits into from

Conversation

lramos15
Copy link
Member

Similar to #114947, my fork got deleted so I had to remake the PR and placing this in editor action offers a solution for both notebooks and custom editors. I would be open to other solutions as it feels weird being prompted before the quickpick is opened. I also think it is a little silly that you get prompted even if you select the same editor that you're in as we shouldn't force the user to take any action in that case.

@lramos15 lramos15 requested a review from mjbvz January 27, 2021 20:25
@@ -1913,6 +1915,28 @@ export class ReopenResourcesAction extends Action {

const options = activeEditorPane.options;
const group = activeEditorPane.group;
// The current editor needs to be saved before it can be triggered with open with
if (activeInput.isDirty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this fire even if you select the default VS Code editor? Also, should it only be fired if you select a custom binary editor since a custom text editor should be able to handle this case correctly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mainly wondering if it should be moved into the call to openEditorWith instead after you have selected an editor but before it has actually replaced the old editor

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I think the logic of asking the user for dealing with dirty files should move into the editorOpenWith method which already contains the logic to show a picker etc. The action should simply call that method as before and let the helper deal with it. That way we ensure consistent behaviour in case the method is called from other places.

Besides, we need to make sure that the dialog is consistent with our current dialog when a file is dirty:

image

As you can see, we do not use Abort. The dialog is defined here:

async showSaveConfirm(fileNamesOrResources: (string | URI)[]): Promise<ConfirmResult> {

Ideally you could just use the same dialog for your purpose.

@bpasero
Copy link
Member

bpasero commented Feb 10, 2021

I have added #116272 where I feel we should cleanup editorOpenWith and move its code into respective existing files.

@bpasero
Copy link
Member

bpasero commented Feb 10, 2021

I believe now that with the work in #100281 you do not have to do anything special. If we end up calling IEditorService.replaceEditors, the editor group will take care of prompting the user in case the editor is dirty.

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@bpasero
Copy link
Member

bpasero commented Feb 22, 2021

My comment #115264 (comment) was wrong, but with f294b6f calling replaceEditors will now ask for each dirty editor that get's replaced what to do with it.

@lramos15
Copy link
Member Author

Closing as in theory this is obsolete now due to Ben's changes

@lramos15 lramos15 closed this Feb 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants