942774 - Files loaded into scratchpad should be recognized as “saved”

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Patrick Westerhoff]

Currently, when loading a file into Scratchpad, the file is immediately marked as “not saved” (visible by the star prepended to the file name in the title). Thus, when closing Scratchpad without actually changing the file, I still get a prompt to save it.

This is obviously not necessary for files that were just loaded but never changed.

The behavior is triggered by `self.setText(content);` in the `SP_importFromFile` function [1]. Setting the text will raise the `textChanged` event which in turn will put the Scratchpad editor in an unsaved state.

It seems that the `textChanged` event is only being listened to when the file was saved until the first change. I would suggest solving this similarly to how it’s done in the initialization by calling `onTextSaved` after the text has been loaded from the file [3]. In addition this would require removing the listener first before starting to load the file.


[1] https://hg.mozilla.org/mozilla-central/file/94a04fd2568d/browser/devtools/scratchpad/scratchpad.js#l578
[2] https://hg.mozilla.org/mozilla-central/file/94a04fd2568d/browser/devtools/scratchpad/scratchpad.js#l882
[3] https://hg.mozilla.org/mozilla-central/file/94a04fd2568d/browser/devtools/scratchpad/scratchpad.js#l792

Comment 1

[Paul Silaghi, QA [:pauly]]

(In reply to Patrick Westerhoff from comment #0)
> This is obviously not necessary for files that were just loaded but never
> changed.
Agreed.
Confirmed on 28.0a1 (2013-12-09), Win 7 x64.

Comment 2

[Bhagya]

I would like to work on the bug. Can I be assigned to it?

Comment 3

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

sure, Bhagya. Attach a patch and we'll set the assignee to you. Thanks!

Comment 4

[Anup Kumar [:reznord]]

Can you please assign this bug to me? I am interested in working on this bug.

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug942774.patch

The code link in description are out of date.

Comment 6

[Patrick Westerhoff]

Is that really all that’s necessary now to fix this? Wow, a lot seems to have happened in that file :o

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

The |set dirty| will call |setClean| in the editor and update the title. I think that can solve this problem. Any suggestion appreciated.

[1]http://dxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#164

Comment 8

[Brandon Benvie [:benvie]]

This looks like the correct fix, but I'd also like to see a test added to confirms this behavior so it won't break in the future again. You should be able to add a check somewhere around [1] to confirm that gScratchpad.dirty is false.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/test/browser_scratchpad_files.js#45

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug942774-V2.patch

Test added. Thank you.

Comment 10

[Brandon Benvie [:benvie]]

Comment on attachment 8362344 [details] [diff] [review]
bug942774-V2.patch

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

Thanks! LGTM pending a green try.

https://tbpl.mozilla.org/?tree=Try&rev=46ff5902f513

Comment 11

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/cbb2876bdaf3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbb2876bdaf3