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

fix: Cannot quit app when there is a dirty editor (#13164) #13173

Conversation

emilhammarstedtst
Copy link
Contributor

@emilhammarstedtst emilhammarstedtst commented Dec 13, 2023

What it does

fixes issue #13164, TheiaBlueprint 1.44.0 cannot quit app when there is a dirty editor

How to test

  1. Open Theia Blueprint,
  2. Disable auto-save,
  3. Create a new file,
  4. Make the editor dirty,
  5. Quit Theia,
  6. Click don't save when prompted

=> the app quits.

Review checklist

Reminder for reviewers

Contributed by STMicroelectronics

@emilhammarstedtst emilhammarstedtst force-pushed the pr/cannot-quit-app-when-dirty-editor branch 2 times, most recently from 1482909 to 2b8ed54 Compare December 13, 2023 15:37
@emilhammarstedtst emilhammarstedtst marked this pull request as ready for review December 13, 2023 15:38
@emilhammarstedtst
Copy link
Contributor Author

The actual bug was the "cancel for the save untitled editor file location picker"-behavior was incorrectly triggered by "don't save" button as well.

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @emilhammarstedtst. The code appears to work fine, but I have a couple of suggestions.

// Asks the user to confirm whether they want to exit with or without saving the changes
export async function confirmExitWithOrWithoutSaving(captionsToSave: string[], performSave: () => Promise<void>): Promise<boolean> {
export async function confirmExitWithOrWithoutSaving(captionsToSave: string[], performSave?: () => Promise<void>): Promise<confirmExitWithOrWithoutSavingResult> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the parameter be optional? There is one usage in Theia code and it does pass something. If saving is a no-op, the user can always pass in a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -458,11 +458,18 @@ export class ConfirmSaveDialog extends AbstractDialog<boolean | undefined> {

}

export enum confirmExitWithOrWithoutSavingResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is to return undefined from dialogs when the user does not make a choice (aka "the close box"). I don't think we should introduce a new type here, But just use boolean | undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the return type.

@@ -458,11 +458,18 @@ export class ConfirmSaveDialog extends AbstractDialog<boolean | undefined> {

}

export enum confirmExitWithOrWithoutSavingResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should not move this method over to common-frontend-contribution.ts. It only seems to be invoked from there and it seems to me we could simplify the code if we did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it. seems logical and collected.

Contributed by STMicroelectronics

Signed-off-by: Emil HAMMARSTEDT <[email protected]>
@emilhammarstedtst emilhammarstedtst force-pushed the pr/cannot-quit-app-when-dirty-editor branch from 2b8ed54 to e900e2f Compare December 18, 2023 14:08
@tsmaeder tsmaeder merged commit d0a09a9 into eclipse-theia:master Dec 19, 2023
14 checks passed
@emilhammarstedtst emilhammarstedtst deleted the pr/cannot-quit-app-when-dirty-editor branch December 19, 2023 14:41
@vince-fugnitto vince-fugnitto added this to the 1.45.0 milestone Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants