1007345 - Redesign AudioNode view

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

To handle more granular control for each audionode, and for future enhancements, like updating on property change, individual controls per node (muting, rerouting), and further interspection (list input/output nodes for current node), we need a way to make this usable.

Currently, all nodes are listed in a VariablesView, but new change makes selecting a node in the graph view open up an inspector view for just that audio node (with tabs for controls, parameters, automation, etc.,).

This is just for displaying parameters in an audio node inspector view. Other tools can come in the future.

Sketch of what the 'single node view' looks like: http://imgur.com/fD6RvSu

Comment 1

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

Attachment: 1007345-redesign-webaudioeditor-property-view.patch

Big patch.. revamping how the node inspector portion works, view only one node at a time, will provide more room for additional features per-node in a nice way, and much prettier to boot

Comment 2

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

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

Comment 3

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

Still needs explicit tests for the InspectorView (hidden by default, test margins, test that it shows empty content message etc) but wanted to get this up for review as its pretty large

Comment 4

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

And this is looking lovely: http://i.imgur.com/It5YmGR.gif

Comment 5

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

While better than what we used to have before, I'm having a few concers with this redesign:
* The top toolbar is taking a lot of space for nothing (or, more accurately, for just one button)
* The huge blue "Parameters" tab doesn't really offer any additional information and it's just wasted space
* Nodes with no params should probably display "No parameters for this node" or something. Currently it feels like a bug when there's just an expanded "AudioParams" scope with no properties.
* Nodes still look bad on the light theme. Black text on very dark backgrounds are hard to read.

Postponing review until those issues are addressed.

Comment 6

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

* The top toolbar will also have a config gear button
* Huge blue "parameters" tab is, I agree, a bit much, but this is where I'd like to add additional tabs: Context (bug 1007321), Connections, Controls (muting [bug 1007776], start/stop [bug 1006625], disabling [bug 1007778]), and an Automation tab for scheduled automation. That being said, this tab should remain to make room for the future features. The main point of this patch is to make the editor scalable, and shooting to have some of these things added by FF32. like: http://i.imgur.com/fD6RvSu.png
* Agreed on nodes with no audio params.
* we have a prettification bug for light theme that you filed bug 1006875, trying to keep this patch focused on just the structural redesign, rather than the aesthetics

Comment 7

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

Attachment: 1007345-redesign-webaudioeditor-property-view.patch

try https://tbpl.mozilla.org/?tree=Try&rev=b95e485c5333

Added tests for the inspector view, and shows 'no audio params' when clicking on a node without audio params, like the destination for example

Comment 8

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

Failing on OSX 10.8, wxp, w7, w8, all non-debug builds:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_params_view_edit.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_params_view_edit.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/webaudioeditor/test/doc_simple-context.html
Return code: 1

Comment 9

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

Nevermind, wasn't the latest tbpl run. (https://tbpl.mozilla.org/?tree=Try&rev=b95e485c5333)

Comment 10

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

New patch to fix failures on try:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_inspector.js | InspectorView shown once node selected.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_params-view-edit.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_params-view-edit.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/webaudioeditor/test/doc_simple-context.html
Return code: 1 

new try: https://tbpl.mozilla.org/?tree=Try&rev=ed533e31de04

Comment 11

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

Attachment: 1007345-redesign-webaudioeditor-property-view.patch

Comment 12

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

Trying to track down last weird issue with VariablesView editing on all machines on try, but not locally, try: https://tbpl.mozilla.org/?tree=Try&rev=8b01829a13bf

Comment 13

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

Comment on attachment 8419808 [details] [diff] [review]
1007345-redesign-webaudioeditor-property-view.patch

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

::: browser/devtools/webaudioeditor/test/browser_wa_graph-click.js
@@ +38,5 @@
> +  is(WebAudioInspectorView.getCurrentNode().id, nodeIds[2], "Clicking the same node again works (idempotent).");
> +
> +  click(panel.panelWin, $("rect", findGraphNode(panelWin, nodeIds[3])));
> +  is(WebAudioInspectorView.getCurrentNode().id, nodeIds[3], "Clicking on a <rect> works as expected.");
> +  

Nit: some trailing whitespace in this file.

::: browser/devtools/webaudioeditor/webaudioeditor-view.js
@@ +10,5 @@
> +// Strings for rendering
> +const EXPAND_INSPECTOR_STRING = L10N.getStr("expandInspector");
> +const COLLAPSE_INSPECTOR_STRING = L10N.getStr("collapseInspector");
> +
> +const INSPECTOR_WIDTH = 300;

Why hardcode this? Wouldn't it be better to have a pref for it?

@@ +280,5 @@
> +    this._inspectorPaneToggleButton = null;
> +    this._tabsPane = null;
> +  },
> +
> +  toggleInspector: function (flags, index) {

Would be nice to have documentation for all these functions. What are flags? Index?

@@ +291,5 @@
> +    if (flags.delayed == null)
> +      flags.delayed = true;
> +
> +    // Override callback so we can emit an event
> +    flags.callback = () => window.emit(EVENTS.UI_INSPECTOR_TOGGLED, flags.visible);

It seems to me that toggleInspector should just take a single `visible` flag, since you're overwriting everything anyway.

@@ +309,5 @@
> +      pane.selectedIndex = index;
> +    }
> +  },
> +
> +  isVisible: function () {

Nit: docs.

@@ +317,5 @@
> +  /**
> +   * Takes a AudioNodeView `node` and sets it as the current
> +   * node and scaffolds the inspector view based off of the new node.
> +   */
> +  setCurrentNode: function (node) {

This name could potentially be confusing without looking at the docs. Node as in DOM node, or audio node? How about `setCurrentAudioNode`?

@@ +325,5 @@
> +    // view.
> +    if (!node) {
> +      $("#web-audio-editor-details-pane-empty").removeAttribute("hidden");
> +      $("#web-audio-editor-tabs").setAttribute("hidden", "true");
> +      window.emit(EVENTS.UI_INSPECTOR_NODE_SET, null);

Nit: you don't really need to pass `null` if there are no extra arguments.

@@ +333,5 @@
> +      $("#web-audio-editor-details-pane-empty").setAttribute("hidden", "true");
> +      $("#web-audio-editor-tabs").removeAttribute("hidden");
> +      this._setTitle();
> +      this._buildPropertiesView()
> +        .then(() => window.emit(EVENTS.UI_INSPECTOR_NODE_SET, this._currentNode.id));

Oh, I guess you want to make it symmetric to this? Ok.

::: browser/devtools/webaudioeditor/webaudioeditor.xul
@@ +51,4 @@
>           class="devtools-responsive-container"
>           flex="1"
>           hidden="true">
> +      <vbox id="web-audio-content-pane" flex="1">

Is there really no way of removing this toolbar? It takes too much space, even with 2 buttons displayed at some point.

::: browser/locales/en-US/chrome/browser/devtools/webaudioeditor.dtd
@@ +21,5 @@
>  
>  <!-- LOCALIZATION NOTE (webAudioEditorUI.emptyNotice): This is the label shown
>    -  while the page is refreshing and the tool waits for a audio context. -->
>  <!ENTITY webAudioEditorUI.emptyNotice     "Waiting for an audio context to be created…">
> +                 

Nit: trailing whitespace.

::: browser/themes/shared/devtools/webaudioeditor.inc.css
@@ +141,5 @@
> +#inspector-pane-toggle:active {
> +  -moz-image-region: rect(0px,32px,16px,16px);
> +}
> +
> +#body[layout=vertical] #web-audio-inspector {

Where is the "layout" attribute set? Did you verify this works?

Comment 14

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

Thanks for the review, Victor. Added documentation, changed some method names, made a bug for the width being stored as a preference, removed extraneous CSS. For the toolbar, I kinda want to leave it for now, and if it proves to be too much space for not enough benefit (want to add other information here), then we can remove it, but for now, I'd like to try it out.

Still tracking that test failure that consistently fails on all OS on try, but works flawlessly on my 512 MB VMs of every operating system.

Comment 15

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

Ugly test hack, try push: https://tbpl.mozilla.org/?tree=Try&rev=52786e52324e

Comment 16

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

Try up, last go, should've fixed the issue with variableView: https://tbpl.mozilla.org/?tree=Try&rev=f33089d37dd0

Comment 17

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

Attachment: 1007345-redesign-webaudioeditor-property-view.patch

Comment 18

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

Made embarrassing VariableView errors, now fixed on try finally. Ready to check in.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/191b1232742b