1013544 - Highlight selected node in graph in web audio editor

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: example

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

https://tbpl.mozilla.org/?tree=Try&rev=656f08748913

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1013544-highlight-active-node-wae.patch

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8427237 [details] [diff] [review]
1013544-highlight-active-node-wae.patch

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

There's some rejects I had to fix before playing with the patch, but this looks good.

Also, tangentially, is there a bug filed to remove the (connX.audionodeYZ) text from the sidebar? This is a protocol implementation detail that users shouldn't know about. Maybe remove that bar altogether, because now we know which node is selected.

::: browser/devtools/webaudioeditor/test/browser_wa_graph-selected.js
@@ +45,5 @@
> +    "Non-selected nodes do not have class 'selected'.");
> +  ok(findGraphNode(panelWin, gainId).classList.contains("selected"),
> +    "Newly selected node now has class 'selected'.");
> +
> +  yield teardown(panel);

Does this test make sure that clicking outside a node deselects everything? Is that tested?

::: browser/devtools/webaudioeditor/webaudioeditor-view.js
@@ +111,4 @@
>     */
>    focusNode: function (actorID) {
>      // Remove class "selected" from all nodes
>      Array.prototype.forEach.call($$(".nodes > g"), $node => $node.classList.remove("selected"));

Sidenote, but I think Array.forEach(...) also works and is more compact.

@@ -112,5 @@
> -
> -  /**
> -   * Unfocuses the corresponding graph node, called from WebAudioParamView
> -   */
> -  blurNode: function (actorID) {

I liked this, but I guess there's no need for it now?

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #3)
> Comment on attachment 8427237 [details] [diff] [review]
> 
> Does this test make sure that clicking outside a node deselects everything?
> Is that tested?
> 

Actually, this doesn't seem to be implemented yet. Do you think it'd be a good idea to have it?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

`blurNode` is no longer needed -- will probably have to be readded for any kind of global 'deselecting', which I'm still not sure how that should work -- debated having that select the overall 'context' view, but still unclear how/what should be shown, so holding off on this for now. Also, need to take into consideration navigation in the graph, sliding/dragging around shouldn't necessarily deselect the node. Created bug 1016036 to figure that out later how that should work.

Created bug 1016037 for removing actor ID display, and possible other unique identifying solutions

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1013544-highlight-active-node-wae.patch

Changed to use static method Array.forEach

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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