1097887 - Autozoom the graph to show all the nodes

Status: RESOLVED INVALID

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Soledad Penades [:sole] [:spenades]]

It's specially cool when you're working with code that creates a lot of nodes and it gets "too vertical" and it's annoying to keep zooming out (not sure if this makes sense?)

Comment 1

[Soledad Penades [:sole] [:spenades]]

Jordan says in irc: "should be pretty straight forward, might make sense to be a checkbox button"

Comment 2

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

This should be pretty straight forward, I think, if you have some D3 knowledge

Comment 3

[Ciro S. Costa [:cirocosta]]

Hey Jordan, i'd like to work on this. Could you provide more info on how to proceed (if ok to take it)? 
Already have a build of fx-team but i'm not into the code (i mean, not much more than knowing that the code *might* be in fx-team/browser/devtools/webaudioeditor and that this *might* relate to 'views'). 

So, the idea here is to make it seem like an 'auto-zoom-out' if, e.g., i dinamically add a node and then it gets bigger then the viewing area?

Comment 4

[Ciro S. Costa [:cirocosta]]

woops, read it again, not i got the sense of the checkbox thing.

Comment 5

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

Hey Ciro! The code is in `./browser/devtools/webaudioeditor/views/context.js`, so this is the view for the D3 graph shown. When a node changes state, the `draw()` function here gets called and rerenders everything, so I think after this function ends is a good place to adjust the zoom level. In terms of the checkbox, there's a patch coming soon that syncs checkboxes with prefs, so for this, might be better to just add a pref to enable this in `./browser/app/profile/firefox.js` (probably called `devtools.webaudioeditor.autozoom`). Let me know if you have any questions, and myself and others are available for help in the #devtools channel

Comment 6

[Ciro S. Costa [:cirocosta]]

Attachment: bug1087887-wa-autozoom.patch

So, this patch adds:
  - hidden checkbox to the webaudioeditor toolbar (shows if pref if set)
  - zoom-out if the graph is bigger (in height) than the svg visible area (keeping the scale if the graph isn't bigger)
  - toggle the checkbox on move/zoom of the graph
  - ontoggle -> applies the zoom-out if necessary or goes back to the initial
  - tests for the above.

Comment 7

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

Thanks Ciro! I'll review this soon for you

Comment 8

[Ciro S. Costa [:cirocosta]]

Hi Jordan!
Firstly, sorry for the delay! Had to spend some time with mercurial. 
I hope that i got what was the point of this; the patch got a little big.

Something that i've notice while coding: when dinamically setting `$("#my-xul-checkbox").checked = true` a checkbox doesn't get updated (it only updates on hover). Is this a known bug?

Also, i just saw that the name of the patch is wrongly set to another bug - sorry for that!
Here's a quick video of how it looks (in terms of UI):

http://youtu.be/gzaF6OPgddM  (in :25 you can see the .checked changing only on hover :( )


(submitting a video demonstrating the feature is a good way of doing this or there's a better approach?)

Thanks for your support!

Comment 9

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

Comment on attachment 8561806 [details] [diff] [review]
bug1087887-wa-autozoom.patch

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

Overall looks great! Some comments I left throughout the code as well.

As a high level, like you mentioned, keeping checkboxes in sync with prefs has been annoying for other panels, so we made an OptionsView in bug 1121700, currently used in the new performance tools (hidden behind build flag). This introduces some complexity, and I don't think we're quite ready to have a checkbox for this tool just yet -- let's just manage it via prefs for this bug, so fetch the state of the auto zoom feature on every render. I can make a new bug for adding the OptionsView in for this as well that I can wrap up to hook in with this, which would be a great combo.

So from a high level: 
* Let's not worry about the checkbox for now, this adds more complexity and we can add the OptionsView in the future to make this easier to toggle, so this reduces a lot of code in the patch as well
* I think just using an existing HTML file for testing will work for the big graph
* Let's follow up with Sole on the autozoom disabling when a manual zoom occurs
* There's some whitespace scattered around the patch

Next steps are to make these changes, and then another review and then we'll be close to sending it out to our test servers to make sure everything passes. Thanks again!

::: browser/devtools/webaudioeditor/test/browser_wa_graph-auto-zoom-01.js
@@ +43,5 @@
> +  is(ContextView.getCurrentScale(), 10, "After zoom, scale is 10.");
> +  is(ContextView.getCurrentTranslation()[0], 100, "After zoom, x-translation is 100.");
> +  is(ContextView.getCurrentTranslation()[1], 400, "After zoom, y-translation is 400.");
> +
> +  // re-Activate autozoom 

whitespace

@@ +46,5 @@
> +
> +  // re-Activate autozoom 
> +
> +  ContextView._onAutoZoomToggle();
> +  

whitespace

::: browser/devtools/webaudioeditor/test/browser_wa_graph-auto-zoom-02.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Tests autozoom functionality for audio context's graph for big graphs

I think this comment should be updated -- this is testing when a graph does NOT get autozoomed, correct?

@@ +43,5 @@
> +  is(ContextView.getCurrentScale(), 10, "After zoom, scale is 10.");
> +  is(ContextView.getCurrentTranslation()[0], 100, "After zoom, x-translation is 100.");
> +  is(ContextView.getCurrentTranslation()[1], 400, "After zoom, y-translation is 400.");
> +
> +  // re-Activate autozoom 

whitespace

@@ +46,5 @@
> +
> +  // re-Activate autozoom 
> +
> +  ContextView._onAutoZoomToggle();
> +  

whitespace

::: browser/devtools/webaudioeditor/test/doc_graph-auto-zoom.html
@@ +19,5 @@
> +
> +      while (sourcesN--) {
> +        let osc = context.createOscillator();
> +
> +        oscs.push(osc)

I think we can just use `doc_simple-node-creation.html`, as that has a lot of nodes that would trigger the autozoom rather than having a new file

::: browser/devtools/webaudioeditor/views/context.js
@@ +5,5 @@
>  
>  const { debounce } = require("sdk/lang/functional");
> +// autozoom pref
> +const gEnableAutoZoom = 
> +  Services.prefs.getBoolPref("devtools.webaudioeditor.autozoom");

This should be fetched everytime autozoom can potentially be called rather than a global

@@ +250,5 @@
>      // store as `this._zoomBinding` so we can unbind during destruction
>      if (!this._zoomBinding) {
>        this._zoomBinding = d3.behavior.zoom().on("zoom", function () {
> +        if (enableAutoZoom)
> +          this._autoZoomToggle.checked = (enableAutoZoom = false);

So looks like if a user manually zooms, the auto zoom is disabled. I can go either way on this, but let's ask Sole (reporter of the bug and does a lot of work with the web audio API) on her thoughts too.

@@ +266,5 @@
>      }
> +
> +    // handles zooming to a proper representation of the graph, matching the
> +    // height of the available svg height
> +    if (gEnableAutoZoom) {

All the autozooming function should be in a new method, like `_autozoom`; that way, when we add the OptionsView in the future, we can just call this function directly when the pref changes, and then `render` would just call it as well.

@@ +268,5 @@
> +    // handles zooming to a proper representation of the graph, matching the
> +    // height of the available svg height
> +    if (gEnableAutoZoom) {
> +      if (!this._autoZoomBinding) {
> +        this._autoZoomBinding = () => {

The `_autoZoomBinding` looks like it was built the same way as _zoomBinding. _zoomBinding was done that way, so we only bind the zoom event once, after render, and have a reference to it so we can unbind it during destruction.

This function however, is not an event where we need to unbind it, so we can just unwrap it as a function, like the `_autozoom` function I mentioned.

@@ +270,5 @@
> +    if (gEnableAutoZoom) {
> +      if (!this._autoZoomBinding) {
> +        this._autoZoomBinding = () => {
> +          if (enableAutoZoom && gTarget.node().getBBox().height) {
> +            let newScale = (svg.node().getBoundingClientRect().height - 40) / 

Some comments here explaining how this new scale is calculated would be helpful, and also, where does `40` come from?

@@ +274,5 @@
> +            let newScale = (svg.node().getBoundingClientRect().height - 40) / 
> +                           (gTarget.node().getBBox().height);
> +
> +            if (newScale >= 1) {
> +              // Few nodes in same layer: go back to original

To clarify, this is what happens when there are not enough nodes, and it's zoomed in too much? Good case to cover!

@@ +277,5 @@
> +            if (newScale >= 1) {
> +              // Few nodes in same layer: go back to original
> +              this._zoomBinding.scale(1);
> +              this._zoomBinding.translate([20, 20]);
> +              gTarget.attr("transform", "translate(20,20) scale(1)");              

extra whitespace

Comment 10

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

Sole, Ciro's working on the implementation for the autozoom and it's coming along great. There's a feature, where, if you have autozoom on, and begin to manually zoom (scroll wheel, etc), autozoom becomes disabled. Currently, with the autozoom disabling, this prevents the scenario where you zoom in, and a new node appears, and then suddenly you're back to the default zoom level, which would be jarring. An alternative would be to always autozoom if the pref is on. Or maybe the pref just determines the default zoom state, so, if the pref is on, it'll autozoom until you manually zoom, in which case, autozoom is OFF for the remainder of the document-life. Upon reload, the graph would be autozoomed again. I think I'm liking the last option the most, as it seems like a good compromise between managing a giant cluster of nodes, no jarring view switches, and not flipping back and forth pref options.

Sole, what're your thoughts on how this should work?

Comment 11

[Ciro S. Costa [:cirocosta]]

Thanks for reviewing jsantell! 
I'd go with the last option you gave, but i think that an indicator of it (e.g., the checkbox thing) improves a lot the experience (as the user actually knows whats going on).

Anyway, just made the changes, adding a 'TODO' in the part that regards Sole's decision.
That hardcoded '40' in the calculation of the newscale is the inclusion of a '20px' padding on top and bottom (so that the first and last node don't get super close to the border).
The comment in the second test was outdated as you mentioned, fixed  that! Also, regarding the tests, to simulate the 'toggle' functionality that i had added in the first patch i just switched to re-draw the graph after a change in the prefs. Is that ok or that yield slows down the tests in general and shouled be avoided?


Thanks for your attention jsantell!

Comment 12

[Ciro S. Costa [:cirocosta]]

Attachment: Adds _autozoom method (conditionated to a pref) along with tests for it.

Comment 13

[Soledad Penades [:sole] [:spenades]]

Hi! Hey Ciro-thanks for submitting the patch, you're awesome! :-)

Answering to your question, yes, I think that the last option would be the less unnerving / jarring:
- Start by zooming automatically while nodes are added, but stop if a manual zoom happens. I guess the autozoom checkbox should reflect that it has been temporarily disabled-not sure if we have states for this?
- Upon page reload, if autozoom is still enabled, go back to autozooming :-)

sounds like your algorithm?

(also sorry I took so long to answer)

Comment 14

[Ciro S. Costa [:cirocosta]]

Hi Sole! Yeah, very close :D It lacks tests for autozoom on node creation and the behavior after page reload.

Regarding the checkbox, should i go for it w/ the optionsview or let it go without it?

Thanks for your attention!

Comment 15

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

Comment on attachment 8564607 [details] [diff] [review]
Adds _autozoom method (conditionated to a pref) along with tests for it.

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

Looks good :) think there'll just be a few more fixes after Sole's feedback -- only autozooming if the pref is on and the graph hasn't been manually moved. Some of the tests probably need to be updated to reflect that. No need for the OptionsView just yet, there's a few pref things being added to web audio editor, so we'll do those all at once. I think after those changes, we should be good to go!

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 17

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 18

[Pavan Karthik [:matrixisreal]]

Hi, I'd like to work on this bug,would u assign this to me?
Also, Id appreciate some help in the testing process, on how it is done and on what all tests to run.
Thanks :D

Comment 19

[shivam gupta]

hello ,i am new here and would like to work on this as my first bug.would you assign it to me?
any info regarding how to test it and where to proceed will be very helpful.

Comment 20

[BMO Automation]

The code behind DevTools:Web Audio Editor has gone away. Closing this bug as INVALID