Closed Bug 1055217 Opened 5 years ago Closed 5 years ago

Add front end for disabling/bypassing audio nodes

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 3 obsolete files)

The front end component of bug 1055211 -- will need a style applied to the node to indicate it's bypassed, as well as a way to toggle this per node, either as a right click/context menu on the node (how will people discover this?) or as a stand alone button on the node, or the inspector for the node.
Assignee: nobody → jsantell
Possibly the first feature containing individual node "controls", which should be done via hover over a node in the graph, displaying the name, as well as a bypass button, similar to the [ev] icon targets in inspector showing event handlers.
Depends on: 1055211
No longer blocks: 1055215
Attached patch 1055217-bypass-audio-nodes.patch (obsolete) — Splinter Review
This is a working patch for adding UI to bypassing audio nodes.

Concerns are some visual ones -- by default, nodes are "on" (power icon), so should we have an icon that denotes the opposite of this? How can we display in the graph which nodes are bypassed? The demo has these painted with a lower opacity, but very hard to see, and harder to spot if something is bypassed.

http://i.imgur.com/jTk6WBF.gif
Attached patch 1055217-bypass-audio-nodes.patch (obsolete) — Splinter Review
Finally got this completed (mostly). Everything is in here, working, tests. Only outstanding thing is tweaking with styling, but sending it over for review now.

Thoughts on what a bypassed node should look like? Opacity doesn't help much as there's not enough noticible contrast, and if we show 'dead nodes', that may be more appropriate for that.
Attachment #8532189 - Attachment is obsolete: true
Attachment #8550064 - Flags: review?(vporof)
another 50K patch jordan
Comment on attachment 8550064 [details] [diff] [review]
1055217-bypass-audio-nodes.patch

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

::: browser/devtools/webaudioeditor/controller.js
@@ +61,5 @@
>      // with CSS
>      gDevTools.on("pref-changed", this._onThemeChange);
> +
> +    // Store the AudioNode definitions from the WebAudioFront
> +    DEFINITION = yield gFront.getDefinition();

Maybe rename this to AUDIO_NODE_DEFINITION?

::: browser/devtools/webaudioeditor/test/head.js
@@ +319,5 @@
>  }
>  
> +function command (button) {
> +  let ev = button.ownerDocument.createEvent("XULCommandEvent");
> +  ev.initCommandEvent("command", true, true, button.ownerDocument.defaultView, 0, false, false, false, false, null);

Ugh.
Attachment #8550064 - Flags: review?(vporof) → review+
Status: NEW → ASSIGNED
Attached patch 1055217-bypass-audio-nodes.patch (obsolete) — Splinter Review
Fixed nit -- added all the styling
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b8f907c9448
Attachment #8550064 - Attachment is obsolete: true
Attachment #8550633 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/a242901a5347
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a242901a5347
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.