Closed Bug 1057042 Opened 6 years ago Closed 6 years ago

Refactor web audio editor client components

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → jsantell
Attached patch 1057042-wae-refactor.patch (obsolete) — Splinter Review
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
Attachment #8490367 - Flags: review?(vporof)
Blocks: 1055215
Status: NEW → ASSIGNED
Attached patch 1057042-wae-refactor.patch (obsolete) — Splinter Review
Added refactoring to the views as well, mainly allowing mixins for toggling view -- this was done for adding a visualization panel in bug 1019100
Attachment #8490367 - Attachment is obsolete: true
Attachment #8490367 - Flags: review?(vporof)
Attachment #8491070 - Flags: review?(vporof)
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.
Attachment #8491070 - Flags: review?(vporof) → review+
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
Lots of comments added, some renames, some commented reasons on the changes. Resubmitting to try: https://tbpl.mozilla.org/?tree=Try&rev=6f8cb8812209
Attachment #8491070 - Attachment is obsolete: true
Attachment #8492479 - Flags: review+
sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48566678&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
Looks like a rebase issue, checking it out
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
try looking good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad818793bdd0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.