1057042 - Refactor web audio editor client components

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

Been awhile since the initial release, so before continuing adding new features which will add a lot of complexity to how the client is handled, a refactor would be really useful and make future-jsantell's life easier.

Comment 1

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

Attachment: 1057042-wae-refactor.patch

I'm sorry, Victor.

This patch introduces a lot of refactoring, making an audio node collection the arbiter of state of the context, exposing event-based hooks, as well as more powerful individual models for nodes to expose additional APIs that are in the works for the actors. Also decouples a lot of the magic between controller/views, as the controller translates the server data by manipulating the front end data structure (AudioNodeCollection instance), and the views based off of those events.

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

Comment 2

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

Attachment: 1057042-wae-refactor.patch

Added refactoring to the views as well, mainly allowing mixins for toggling view -- this was done for adding a visualization panel in bug 1019100

Comment 3

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

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

Comment 4

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

Comment on attachment 8491070 [details] [diff] [review]
1057042-wae-refactor.patch

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

::: browser/devtools/webaudioeditor/controller.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Track an array of audio nodes

Canihaz more details about this here? What kind of audio nodes? I'm assuming not *actual* audio nodes, but models based on the server-side data.

@@ +4,5 @@
> +
> +/**
> + * Track an array of audio nodes
> + */
> +let audioNodes = new AudioNodesCollection();

s/audioNodes/gAudioNodes/

@@ +54,5 @@
> +    // Hook into theme change so we can change
> +    // the graph's marker styling, since we can't do this
> +    // with CSS
> +    gDevTools.on("pref-changed", this._onThemeChange);
> +

Nit: extra newline here.

@@ +178,5 @@
> +   * Called when a new node is created. Creates an `AudioNodeView` instance
> +   * for tracking throughout the editor.
> +   */
> +  _onCreateNode: Task.async(function* (nodeActor) {
> +    yield audioNodes.add(nodeActor);

Any reason to yield here?

::: browser/devtools/webaudioeditor/head.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I find it super weird to have a head.js outside test/. Even if there is some code duplication, let's keep the old (but consistent) file structure. At the very least, you should rename this file, since I instinctively assumed it had something to do with tests.

::: browser/devtools/webaudioeditor/models.js
@@ +22,5 @@
> +  collection: null,
> +
> +  initialize: function (actor) {
> +    this.actor = actor;
> +    this.id = actor.actorID;

Do we need both `actor` and `id` properties? Can the id change?

@@ +26,5 @@
> +    this.id = actor.actorID;
> +    this.connections = [];
> +  },
> +
> +  setup: Task.async(function* () {

Document me.

@@ +37,5 @@
> +    this.type = yield this.actor.getType();
> +    return this.type;
> +  }),
> +
> +  connect: function (destination, param) {

Document me.

@@ +46,5 @@
> +      coreEmit(this, "connect", this, destination, param);
> +    }
> +  },
> +
> +  disconnect: function () {

Document me.

@@ +53,5 @@
> +  },
> +
> +  // Returns a promise that resolves to an array of objects containing
> +  // both a `param` name property and a `value` property.
> +  getParams: function () {

This method looks unnecessary, since `actor` is public.

@@ +72,5 @@
> +  // the graph to be rendered. Separate from `addToGraph`,
> +  // as while we depend on D3/Dagre's constraints, we cannot
> +  // add edges for nodes that have not yet been added to the graph.
> +  addEdgesToGraph: function (graph) {
> +    this.connections.forEach(edge => {

Nit: for (let edge of this.connections) ?

@@ +115,5 @@
> +  forEach: function (fn) {
> +    return this.models.forEach(fn);
> +  },
> +
> +  add: Task.async(function* (obj) {

Please document all public methods. What exactly happens when adding a node here, for example? There's a bunch of stuff going on, setups, events emited etc., so it's not just like adding something to a set.

Also, can the same node be added twice? What is the expected type of `obj`? Suppose I'm a very lazy programmer and don't like reading code :)

::: browser/devtools/webaudioeditor/views/context.js
@@ +41,5 @@
> +    this._onNodeSelect = this._onNodeSelect.bind(this);
> +    this._onStartContext = this._onStartContext.bind(this);
> +    this._onEvent = this._onEvent.bind(this);
> +
> +    this.draw = debounce(this.draw.bind(this), GRAPH_DEBOUNCE_TIMER);

Pretty.

@@ +54,5 @@
> +  /**
> +   * Destruction function, called when the tool is closed.
> +   */
> +  destroy: function() {
> +    if (this._zoomBinding) {

It is not immediately obvious when zoomBinding is null. Add a comment.

@@ +84,5 @@
> +
> +  /**
> +   * Moves the graph back to its original scale and translation.
> +   */
> +  resetGraphPosition: function () {

Nit: since this affects both scale and translation, i'd rename this method to `resetGraphTransform` or something similar.

@@ +127,5 @@
> +    return $(".nodes > g[data-id='" + actorID + "']");
> +  },
> +
> +  /**
> +   * `draw` renders the ViewNodes currently available in `audioNodes` with `AudioNodeConnections`,

s/`draw`/This method/

@@ +129,5 @@
> +
> +  /**
> +   * `draw` renders the ViewNodes currently available in `audioNodes` with `AudioNodeConnections`,
> +   * and `AudioParamConnections` and is throttled to be called at most every
> +   * `GRAPH_DEBOUNCE_TIMER` milliseconds. Is called whenever the audio context routing changes,

s/Is/It's/

@@ +138,5 @@
> +    this.clearGraph();
> +
> +    let graph = new dagreD3.Digraph();
> +    audioNodes.populateGraph(graph);
> +    /*

Shitton of commented out code in here.

Comment 5

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

Comment on attachment 8491070 [details] [diff] [review]
1057042-wae-refactor.patch

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

::: browser/devtools/webaudioeditor/controller.js
@@ +178,5 @@
> +   * Called when a new node is created. Creates an `AudioNodeView` instance
> +   * for tracking throughout the editor.
> +   */
> +  _onCreateNode: Task.async(function* (nodeActor) {
> +    yield audioNodes.add(nodeActor);

AudioNodeCollection#add returns a promise, as it instantiates an AudioNodeModel wrapper, and sets it up, caching values used elsewhere within the model

::: browser/devtools/webaudioeditor/models.js
@@ +22,5 @@
> +  collection: null,
> +
> +  initialize: function (actor) {
> +    this.actor = actor;
> +    this.id = actor.actorID;

The ID cannot change, however, I don't want to expose the actor directly (can hide it in a WeakMap or something if necessary) to the editor, incase something on the actor API changes, the front end does not have to change, and uses the model wrapper also to prevent that. Also the reason for `getParams` method on the model itself as well

::: browser/devtools/webaudioeditor/views/context.js
@@ +84,5 @@
> +
> +  /**
> +   * Moves the graph back to its original scale and translation.
> +   */
> +  resetGraphPosition: function () {

+1

Comment 6

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

Attachment: 1057042-wae-refactor.patch

Lots of comments added, some renames, some commented reasons on the changes. Resubmitting to try: https://tbpl.mozilla.org/?tree=Try&rev=6f8cb8812209

Comment 7

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

build failures on last try: https://tbpl.mozilla.org/?tree=Try&rev=c04c99724c31

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48566678&tree=Fx-Team

Comment 10

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

Looks like a rebase issue, checking it out

Comment 11

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

Ah, failure from tests from bug 1066450, looks like this[1] landed between try and checkin 

[1] http://comments.gmane.org/gmane.comp.mozilla.devel.platform/10488

Comment 12

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

bug 1066450 merged in, rebasing with another try:  https://tbpl.mozilla.org/?tree=Try&rev=14950ee32c0f

Comment 13

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

try looking good

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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