1428442 - Enable highlighting the tab of more than one tool at a time (like when the debugger is paused)

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Right now it is only possible to highlight 1 tool at a time (debugger OR network monitor), it is possible that more than one tool is currently active/has an affect on other tools so this functionality needs to be expanded to account for that.

Comment 1

[Patrick Brosset <:pbro>]

Do you mean being able to see more than 1 tool at the same time? Like the split console for instance?
If so, then this is a duplicate of bug 1121414.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Ah, I should've been more descriptive. This is just related to the indication of an active tool - e.g. the green colored tab highlighting (debugger for example).

Comment 3

[Patrick Brosset <:pbro>]

Oh I see. Thanks for clarifying.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1428442 patch

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8947821 [details] [diff] [review]
1428442 patch

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

Thanks Yura. Will forward this to Greg who has been working on this code last.

Comment 6

[Greg Tatum [:gregtatum]]

Comment on attachment 8947821 [details] [diff] [review]
1428442 patch

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

Looks like a pretty straightforward patch.

Please update the browser_toolbox_highlight.js to take into account the new behavior, and then I'll be comfortable with an r+.

Thanks!

devtools/client/framework/test/browser_toolbox_highlight.js

::: devtools/client/framework/components/toolbox-tab.js
@@ +46,5 @@
>  
>      const className = [
>        "devtools-tab",
>        currentToolId === id ? "selected" : "",
> +      highlightedTools && highlightedTools.has(id) ? "highlighted" : "",

You could make this line simpler by having highlightedTools non-optional in the PropTypes, but I'll leave it up to you with what you want to do.

::: devtools/client/framework/components/toolbox-toolbar.js
@@ +26,5 @@
>        // List of command button definitions.
>        toolboxButtons: PropTypes.array,
>        // The id of the currently selected tool, e.g. "inspector"
>        currentToolId: PropTypes.string,
> +      // An optionally highlighted tools, e.g. "inspector"

I would like to see this comment mention that this prop is a Set, since it's a bit ambiguos when declared as an PropTypes.object.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1428442 patch v2

Updated test, addressed comments, thanks!

Comment 8

[Greg Tatum [:gregtatum]]

Comment on attachment 8948515 [details] [diff] [review]
1428442 patch v2

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

Looks great, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d719873a21a9
enabling tab highlighting for more than one tool at a time. r=gregtatum

Comment 10

[Dorel Luca [:dluca]]

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