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

custom-editor: fix closing dirty custom text editor issue #11593

Merged
merged 8 commits into from
Sep 19, 2022
Merged

custom-editor: fix closing dirty custom text editor issue #11593

merged 8 commits into from
Sep 19, 2022

Conversation

safisa
Copy link
Contributor

@safisa safisa commented Aug 22, 2022

What it does

Fix the issue: Closing a dirty/modified custom editor based on CustomTextEditorProvider doesn't ask the user to save, it just saves it on close (#11592)

How to test

  1. use the cat scratch custom editor extension example from vscode (you can download it from this issue custom-editor: restore editor as part of layout #10787)
  2. open the example.cscratch file
  3. do some changes in the custom editor UI (add some cats by clicking on the scratch button)
  4. close the dirty tab, you need to see the popup asking to save the file.

Review checklist

Reminder for reviewers

safisa added 3 commits August 22, 2022 14:52
… on CustomTextEditorProvider doesn't ask the user to save, it just save it on close

#11592

The issue is that the autoSave member was undefined in the CustomTextEditorModel class, and it is used in the shouldSave method in saveable.ts file. The shouldSave method is called on close, and was returning true since the saveable.autoSave was undefined istead of invoking the cb() method.
@safisa safisa changed the title Safi fix closing dirty custom text editor issue custom-editor: fix closing dirty custom text editor issue Aug 22, 2022
@vince-fugnitto vince-fugnitto added the custom-editor issues related to custom-editor functionality label Aug 22, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

@safisa
Copy link
Contributor Author

safisa commented Aug 22, 2022

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

I already signed it.
It is my first PR to Theia, do I need to do anything else?
Thanks in advance

@vince-fugnitto
Copy link
Member

I already signed it. It is my first PR to Theia, do I need to do anything else? Thanks in advance

@safisa just the ECA for the moment, I verified and there does not seem to be a valid one registered yet (for [email protected]):

Screen Shot 2022-08-22 at 8 41 08 AM

@safisa
Copy link
Contributor Author

safisa commented Aug 22, 2022

I used my private email: [email protected]
This email is also linked to my Github account, but it is not set as my primary email address!

@vince-fugnitto
Copy link
Member

I used my private email: [email protected] This email is also linked to my Github account.

@safisa in order for the ECA check to actually pass you'll need to use the same email in both your authorship and ECA.
At the moment there is a mismatch:

From 02ec66c106d900818ff3ed9e92e0422c2f4522b9 Mon Sep 17 00:00:00 2001
From: Safi <[email protected]>
Date: Mon, 22 Aug 2022 14:52:53 +0300
Subject: [PATCH 1/2] This will fix the issue: Closing a dirty/modified custom
 editor based on CustomTextEditorProvider doesn't ask the user to save, it
 just save it on close https://github.com/eclipse-theia/theia/issues/11592
...

@safisa
Copy link
Contributor Author

safisa commented Aug 24, 2022

Hi @vince-fugnitto @msujew
Can you review this PR?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes work well 👍

  • if a custom-editor is dirty, closing the editor triggers the prompt
  • save from the prompt will save the editor
  • cancel will close the dialog
  • don't save will revert the changes and close the editor

@safisa
Copy link
Contributor Author

safisa commented Sep 15, 2022

Hi @vince-fugnitto
This PR is already approved, is it going to be merged before the next Theia version?

@vince-fugnitto
Copy link
Member

@safisa it should, I just wanted to give others a chance to review.
Do you mind removing the changelog entry since it is in the wrong release? (I'll re-add it when I perform the release at the end of the month).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
custom-editor issues related to custom-editor functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants