653427 - Prompt to save file before closing Scratchpad window

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110427 Firefox/6.0a1

Accidentally closing the workspace window could lead to the loss of data -> no save dialog is displayed.

Reproducible: always

Steps to reproduce:
 1. Start Workspaces (F4)
 2. Write anything inside the window
 3. Close window 

Actual results:
 - window closes without any save dialog being prompted

Expected results:
 - a save dialog is displayed

Comment 1

[Erik Vold]

I'm not sure that a save dialog makes sense. But if there is unsaved content when closing then we could add a confirmation alert imo.

Comment 2

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #1)
> I'm not sure that a save dialog makes sense. But if there is unsaved content
> when closing then we could add a confirmation alert imo.

Agreed. I've burned myself closing workspaces with unsaved content before myself.

Comment 3

[Heather Arthur [:harth]]

Opinions wanted:

1. Should we prompt to save on restart? application quit? (They would be restored by session restore in both cases)

2. Should we prompt if the scratchpad in question isn't from a file (hasn't been saved yet)?



My opinions are:
1. No and No.
2. leaning No, with bug 360408 for unbacked scratchpads. Text editors prompt in this case, but I feel the scratchpad use case is different, since you can execute without saving. This could be annoying.

Comment 4

[Heather Arthur [:harth]]

my bad, it's bug 651942 not bug 360408 for "list of recently-open scratchpads"

Comment 5

[Joe Walker [:jwalker]]

We're trying to streamline the restart experience, I'm not sure of all the issues involved, but our failure to totally nail the issue has to be hurting us. I would hate for us to needlessly be part of the problem.

Comment 6

[Mihai Sucan [:msucan]]

I think the confirmation should only show when the Scratchpad has unsaved changes for known loaded files. If there's no file, no confirmation.

Restarts should not trigger the confirmation either.

I believe application quit should trigger the confirmation (for known files). The user might end up wondering why his file doesn't have the latest changes he did in Firefox (forgetting he did not save the changes).

Thinking of it like this: even if gedit/vim/etc would restore their sessions exactly as they were, with unsaved file changes, I would still want the file save confirmation (on quit). That's because I am editing a file - it's the file I care about - not the tool I am editing it with. Say I edit a network-shared file for some friend - I forget to save and I quit Firefox. My friend won't get the updated file and he can't get the update until I restart Firefox and save.

(in the meantime I may have broken my Firefox profile or my OS and so on.)

Just my thoughts...

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

I'm with Heather here. If the current modifications (and star) are persisted by session restore, then we shouldn't be nagging the user at all. I think the case Mihai presented is not one we should optimize for. People who use shared files are acutely aware of the fact and in my experience tend to save way too often as a safety precaution.

I also agree that asking to save a new modified scratchpad, wouldn't do justice to its name.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #7)
> I'm with Heather here. If the current modifications (and star) are persisted
> by session restore, then we shouldn't be nagging the user at all. I think
> the case Mihai presented is not one we should optimize for. People who use
> shared files are acutely aware of the fact and in my experience tend to save
> way too often as a safety precaution.

It is a matter of people's habits. Anyway, I don't have any strong feelings about this. Either way is fine. ;)


> I also agree that asking to save a new modified scratchpad, wouldn't do
> justice to its name.

As said in previous comment: I agree with this. New Scratchpads shouldn't show any confirmation.

Comment 9

[Heather Arthur [:harth]]

Okay, so consensus is definitely don't prompt whenever the scratchpad is just some temp unbacked scratchpad, and don't prompt on restart.

Mihai, I see your point about prompting on quit. Quitting is also different from restarting in that while the scratchpad is saved to the session, the default session restore pref is to show a "Restore Previous Session" button on the homepage. If you don't choose to do this (or your setting is to show a blank page), your previous scratchpad changes are lost forever.

I'm going to change my stance a bit and say that on app quit we should prompt to save scratchpads from file that have unsaved changes. Does anyone else have opinions here?

Comment 10

[Heather Arthur [:harth]]

Attachment: Prompt to save when closing a scratchpad with unsaved changes.

This will prompt to save when closing a scratchpad (backed by a file) with unsaved changes. This is only when the user closes the scratchpad themselves, not on application quit or restart. I think quit would be nice, but that could be another bug, it'd be good to get this in.

I couldn't figure out how to automate accepting the prompt, but I did write a test to check that closing a new scratchpad and closing a saved scratchpad doesn't prompt.

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 575007 [details] [diff] [review]
Prompt to save when closing a scratchpad with unsaved changes.

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

Patch looks great! Thank you for your patch Heather!

I have some general comments:

- When go to File > Close I get the prompt, but not when I close the window using the window manager (eg. when I click the close icon on the window, or when I press Alt-F4, or when I right-click on the Scratchpad window and select Close). This needs a window.onclose event handler.

Please also add a test for this case.

- You can automate accepting the confirmation. See one such example in browser/components/tabview/test/browser_tabview_bug626455.js. Or you should be able to overwrite the confirmEx() method from Services.prefs with your own that acts as if the user picked any button you want to test - you can see the browser_scratchpad_files.js test file.

Please add such testing as well (any of the two approaches is acceptable).


Looking forward for your updated patch. Thank you!

::: browser/devtools/scratchpad/scratchpad.js
@@ +877,5 @@
>      this.editor.destroy();
>      this.editor = null;
>    },
> +
> +  close: function SP_close()

Please add a jsdoc comment here.

@@ +882,5 @@
> +  {
> +    if (this.filename && !this.saved) {
> +      let ps = Services.prompt;
> +      let flags = ps.BUTTON_POS_0 * ps.BUTTON_TITLE_SAVE +
> +                  ps.BUTTON_POS_1 * ps.BUTTON_TITLE_CANCEL  +

Nit: there's a double space before the +.

@@ +891,5 @@
> +                                this.strings.GetStringFromName("confirmClose"),
> +                                flags, null, null, null, null, {});
> +      if (button == 1) {
> +        return false;  // "Cancel"
> +      }

Nit: this method seems to return a boolean value (here), but at the end of the method, there's no return value.

Comment 12

[Heather Arthur [:harth]]

Attachment: Prompt to save when closing a scratchpad with unsaved changes. v2

Aha! I didn't know that "close" event existed, thanks for the review Mihai.

Patch updated to the comments and tests for a shimmed confirmEx().

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 576205 [details] [diff] [review]
Prompt to save when closing a scratchpad with unsaved changes. v2

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

Giving the patch r+ with the comments below addressed (mostly nits).

Thanks for your update Heather!

::: browser/devtools/scratchpad/scratchpad.js
@@ +616,5 @@
> +      this.onTextSaved.bind(this);
> +      if (aCallback) {
> +        aCallback(aStatus);
> +      }
> +    });

Nit: saveFile() allows a callback, but if !this.filename, this.saveFileAs() is called, without a callback. That's not consistent. The caller of saveFile() expects (per documentation) that the callback is invoked when the file is saved (even if it's a "save as" operation).

@@ +912,5 @@
> +
> +      if (button == 1) {  // "Cancel"
> +        return false;
> +      }
> +      if (button == 0) {  // "Save"

Nit: Please use the ps.BUTTON_TITLE_* constants here.

@@ +1017,5 @@
>    return Services.strings.createBundle(SCRATCHPAD_L10N);
>  });
>  
> +// so we can override confirmEx() for testing prompt
> +Scratchpad.confirmEx = Services.prompt.confirmEx;

Nit: not exactly what I had in mind, but it works. I was more inclined of having the test do:

Services.prompt = {confirmEx: myFooFunction}

(instead of having specific code in scratchpad.js to support testing conditions)

Anyway, this works too, so it's fine. No need to change! ;)

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_653427_confirm_close.js
@@ +11,5 @@
> +var count = 0;
> +function done()
> +{
> +  if (++count == expected) {
> +    finish();

Please clear the gFile reference when the test completes.

@@ +74,5 @@
> +    win.Scratchpad.filename = "test.js";
> +    win.Scratchpad.saved = false;
> +  
> +    win.Scratchpad.confirmEx = function() {
> +      return 1; // "Cancel"

Nit: use the Services.prompt.BUTTON_TITLE_* constants.

@@ +99,5 @@
> +      let text = "new text";
> +      win.Scratchpad.setText(text);
> +
> +      win.Scratchpad.confirmEx = function() {
> +        return 0; // "Save"

Ditto.

@@ +123,5 @@
> +    win.Scratchpad.filename = gFile.path;
> +    win.Scratchpad.saved = false;
> +
> +    win.Scratchpad.confirmEx = function() {
> +      return 2; // "Don't Save"

Ditto.

Comment 14

[Heather Arthur [:harth]]

(In reply to Mihai Sucan [:msucan] from comment #13)
> 
> @@ +912,5 @@
> > +
> > +      if (button == 1) {  // "Cancel"
> > +        return false;
> > +      }
> > +      if (button == 0) {  // "Save"
> 
> Nit: Please use the ps.BUTTON_TITLE_* constants here.

Those constants don't hold the position indexes, you still have to specify the index yourself. We could define constants on the Scratchpad object though?
> 
> @@ +1017,5 @@
> >    return Services.strings.createBundle(SCRATCHPAD_L10N);
> >  });
> >  
> > +// so we can override confirmEx() for testing prompt
> > +Scratchpad.confirmEx = Services.prompt.confirmEx;
> 
> Nit: not exactly what I had in mind, but it works. I was more inclined of
> having the test do:
> 
> Services.prompt = {confirmEx: myFooFunction}

Not sure how that would affect other tests?

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Heather Arthur [:harth] from comment #14)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > 
> > @@ +912,5 @@
> > > +
> > > +      if (button == 1) {  // "Cancel"
> > > +        return false;
> > > +      }
> > > +      if (button == 0) {  // "Save"
> > 
> > Nit: Please use the ps.BUTTON_TITLE_* constants here.
> 
> Those constants don't hold the position indexes, you still have to specify
> the index yourself. We could define constants on the Scratchpad object
> though?

Yesterday I just did a quick scan through the MDN page about the prompt service. I didn't see those constants don't match properly to the button indexes. (Sorry!)

Yep, defining our own constants within Scratchpad sounds be fine.



> > @@ +1017,5 @@
> > >    return Services.strings.createBundle(SCRATCHPAD_L10N);
> > >  });
> > >  
> > > +// so we can override confirmEx() for testing prompt
> > > +Scratchpad.confirmEx = Services.prompt.confirmEx;
> > 
> > Nit: not exactly what I had in mind, but it works. I was more inclined of
> > having the test do:
> > 
> > Services.prompt = {confirmEx: myFooFunction}
> 
> Not sure how that would affect other tests?

You should be able to do oldPrompt = Services.prompt; then put it back Services.prompt = oldPrompt. Would this work for your test?


(One thing I noticed now: the patch has trailing whitespace - please remove it. Thanks!)

Comment 16

[Heather Arthur [:harth]]

Thanks again Mihai for the patient reviews and tips. fx-team:

http://hg.mozilla.org/integration/fx-team/rev/94a04fd2568d

Comment 17

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/94a04fd2568d