Last Comment Bug 684546 - The Scratchpad editor's undo/redo state is preserved across file loads
: The Scratchpad editor's undo/redo state is preserved across file loads
Status: RESOLVED FIXED
[sourceeditor][scratchpad][good first...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 9 Branch
: All All
: P3 normal with 1 vote (vote)
: Firefox 12
Assigned To: Kenny Heaton
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-03 16:38 PDT by Greg Parris
Modified: 2012-04-23 10:25 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add reset for undo stack (3.34 KB, patch)
2011-12-14 15:12 PST, Kenny Heaton
mihai.sucan: feedback+
Details | Diff | Review
Adding tests for the reset undo patch (10.01 KB, patch)
2011-12-17 14:43 PST, Kenny Heaton
no flags Details | Diff | Review
reset undo and chrome tests (13.01 KB, patch)
2011-12-17 16:17 PST, Kenny Heaton
mihai.sucan: review+
Details | Diff | Review
re-based and applied feedback to earlyer patch (14.81 KB, patch)
2011-12-18 18:43 PST, Kenny Heaton
mihai.sucan: review+
Details | Diff | Review
[in-fx-team] Applied feedback to earlyer patch (14.67 KB, patch)
2011-12-19 21:39 PST, Kenny Heaton
mihai.sucan: review+
Details | Diff | Review

Description Greg Parris 2011-09-03 16:38:12 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110903 Firefox/9.0a1
Build ID: 20110903030832

Steps to reproduce:

Pressed Ctrl+Z after opening a file in Scratchpad.


Actual results:

1. Open Scratchpad
2. Note the placeholder text
3. Open any file (File -> Open File...) in Scratchpad
4. Select Edit -> Undo
5. Note that the placeholder text is back


Expected results:

The "undo/redo" stack should have been reset upon the new file being opened.
Comment 1 Greg Parris 2011-09-03 16:42:40 PDT
It would be nice if there was a simple reset() method set up for the Orion and textarea editors.  I'm currently doing:

"aWin.Scratchpad.editor._undoStack.reset()" for Orion, and

"aWin.Scratchpad.editor._editor.resetModificationCount(); aWin.Scratchpad.editor_editor.transactionManager.clear()" for textarea

Not pretty...
Comment 2 Mihai Sucan [:msucan] 2011-10-07 13:44:09 PDT
Thank you for the bug report!

That is a good point, we need to clear the undo stack / transaction manager across file loads in Scratchpad, and the SourceEditor needs to provide a method to do that.
Comment 3 Kenny Heaton 2011-12-14 15:12:31 PST
Created attachment 581808 [details] [diff] [review]
Add reset for undo stack

Adding a resetUndo function to both the textarea and orion and calling when a file successfully loads.
Comment 4 Kenny Heaton 2011-12-14 15:16:10 PST
This bug say's it's for window 7 but it should repro on all platforms. I was able to verify on windows and linux. I'm not able to make that edit.
Comment 5 Mihai Sucan [:msucan] 2011-12-15 02:56:42 PST
Thanks for your contribution Kenny! Much appreciated!

I have assigned the bug to you now and I will review your patch as soon as possible.
Comment 6 Mihai Sucan [:msucan] 2011-12-15 08:49:59 PST
Comment on attachment 581808 [details] [diff] [review]
Add reset for undo stack

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

Kenny, this patch applied cleanly and it does exactly what it needs to do. Really great work! Surgical patch that does pretty much what I had in mind.

The next step is writing a test for the new Source Editor API and another test for Scratchpad, to check that file opens clear the undo stack.

For further information on running and writing tests please read:
https://developer.mozilla.org/en/Browser_chrome_tests

The Source Editor tests live in browser/devtools/sourceeditor/test/ and the Scratchpad tests live in browser/devtools/scratchpad/test.

Please copy in each folder one of the existing tests (the ones you see simplest / most fit for your testing purposes). Give the new tests file names that match the naming pattern of existing test files. Then edit each file as needed.

For the Source Editor test I recommend making some text changes then undo, redo, check after each step if the editor content is the expected one. Then do resetUndo() and check again. Make more changes, then check state again. In the Source Editor _initialization.js file you should find sufficient code for inspiration.

For the Scratchpad test I recommend you create two temporary files, fileA and fileB. open a Scratchpad, load fileA, make some changes, test undo/redo, check state, then open fileB, and check undo/redo states, see if the history was cleared. There's a _files.js test that has code working with files and with Scratchpad - you can use that for inspiration.

For functions and methods you don't know about, please search MDN - there's a lot of cool and helpful documentation there. ;)

Thank you very much for your contribution - much appreciated.

(PS. on irc.mozilla.com you can join #devtools and talk to us!)
Comment 7 Kenny Heaton 2011-12-17 14:43:41 PST
Created attachment 582599 [details] [diff] [review]
Adding tests for the reset undo patch

Here are the tests. Sorry I was not able to get the code and tests into one patch, I use Perforce at my work and I'm just learning Mercurial. Asked on #devtools but I guess no one was around. Let me know if there are further changes that need to be made.
Comment 8 Kenny Heaton 2011-12-17 16:17:11 PST
Created attachment 582606 [details] [diff] [review]
reset undo and chrome tests

Sorry for the noise. I was able to get some help using MQ to put all the code into one patch.
Comment 9 Mihai Sucan [:msucan] 2011-12-18 03:00:47 PST
Comment on attachment 582606 [details] [diff] [review]
reset undo and chrome tests

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

This is really good work. Thank you Kenny!

This patch is almost good to go, unfortunately you need to rebase it to the latest version of the code. Please do that - there are only minor changes to be made in the Scratchpad test.

General nit-pick: the patch has trailing whitespace - please remove it.

Thank you very much for your outright awesome contribution! Looking forward for the updated patch!

::: browser/devtools/scratchpad/test/browser_scratchpad_bug684546_reset_undo.js
@@ +31,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
> +
> +    gScratchpadWindow = Scratchpad.openScratchpad();
> +    gScratchpadWindow.addEventListener("load", runTests, false);
> +  }, true);

There have been changed in the fx-team repository that cause this test to fail now. Please update your repository to the latest code.

Look into the updated tests and you'll see that the opening of scratchpads has changed a bit.

::: browser/devtools/sourceeditor/test/browser_bug684546_reset_undo.js
@@ +32,5 @@
> +{
> +  box = testWin.document.querySelector("box");
> +
> +  editorType = SpecialPowers.getCharPref("devtools.editor.component");
> +  SpecialPowers.setCharPref("devtools.editor.component", "textarea");

Please test only the default editor component. Changing the component during runtime doesn't work. Once the Source Editor is loaded, subsequent changes of the preference have no impact - due to the way JSM loading works.
Comment 10 Kenny Heaton 2011-12-18 18:43:39 PST
Created attachment 582729 [details] [diff] [review]
re-based and applied feedback to earlyer patch

I applied all your feedback
* rebased
* removed all whitespace
* changed the way I open the scratch pad
* now only testing the default editor
* double checked I had the lastest files before creating the patch
* built and ran all devtools tests

This patch was created from hg diff on my workspace instead of a committed changeset. It seems easier if I need to make additional changes. I hope this is ok as it's missing some of the changeset description information. I've found different wiki pages giving different instructions on creating patches.
Comment 11 Mihai Sucan [:msucan] 2011-12-19 13:23:49 PST
Comment on attachment 582729 [details] [diff] [review]
re-based and applied feedback to earlyer patch

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

Patch looks really good. Thank you!

Only some nits remain, see the comments below.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug684546_reset_undo.js
@@ +27,5 @@
> +  waitForExplicitFinish();
> +
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  gBrowser.selectedBrowser.addEventListener("load", function() {
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

arguments.callee is deprecated, please name the function.

@@ +36,5 @@
> +}
> +
> +function runTests()
> +{
> +  gScratchpadWindow.removeEventListener("load", arguments.callee, false);

This line is no longer needed once you changed to the new way of opening scratchpads.

::: browser/devtools/sourceeditor/test/browser_bug684546_reset_undo.js
@@ +14,5 @@
> +  waitForExplicitFinish();
> +
> +  const windowUrl = "data:application/vnd.mozilla.xul+xml,<?xml version='1.0'?>" +
> +    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
> +    " title='test for bug 687573 - vertical scroll' width='300' height='500'>" +

Please update the window title.

@@ +37,5 @@
> +function editorLoaded()
> +{
> +  editor.setText("First");
> +  editor.setText("Second", 5);
> +  is(editor.getText(), "FirstSecond", "textarea: Text set correctly.");

The "textarea: " prefix is no longer needed in these tests.
Comment 12 Mihai Sucan [:msucan] 2011-12-19 13:31:04 PST
(In reply to Kenny Heaton from comment #10)
> Created attachment 582729 [details] [diff] [review]
> re-based and applied feedback to earlyer patch
> 
> I applied all your feedback
> * rebased
> * removed all whitespace
> * changed the way I open the scratch pad
> * now only testing the default editor
> * double checked I had the lastest files before creating the patch
> * built and ran all devtools tests
> 
> This patch was created from hg diff on my workspace instead of a committed
> changeset. It seems easier if I need to make additional changes. I hope this
> is ok as it's missing some of the changeset description information. I've
> found different wiki pages giving different instructions on creating patches.

To improve your workflow I recommend you use mercurial queues. See:

https://developer.mozilla.org/en/Mercurial_Queues
http://mercurial.selenic.com/wiki/MqExtension
http://mercurial.selenic.com/wiki/MqTutorial

General Mercurial info:
https://developer.mozilla.org/en/Mercurial_FAQ

Good luck and thanks for your well-executed contribution!
Comment 13 Kenny Heaton 2011-12-19 21:39:25 PST
Created attachment 583069 [details] [diff] [review]
[in-fx-team] Applied feedback to earlyer patch
Comment 14 Mihai Sucan [:msucan] 2011-12-20 04:10:39 PST
Comment on attachment 583069 [details] [diff] [review]
[in-fx-team] Applied feedback to earlyer patch

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

This is ready to land! Awesome work! Thank you Kenny!
Comment 15 Kenny Heaton 2011-12-20 06:25:23 PST
Thanks for all your help. Since I don't have push privileges will you be able to push this patch for me?
Comment 16 Mihai Sucan [:msucan] 2011-12-20 07:01:11 PST
(In reply to Kenny Heaton from comment #15)
> Thanks for all your help. Since I don't have push privileges will you be
> able to push this patch for me?

Yep, that's the plan. I will land your patch, hopefully today. Thanks!
Comment 17 Mihai Sucan [:msucan] 2011-12-20 10:21:51 PST
Comment on attachment 583069 [details] [diff] [review]
[in-fx-team] Applied feedback to earlyer patch

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

Kenny, thank you for this high quality patch!
Comment 18 Ed Morley [:emorley] 2011-12-21 11:56:54 PST
https://hg.mozilla.org/mozilla-central/rev/a98e349f29f7

Nice work Kenny! :-)
Comment 19 Mihai Sucan [:msucan] 2012-02-09 11:35:39 PST
The addition of resetUndo() to the Source Editor API in Firefox 12 should be documented. Thank you!
Comment 20 Eric Shepherd [:sheppy] 2012-04-23 10:25:32 PDT
resetUndo() method documented:

Listed on Firefox 12 for developers.

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