Last Comment Bug 669612 - In scratchpad, add "*" postfix to window title when editor contains unsaved changes
: In scratchpad, add "*" postfix to window title when editor contains unsaved c...
Status: RESOLVED FIXED
[scratchpad][good-first-bugs][fixed-i...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Heather Arthur [:harth]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-06 06:24 PDT by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2011-11-15 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add "*" to scratchpad window title when there are unsaved changes (8.30 KB, patch)
2011-11-04 18:27 PDT, Heather Arthur [:harth]
no flags Details | Diff | Splinter Review
Add "*" to scratchpad window title when there are unsaved changes, clean apply (8.31 KB, patch)
2011-11-04 18:32 PDT, Heather Arthur [:harth]
mihai.sucan: review-
Details | Diff | Splinter Review
patch updated to comments (8.81 KB, patch)
2011-11-08 13:12 PST, Heather Arthur [:harth]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-07-06 06:24:42 PDT
It's common practice in editors to add a "*" postfix to the window title when the editor contains unsaved changes. Scratchpad doesn't currently do this.
Comment 1 Heather Arthur [:harth] 2011-11-04 18:27:44 PDT
Created attachment 572147 [details] [diff] [review]
Add "*" to scratchpad window title when there are unsaved changes

This patch adds a "*" to the scratchpad title when the scratchpad is from a file and there are unsaved changes.

This does not cover the case where you undo back to the saved state (most editors remove the star in this case). I think that would require some deeper integration with the editor, which I think would be okay as a separate bug.
Comment 2 Heather Arthur [:harth] 2011-11-04 18:32:21 PDT
Created attachment 572149 [details] [diff] [review]
Add "*" to scratchpad window title when there are unsaved changes, clean apply

Sorry, that patch didn't apply to fx-team cleanly. Here's one that does.
Comment 3 Cedric Vivier [:cedricv] 2011-11-04 21:57:12 PDT
Shouldn't this be a prefix instead?
When the scratchpad has a long title (ie. long file name or long page title) a postfix "*" would not be visible in a task bar (or the title bar even).
Comment 4 Mihai Sucan [:msucan] 2011-11-05 04:33:41 PDT
Comment on attachment 572149 [details] [diff] [review]
Add "*" to scratchpad window title when there are unsaved changes, clean apply

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

Heather, thank you very much for your patch!

This patch is great! Some concerns:

1. the onTextChanged event listener is added only by onTextSaved which means that when the user starts Scratchpad and makes changes ... those are not marked as unsaved. They should be. Even the test expects that, but the test manually calls onTextSaved which adds the onTextChanged event listener so it doesn't fail (hehe).

2. before Scratchpad closes the user should be asked to save the changes - not much point in marking the window with a star if the user is going to lose data by mistake. This is something for a follow up bug report - please open one.

3. please open another follow up bug report about adding isDirty state tracking into the SourceEditor component itself. This is needed beyond Scratchpad.

(please mention in this bug the new bug reports, for easier tracking. thank you!)


As for Cedric's comment - it's fine either way (prepend or append the star) - I have no strong preference. The choice is yours.

Giving the patch r- for point 1 and for the problem with the import/export callback (onTextSaved doesn't check if the operation was successful).

More comments below.

Looking forward for the updated patch! Thanks!

::: browser/devtools/scratchpad/scratchpad.js
@@ +765,5 @@
> +    if (this.saved === false) {
> +      this.onTextChanged();
> +    }
> +    else if (this.saved === true) {
> +      this.onTextSaved();

(Non-review comment) just a nit: this could be if (!this.saved) { ... } else { ... }

(Why: the strict value check is not needed. You just check if .saved is falsy or not.)

@@ +837,5 @@
> +   * a scratchpad is saved, opened from file, or restored from a saved file.
> +   */
> +  onTextSaved: function SP_onTextSaved()
> +  {
> +    document.title = document.title.replace(/\*$/, "");

onTextSaved is used as a callback for importFromFile() and exportToFile(). You should check if the import/export was successful.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-11-05 08:08:49 PDT
I'm with Cedric, I'd prefer this as a prefix. I think most editors put their unsaved changes indicator before the title.
Comment 6 Heather Arthur [:harth] 2011-11-07 16:08:27 PST
(In reply to Mihai Sucan [:msucan] from comment #4)

> 1. the onTextChanged event listener is added only by onTextSaved which means
> that when the user starts Scratchpad and makes changes ... those are not
> marked as unsaved. They should be. Even the test expects that, but the test
> manually calls onTextSaved which adds the onTextChanged event listener so it
> doesn't fail (hehe).

This brings up a good point.

The most common use case for scratchpad (I think) is opening it, trying something out, and closing it without saving it. In this case we just show "Scratchpad" in the titlebar. I think it'd be distracting to show the star in this case, it would make you feel like you had to save the scratchpad. I'm open to be convinced.

Right now it will only show the star if there are unsaved changes in a scratchpad that's from a file. I'm not sure what most editors do in this case.

> 2. before Scratchpad closes the user should be asked to save the changes -
> not much point in marking the window with a star if the user is going to
> lose data by mistake. This is something for a follow up bug report - please
> open one.

This is bug 653427.

> 
> As for Cedric's comment - it's fine either way (prepend or append the star)
> - I have no strong preference. The choice is yours.

Sounds like prepend is the consensus, I'll do that.

> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +765,5 @@
> > +    if (this.saved === false) {
> > +      this.onTextChanged();
> > +    }
> > +    else if (this.saved === true) {
> > +      this.onTextSaved();
> 
> (Non-review comment) just a nit: this could be if (!this.saved) { ... } else
> { ... }
> 
> (Why: the strict value check is not needed. You just check if .saved is
> falsy or not.)

Right now I don't want to do anything if this.saved is "undefined", but that depends on the decision above.
Comment 7 Mihai Sucan [:msucan] 2011-11-08 10:12:30 PST
Hello Heather!


(In reply to Heather Arthur [:harth] from comment #6)
> (In reply to Mihai Sucan [:msucan] from comment #4)
> 
> > 1. the onTextChanged event listener is added only by onTextSaved which means
> > that when the user starts Scratchpad and makes changes ... those are not
> > marked as unsaved. They should be. Even the test expects that, but the test
> > manually calls onTextSaved which adds the onTextChanged event listener so it
> > doesn't fail (hehe).
> 
> This brings up a good point.
> 
> The most common use case for scratchpad (I think) is opening it, trying
> something out, and closing it without saving it. In this case we just show
> "Scratchpad" in the titlebar. I think it'd be distracting to show the star
> in this case, it would make you feel like you had to save the scratchpad.
> I'm open to be convinced.
> 
> Right now it will only show the star if there are unsaved changes in a
> scratchpad that's from a file. I'm not sure what most editors do in this
> case.

I can agree with the idea. Sure, but one can also start to work some cool snippet and forget to save, forget the transitory nature of Scratchpads, and lose his one-hour work of coding just because Scratchpad has no close confirmation to ask him/her to save the code.

I suggest that we should provide a safeguard for such cases. Maybe ask for confirmation if the user had the window open for more than 10 minutes? Or if the code is greater than 2KB? True that for two lines of code we shouldn't always ask for a save, because it would start to be annoying. Still, it's far more annoying to lose an hour-worth of work (or more).

Anyhow, as you pointed out, this is for bug 653427. I think, nonetheless, the star should show up when there are unsaved changes - please ping Rob for a decision on this.


> > As for Cedric's comment - it's fine either way (prepend or append the star)
> > - I have no strong preference. The choice is yours.
> 
> Sounds like prepend is the consensus, I'll do that.

Yep.

> > ::: browser/devtools/scratchpad/scratchpad.js
> > @@ +765,5 @@
> > > +    if (this.saved === false) {
> > > +      this.onTextChanged();
> > > +    }
> > > +    else if (this.saved === true) {
> > > +      this.onTextSaved();
> > 
> > (Non-review comment) just a nit: this could be if (!this.saved) { ... } else
> > { ... }
> > 
> > (Why: the strict value check is not needed. You just check if .saved is
> > falsy or not.)
> 
> Right now I don't want to do anything if this.saved is "undefined", but that
> depends on the decision above.

The initial impression is that this.saved is a boolean, but it seems to be a tri-state property: saved, unsaved, not-tracking. That's what surprised me.


Thanks for your reply! Looking forward for the updated patch.
Comment 8 Heather Arthur [:harth] 2011-11-08 13:12:32 PST
Created attachment 572978 [details] [diff] [review]
patch updated to comments

Patch updated to comments. As decided on irc, we won't show the star unless the scratchpad is from a file.

Thanks for the review Mihai, good point about the three-state property, I made it a bit clearer.
Comment 9 Mihai Sucan [:msucan] 2011-11-09 05:57:19 PST
Comment on attachment 572978 [details] [diff] [review]
patch updated to comments

Thanks for your updated patch Heather!
Comment 10 Heather Arthur [:harth] 2011-11-09 10:23:50 PST
http://hg.mozilla.org/integration/fx-team/rev/7e1f046b173b

Filed bug 700893 for aiding the "undo to a saved state" senario.

To make the story complete, we've discussed implementing an "undo closed tab" (could be part of bug 651942) feature so you don't lose work in temporary scratchpads and a "prompt for saving before closing" feature (bug 653427).
Comment 11 Cedric Vivier [:cedricv] 2011-11-10 22:57:01 PST
(In reply to Heather Arthur [:harth] from comment #10)
> http://hg.mozilla.org/integration/fx-team/rev/7e1f046b173b

It seems to consistently fail on fx-team (and I can repro locally) :
https://tbpl.mozilla.org/?tree=Try&rev=9acea551a4cf

(the failing tests might depend on another patch?)
Comment 12 Mihai Sucan [:msucan] 2011-11-11 09:35:23 PST
(In reply to Cedric Vivier [cedricv] from comment #11)
> (In reply to Heather Arthur [:harth] from comment #10)
> > http://hg.mozilla.org/integration/fx-team/rev/7e1f046b173b
> 
> It seems to consistently fail on fx-team (and I can repro locally) :
> https://tbpl.mozilla.org/?tree=Try&rev=9acea551a4cf

This not on fx-team. It's on try server with your patches pushed. :)

> (the failing tests might depend on another patch?)

Do you have a link to show failures of Heather's test without your patches? (on fx-team)

I believe you need to fix the test, as you fixed browser_scratchpad_bug_660560_tab.js. The Orion change causes the failure in Heather's test. I can also repro the failure. Please add a patch in bug 583041.

Thank you!
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-15 13:59:52 PST
https://hg.mozilla.org/mozilla-central/rev/7e1f046b173b

Note You need to log in before you can comment on or make changes to this bug.