Closed Bug 1073935 Opened 5 years ago Closed 5 years ago

Prompt user when switching projects/creating new app if there are unsaved changes

Categories

(DevTools :: WebIDE, defect)

x86_64
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: aryx, Assigned: shivanker.goel, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 4 obsolete files)

Firefox Nightly and Aurora 20140927 on Windows 8.1

Having an app open in the editor and change some files but not saved these changes should warn the user that the changes will be lost if a different app gets loaded or a new app created. Closing the window warns.
Mentor: jryans
Whiteboard: [good first bug][lang=js]
Hi I am interested in this bug, please could I have details on this, where is the problem? how to reproduce and the code location?

This is my first bug.

Thanks
Reproduced using Aurora and Nightly 2014-10-08 under all platforms (https://bugzilla.mozilla.org/show_bug.cgi?id=1039484#c22).
OS: Windows 8.1 → All
Jesal, please take a look at our hacking guide[1] to get started with the code base.  Once you have created a your first patch, attach it here and set feedback? (if you have running code, but aren't quite sure about it) or review? (if you really think everything is complete) to me.

This bug is about switching projects in WebIDE[2].  We already check for file modifications when closing all of WebIDE[3].  We should do something similar when changing projects away from the current project.

If you have questions in meantime, set the need info box on this bug to me to make sure I don't miss your questions.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://developer.mozilla.org/en-US/docs/Tools/WebIDE
[3]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#96-101
Hi Jesal, the problem can be reproduced like this:

0. Get the Nightly version of Firefox and install it: http://nightly.mozilla.org/
1. Open the WebIDE (either from the menu at the end of the navigation bar [might require customization before] or by pressing the Alt key and selecting Tools > Web developer > WebIDE.
2. In the toolbar, select the first button and create a new app "app1", select the Hello World app, create a new folder for storing the app.
3. Create a new app "app2" like in 2.
4. Select the manifest.webapp file.
5. Change the description value from "A Hello World app" to "A Hello World app helps debugging".
6. Use the first button to switch to app1.
7. Switch back to app2.
Actual result: Change from 5. is gone.
Expected result: Firefox should warn if there are unsaved changes when switching the app (6.) like when you try to close the WebIDE window.

The 'Quit' version works like this:
1. Quit command gets registered at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#896
2. When the Quit command gets called, http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#96 get executed
3. This calls http://mxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/projecteditor.js#752

The project switching itself happens at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#989
Hi, I would like to work on this bug. I already built firefox on my computer.
great thanks guys I will start working on it
Thanks Guys, I will have a look at the problem and read through the guides.
Hi Guys.
Just wondering whats the difference between the fx team build and the Mozilla central build, I understand some things are not on the Mozilla-central build? I have both anyways.
Also the webide.js file you have linked, that's the code that needs looking at right?
Do I need to make the patch using code from that file?
How can I view changes that I make to the code in action?
If I get the code right, is it a case you uploading it here for checking?

Thanks, sorry if the questions seem stupid, I am new to this environment :)

Jesal
mozilla-central is the development repository in which eventually all changes land. Changes to Firefox functionality (not web accessible stuff like DOM, JavaScript APIs) like the UI land in fx-team and get later merged to mozilla-central so the new code gets tested on fx-team before merging to mozilla-central and developers using fx-team for pulling code haven't to wait until the latest change land in mozilla-central.

The fix for this bug is basically checking for files with unsaved changes in the currently opened app and prompting the user if they should be discarded or saved or the app switching be canceled.

To see the changes in action, you have to save the changes and build Firefox. Only the first build should take some time, the following ones should be fast unless you pull updates.

When you think your changes work as expected, you create a patch, attach it to the bug and set the 'review' field to '?' and put ':jryans' into it and select the autocomplete suggestion. More about the creation of patches: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F
Hi! I'm interested to fix one bug, but I'm new no bugzilla. I want to work in this bug and try to fix it until 30th october. Could someone mentor me?
OK, is anyone currently working on this bug? I see a lot of enthusiasm...
IMHO, the confirmation dialog should also appear when the user clicks on "Remove Project", but I was not able to figure out how to do that. I tried calling UI.canWindowClose() in the method removeProject[1], but that only triggers the confirmation dialog when a new project is opened after removing the current one, and by then, the user has no way to get back his unsaved changes.


[1]: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.js#1282
Comment on attachment 8543535 [details] [diff] [review]
Called UI.canWindowClose() in newApp, import{Packaged,Hosted}App & click event listeners in showProjectPanel

Review of attachment 8543535 [details] [diff] [review]:
-----------------------------------------------------------------

You've got the right idea here: we want to check if it's safe to close the current project whenever the project might change.

However, I'd like to see that happen in only one place, instead of each place where the project can change.

Here's a sketch of the approach I'd like to see:

1. In app-manager.js, send a new |update| in the selectedProject setter before actually changing the project, and include an object webide.js can use to abort.  Something like:
  this.update("beforeproject", { cancel: () => cancelled = true; });
2. The |update| call is synchronous, so later code in the selectedProject setter can check if cancelled is true before proceeding.
3. webide.js would listen for "beforeproject", check for unsaved files, and call |cancel| if user wants to cancel
4. Rename |canWindowClose| to |canCloseProject|

By doing this in the selectedProject setter, it should also cover the remove project case you mentioned.  Let me know if more info is needed!
Attachment #8543535 - Flags: review?(jryans)
Attachment #8543535 - Flags: feedback?(jryans)
Assignee: nobody → shivanker.goel
Status: NEW → ASSIGNED
Attached patch confirmSwitch.patch (obsolete) — Splinter Review
Sorry for the delay. This is my first bug. :)
Attachment #8543535 - Attachment is obsolete: true
Attachment #8545907 - Flags: review?(jryans)
Comment on attachment 8545907 [details] [diff] [review]
confirmSwitch.patch

Review of attachment 8545907 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this works quite nicely!  I made a few small comments below, but this is quite close.

We should also update the message shown when there are unsaved changes.  Right now, it says "You have unsaved changes that will be lost if you exit.", but we are no longer always "exiting", so let's just drop the "if you exit" part.  You'll also need to update the string's ID (needed by translation tools if the meaning has changed).

It would be nice to also add a test for this behavior, if you'd like to try that, but I won't require it.

::: browser/devtools/webide/content/webide.js
@@ +189,5 @@
>          break;
>        case "runtime-apps-found":
>          this.autoSelectProject();
>          break;
> +      case "checkunsaved":

Let's call this "before-project", meaning "before the project changes".  Most of the existing events are named based on "what is happening" not "what do we want webide to do".

Also, move this case up so it's above the "project" one.

@@ +190,5 @@
>        case "runtime-apps-found":
>          this.autoSelectProject();
>          break;
> +      case "checkunsaved":
> +        if(!UI.canCloseProject())  {

Nit: Space after "if"

You are already inside |UI|, so you can use |this| instead.

::: browser/devtools/webide/modules/app-manager.js
@@ +279,3 @@
>  
> +      let cancelled = false;
> +      this.update("checkunsaved", { cancel: () => { cancelled = true; } });

Use "before-project" as mentioned in webide.js.

@@ +282,2 @@
>  
> +      if(!cancelled)  {

Nit: Space after if

Also, you should invert this and return if |cancelled| is true, so you don't need to indent the rest of the code.
Attachment #8545907 - Flags: review?(jryans) → feedback+
Attached patch confirmSwitch.patch (obsolete) — Splinter Review
Thanks for the feedback!
PFA the updated patch.
Attachment #8545907 - Attachment is obsolete: true
Attachment #8546715 - Flags: review?(jryans)
Comment on attachment 8546715 [details] [diff] [review]
confirmSwitch.patch

Review of attachment 8546715 [details] [diff] [review]:
-----------------------------------------------------------------

I noticed an important thing I missed last time: we aren't actually blocking the remove project step if you choose "cancel".

You should update |AppManager.removeSelectedProject| to check whether |selectedProject| is still null after it sets it (and also add a comment about how it can be cancelled), and return before removing if it is not null.

Sorry for missing this before!

With that change, this should be ready to go!  Once you attach your next patch, I'll send it through our Try test infrastructure.

::: browser/devtools/projecteditor/lib/projecteditor.js
@@ +753,5 @@
>    confirmUnsaved: function() {
>  
>      if (this.hasUnsavedResources) {
>        return confirm(
> +        getLocalizedString("projecteditor.confirmUnsavedTitle2"),

This line can be reverted since we don't need the ID change.

::: browser/locales/en-US/chrome/browser/devtools/projecteditor.properties
@@ +15,3 @@
>  # This string is displayed as as the title of the confirm prompt that checks
> +# to make sure if the project can be closed/switched without saving changes
> +projecteditor.confirmUnsavedTitle2=Unsaved Changes

This string's ID does not need to change, since the text is unchanged.  It's okay to update the comment and leave the ID alone.
Attachment #8546715 - Flags: review?(jryans)
Attached patch confirmSwitch.patch (obsolete) — Splinter Review
Done!
Thanks!
Attachment #8546715 - Attachment is obsolete: true
Attachment #8546799 - Flags: review?(jryans)
Comment on attachment 8546799 [details] [diff] [review]
confirmSwitch.patch

Review of attachment 8546799 [details] [diff] [review]:
-----------------------------------------------------------------

Very close, but please fix the style nits.

::: browser/devtools/webide/modules/app-manager.js
@@ +310,5 @@
>    removeSelectedProject: function() {
>      let location = this.selectedProject.location;
>      AppManager.selectedProject = null;
> +    // If the user cancels the removeProject operation, don't remove the project
> +    if (AppManager.selectedProject == null)

Nit: Invert the condition and return.

Also, our code style requires braces for one line blocks[1].

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
Attachment #8546799 - Flags: review?(jryans) → review+
Fixed nits.
Attachment #8546799 - Attachment is obsolete: true
Attachment #8546988 - Flags: review?(jryans)
Comment on attachment 8546988 [details] [diff] [review]
confirmSwitch.patch

Great, this look good to me!

I've pushed this to try, so we'll wait for the results first.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3142bac3cb67
Attachment #8546988 - Flags: review?(jryans) → review+
Okay, looks good, marking to be checked in.

Thanks for working on this!  If you're interested, take a look at some of our other bugs! :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7d9a5ab12653
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Thanks jryans! I'll have a look at other bugs. :)
https://hg.mozilla.org/mozilla-central/rev/7d9a5ab12653
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.