1064282 - Scratchpad should have statusbar like Page Source viewer

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Adrian Aichner [:anaran]]

It should be as easy as making a selection or click in the scratchpad text to see what location this is, just like in Page Source viewer.

E.g. It's a good way to watch out for excessively long lines when writing code.

Also handy to quickly see line count on Select All.

I have a prototype patch close to getting ready.

Comment 1

[Adrian Aichner [:anaran]]

Attachment: Bug1064282-20140908T192555+0200.patch

Here is a prototype patch, taking inspiration from gViewSourceUtils.viewSource.
I read in MDN statusbar is deprecated[1], but it can't be that bad as long as viewSource uses it.

I am sure my setup of event listeners is not right and would appreciate review on that.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/statusbar

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8485843 [details] [diff] [review]
Bug1064282-20140908T192555+0200.patch

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +1713,5 @@
>      }
>      this.editor.off("change", this._onChanged);
> +    window.removeEventListener("select", Scratchpad.updateStatusBar.bind(Scratchpad), false);
> +    window.removeEventListener("click", Scratchpad.updateStatusBar.bind(Scratchpad), false);
> +    window.removeEventListener("keydown", Scratchpad.updateStatusBar.bind(Scratchpad), false);

This won't remove the listeners you added earlier, as calling bind() multiple times with the same arguments produces different functions:

  > function f() {}
  undefined
  > f.bind(null) === f.bind(null)
  false

You need to bind and save the functions on Scratchpad initialization and then just reference those saved bound functions when attaching and detaching the listeners.

Aside: please use 8 lines of context in your patches, it makes it a lot easier to see what's going on. Add this to your hgrc:

  [diff]
  unified = 8

::: browser/devtools/scratchpad/scratchpad.xul
@@ +406,5 @@
>  </notificationbox>
>  
> +  <statusbar id="status-bar" class="chromeclass-status">
> +    <statusbarpanel id="statusbar-line-col" label="" flex="1"/>
> +  </statusbar>

Can we just use an hbox or something, since statusbar is deprecated? View Source shouldn't generally be cribbed from, as it has some serious legacy cruft ;)

Comment 3

[Adrian Aichner [:anaran]]

Attachment: 0002-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch

Thanks for the information about bind.
I finally succeeded at tying in to CodeMirror cursorActivity instead.
This new patch is in HG format and also has 8 lines of context.
I have
this.editor.off(this.editor, "cursorActivity", this.updateStatusBar);
currently commented out since I plan to remove it if you agree.

It took me the most time to find a statusbarpanel replacement.
I followed the MDN XUL advice to use toolbar instead and it works aside from some styling polish one could add.

I haven't really seen any good "statusbar" type examples to steal from.
Are there any?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8486811 [details] [diff] [review]
0002-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch

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

We need at least a simple test for this, where we move the cursor around and the status bar updates correctly.

Regarding binding, adding, and removing the listener, we need to:

1. Save the bound method when the scratchpad is initialized, ie: `this.updateStatusBar = this.updateStatusBar.bind(this);`.

2. Add the the bound method as a listener.

3. On unload, remove the bound listener.

Sorry if I wasn't clear enough before. If we skip 3, we risk accidentally holding references to various structures/objects that prevents those things from being collected. This is not a theoretical issue, and has bitten us before. Unfortunately, the Scratchpad isn't very disciplined and rigid about removing listeners, but that's no excuse to be lax in this patch :) I think 1 and 2 should happen in Scratchpad.onLoad, and 3 needs to happen on Scratchpad.onUnload.

Thanks :)

::: browser/devtools/scratchpad/scratchpad.js
@@ +1556,5 @@
> +  {
> +    var statusBarField = document.getElementById("statusbar-line-col");
> +    let { line, ch } = this.editor.getCursor();
> +    statusBarField.textContent = this.strings.formatStringFromName(
> +      "scratchpad.statusBarLineCol", [ line + 1, ch + 1], 2);

Lines are usually 1-based and columns are usually 0-based, in my experience. At least this is how emacs and source maps work, by default. Unless you have compelling reasons not to, I think we should follow this convention.

@@ +1706,5 @@
>        editorElement.removeEventListener("drop", this._onPaste);
>        this._onPaste = null;
>      }
>      this.editor.off("change", this._onChanged);
> +    // this.editor.off(this.editor, "cursorActivity", this.updateStatusBar);

This would attempt to remove the unbound method as a listener, but the unbound method was never added as a listener. The bound version was. This would not result in the bound listener being removed, which leads to potentially holding references to things and keeping them from being collected.

Comment 5

[Adrian Aichner [:anaran]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Comment on attachment 8486811 [details] [diff] [review]
> 0002-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch
> 
> Review of attachment 8486811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need at least a simple test for this, where we move the cursor around and
> the status bar updates correctly.

I will add the coverage, now this seems to be going somewhere.

> 
> Regarding binding, adding, and removing the listener, we need to:
> 
> 1. Save the bound method when the scratchpad is initialized, ie:
> `this.updateStatusBar = this.updateStatusBar.bind(this);`.
> 
> 2. Add the the bound method as a listener.
> 
> 3. On unload, remove the bound listener.
> 
> Sorry if I wasn't clear enough before. If we skip 3, we risk accidentally
> holding references to various structures/objects that prevents those things
> from being collected. This is not a theoretical issue, and has bitten us
> before. Unfortunately, the Scratchpad isn't very disciplined and rigid about
> removing listeners, but that's no excuse to be lax in this patch :) I think
> 1 and 2 should happen in Scratchpad.onLoad, and 3 needs to happen on
> Scratchpad.onUnload.

You were clear enough, I was just misguided by the existing code.

I agree to the lack of excuses.

> 
> Thanks :)
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +1556,5 @@
> > +  {
> > +    var statusBarField = document.getElementById("statusbar-line-col");
> > +    let { line, ch } = this.editor.getCursor();
> > +    statusBarField.textContent = this.strings.formatStringFromName(
> > +      "scratchpad.statusBarLineCol", [ line + 1, ch + 1], 2);
> 
> Lines are usually 1-based and columns are usually 0-based, in my experience.
> At least this is how emacs and source maps work, by default. Unless you have
> compelling reasons not to, I think we should follow this convention.

Actually I do: Scratchpad "Jump to line..." is one-based for line and column (character), also parses syntax error in active selection:
----SCRATCHPAD SAMPLE START----
(function we() {
  try {
to
  } catch (e) {
    // console.log(e, null, 2);
    return JSON.stringify(e, Object.getOwnPropertyNames(e), 2);
  }
})();

/*
{
  "fileName": "Scratchpad/2",
  "lineNumber": 3,
  "columnNumber": 0,
  "stack": "we@Scratchpad/2:3:1\n@Scratchpad/2:1:11\n",
  "message": "to is not defined"
}
*/
bad

/*
Exception: bad is not defined
@Scratchpad/2:19:1
*/
----SCRATCHPAD SAMPLE END----
> 
> @@ +1706,5 @@
> >        editorElement.removeEventListener("drop", this._onPaste);
> >        this._onPaste = null;
> >      }
> >      this.editor.off("change", this._onChanged);
> > +    // this.editor.off(this.editor, "cursorActivity", this.updateStatusBar);
> 
> This would attempt to remove the unbound method as a listener, but the
> unbound method was never added as a listener. The bound version was. This
> would not result in the bound listener being removed, which leads to
> potentially holding references to things and keeping them from being
> collected.

Yep, I'll fix that.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok.

Comment 7

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch

This should address your advice.

Testing coverage is minimal, but it covers the localization of the statusbar.

Have not found a good remedy for
XUL box for toolbar element contained an inline #text child
since there are no good (undeprecated) elements to choose from, so I left this as is.

Relevant test results (use CTRL+F statusbar):

725 INFO Console message: [JavaScript Warning: "XUL box for toolbar element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://browser/content/devtools/scratchpad.xul" line: 0}]
726 INFO Console message: [JavaScript Warning: "XUL box for toolbar element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://browser/content/devtools/codemirror/codemirror.js" line: 7281}]
727 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js | jumpToLine input field is set from editor selection 
728 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js | jumpToLine goto error location (line) 
729 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js | jumpToLine goto error location (column) 
730 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js | statusbar text is correct (Line 3, Col 1)

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8488041 [details] [diff] [review]
0001-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +1709,5 @@
>        editorElement.removeEventListener("drop", this._onPaste);
>        this._onPaste = null;
>      }
>      this.editor.off("change", this._onChanged);
> +    this.editor.off(this.editor, "cursorActivity", this.updateStatusBar);

I don't think you need this first parameter here.

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +51,5 @@
> +    var statusBarField = sp.editor.container.ownerDocument.querySelector("#statusbar-line-col");
> +    // var oSerializer = new XMLSerializer();
> +    // var sXML = oSerializer.serializeToString(sp.editor.container.ownerDocument);
> +    // info(sXML);
> +    // info(JSON.stringify(sp.editor, Object.getOwnPropertyNames(sp.editor), 2));

Don't forget to remove this debugging stuff.

Comment 9

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1064282-Scratchpad-should-have-statusbar-like-Pa.patch

removed debug code from tests.
removed unwanted first argument from cm.off.

Nick, am I doing this right to attach this new version, obsolete the old one, and ask for your review again?

Are we done with this now?

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Once a patch gets r+ it doesn't need review again unless there are significant changes from the last version (eg anything beyond the little comments made when the patch was given r+).

Yes, you obsolete the old patches.

Now we need a try push, to run the relevant tests on all of our platforms. I can push this one for you, but that's not something I want to do everytime, so you should request commit access level 1 so you can do it yourself in the future: https://www.mozilla.org/hacking/committer/ I will vouch for you.

Once the try push comes back green, we will add the checkin-needed keyword and a sheriff will come and push the patch to fx-team, where it will incubate before being merged into mozilla-central and gets into the next Nightly.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fe2f3cb19382

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/797f355e77fb