Closed Bug 862398 Opened 7 years ago Closed 7 years ago

Maintain the order of tools in the tab and the options panel list

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

Attachments

(1 file, 2 obsolete files)

The options panel list should show the checkboxes in the correct order. Also, upon re-registering tools, they should be at the right index in the tab bar as per their ordinal.
Attached patch patch v0.1 (obsolete) — Splinter Review
Properly inserts the tool at the correct position, and shows the correct ordering of tools in the options panel.

Order of patches : 

bug 851546
bug 862363
this bug

All tests pass locally. try run at : https://tbpl.mozilla.org/?tree=Try&rev=e3b0f58d5764
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #738427 - Flags: review?(past)
Depends on: 862363
Comment on attachment 738427 [details] [diff] [review]
patch v0.1

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

::: browser/devtools/framework/Toolbox.jsm
@@ +685,4 @@
>      let defs = gDevTools.getToolDefinitionMap();
>      let tool = defs.get(toolId);
>  
> +    this._buildTabForTool(tool, reason == "options-panel");

Can't we just use toolId == "options" ?

(See next comment)

::: browser/devtools/framework/toolbox-options.js
@@ +26,5 @@
>    let onCheckboxClick = function(id) {
>      if (disabledTools.indexOf(id) > -1) {
>        disabledTools.splice(disabledTools.indexOf(id), 1);
>        Services.prefs.setCharPref(DISABLED_TOOLS, JSON.stringify(disabledTools));
> +      gDevTools.emit("tool-registered", id, "options-panel");

See bug 862363 comment 6, 7 and 8.
I think we should avoid adding lots of parameters to events where possible. The solution that we're looking at there is to pass around the tool rather than just the tool id.
(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 738427 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 738427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/Toolbox.jsm
> @@ +685,4 @@
> >      let defs = gDevTools.getToolDefinitionMap();
> >      let tool = defs.get(toolId);
> >  
> > +    this._buildTabForTool(tool, reason == "options-panel");
> 
> Can't we just use toolId == "options" ?

But... toolId can never be "options"

> (See next comment)
> 
> ::: browser/devtools/framework/toolbox-options.js
> @@ +26,5 @@
> >    let onCheckboxClick = function(id) {
> >      if (disabledTools.indexOf(id) > -1) {
> >        disabledTools.splice(disabledTools.indexOf(id), 1);
> >        Services.prefs.setCharPref(DISABLED_TOOLS, JSON.stringify(disabledTools));
> > +      gDevTools.emit("tool-registered", id, "options-panel");
> 
> See bug 862363 comment 6, 7 and 8.
> I think we should avoid adding lots of parameters to events where possible.
> The solution that we're looking at there is to pass around the tool rather
> than just the tool id.

Actually, here I can very well do without the extra parameter and do the ordering *every time*. Its just that I wanted to avoid the ordering when the toolbox opens up as ordering takes O(n) time. Thus the extra event to know whether ordering is even required or not.
Comment on attachment 738427 [details] [diff] [review]
patch v0.1

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

I don't have much more to add to Joe's feedback, and he is more experienced with this code than I am, so I'll let him have the final say on this.

While you are here, please add the standard copyright header to toolbox-options.js.

::: browser/devtools/framework/Toolbox.jsm
@@ +412,3 @@
>     */
> +  _buildTabForTool:
> +  function TBOX_buildTabForTool(toolDefinition, maintainOrder = false) {

Perhaps this boat has already sailed, but wouldn't having the options panel be a regular tool with ordinal 0, obviate the need for special-casing like this? That way any special treatment for the options panel could be localized to the options panel code and not pollute the rest of the toolbox API.

::: browser/devtools/framework/test/browser_toolbox_options.js
@@ +130,5 @@
> +    let radio = doc.getElementById("toolbox-tab-" + data);
> +    ok(radio, "Tab added back for " + data);
> +    if (radio.previousSibling) {
> +      ok(radio.getAttribute("ordinal")*1 >=
> +         radio.previousSibling.getAttribute("ordinal")*1,

Nit: a more widely-used idiom for converting a string to a number is using the + unary operator:

+radio.getAttribute("ordinal")
Attachment #738427 - Flags: review?(past) → review?(jwalker)
(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 738427 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 738427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have much more to add to Joe's feedback, and he is more experienced
> with this code than I am, so I'll let him have the final say on this.
> 
> While you are here, please add the standard copyright header to
> toolbox-options.js.

Noted.

> ::: browser/devtools/framework/Toolbox.jsm
> @@ +412,3 @@
> >     */
> > +  _buildTabForTool:
> > +  function TBOX_buildTabForTool(toolDefinition, maintainOrder = false) {
> 
> Perhaps this boat has already sailed, but wouldn't having the options panel
> be a regular tool with ordinal 0, obviate the need for special-casing like
> this? That way any special treatment for the options panel could be
> localized to the options panel code and not pollute the rest of the toolbox
> API.

Right now, all the options panel special casing (+1 to the decks index) is in Toolbox.jsm. If Options Panel is a regular tool at 0th ordinal, then special casing will be in :

1) gDevTools.jsm - to avoid addition of menu items, shortcut keys etc.
2) toolbox-options.js - to avoid removal of options panel as a tool.
3) Toolbox.jsm - to avoid addition of a full fledged tab with a label in the tabs bar.

I am okay with the change if you and Joe agree on it.

> ::: browser/devtools/framework/test/browser_toolbox_options.js
> @@ +130,5 @@
> > +    let radio = doc.getElementById("toolbox-tab-" + data);
> > +    ok(radio, "Tab added back for " + data);
> > +    if (radio.previousSibling) {
> > +      ok(radio.getAttribute("ordinal")*1 >=
> > +         radio.previousSibling.getAttribute("ordinal")*1,
> 
> Nit: a more widely-used idiom for converting a string to a number is using
> the + unary operator:
> 
> +radio.getAttribute("ordinal")

TIL.
Comment on attachment 738427 [details] [diff] [review]
patch v0.1

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

I have to say, I think Panos is right about the ordinal number thing. It feels like the hacks are in the right places that way. Is it lots of work?
Attachment #738427 - Flags: review?(jwalker)
(In reply to Joe Walker [:jwalker] from comment #6)
> Comment on attachment 738427 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 738427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> [..] Is it lots of work?

I think no, but maybe yes. Should we track that in a new bug and keep this one for ordering things ?

Since "tool-unregistered" got rid of extra parameter in bug 862363, I am going to remove the extra parameter here also.
Attached patch patch v0.2 (obsolete) — Splinter Review
removed the extra pref and added MPL license header to toolbox-options.js
Attachment #738427 - Attachment is obsolete: true
Attachment #739719 - Flags: review?(jwalker)
Duplicate of this bug: 864299
Blocks: 866138
Comment on attachment 739719 [details] [diff] [review]
patch v0.2

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

(In reply to Girish Sharma [:Optimizer] from comment #7)
> (In reply to Joe Walker [:jwalker] from comment #6)
> > Comment on attachment 738427 [details] [diff] [review]
> > patch v0.1
> > 
> > Review of attachment 738427 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > [..] Is it lots of work?
> 
> I think no, but maybe yes. Should we track that in a new bug and keep this
> one for ordering things ?

I'm happy with doing it either way, but if it's not lots of work, then I think we should probably do it in this bug.

It's up to you though.

Thanks.
Attachment #739719 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #10)
> I'm happy with doing it either way, but if it's not lots of work, then I
> think we should probably do it in this bug.
> 
> It's up to you though.
> 
> Thanks.

Already opened bug 866138 for that and one more hack removal. It will be a big patch as it touches more than 4 files. Not sure yet. Lets see.
Attached patch patch v0.3Splinter Review
Rebased on top of latest fx-team
Attachment #739719 - Attachment is obsolete: true
Attachment #743706 - Flags: review+
Whiteboard: [land-in-fx-team]
This should have a unittest to verify the ordering is correct.
Whiteboard: [land-in-fx-team] → [needs-tests]
oops. Missed the test, sorry!
Whiteboard: [needs-tests] → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/aa7ee2a8076f
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/aa7ee2a8076f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.