1009056 - Web Audio Editor inspector width should be a preference

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

To store the width that the user sets it, like the debugger sidepanel's width

Comment 1

[Yash Mehrotra]

hi, I would like to work on this bug. I have already set up my firefox dev environment. Can you please help me get started with this bug.

Comment 2

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

Hello Yash! Thanks for checking out this bug. Some background information:

The inspector view can be found in ./browser/devtools/webaudioeditor/views/inspector.js

We'll need to do 2 things:

* Set the inspector width to whatever value this preference is when it starts up (done in the initialize() function, currently setting to the hardcoded INSPECTOR_WIDTH value.
* On resize (onResize() function), we should set the preference to the new width of the inspector (which is stored as this.el in the inspector view)

For setting and storing the pref, we can use the Services global:

Services.prefs.setIntPref(PREFNAME, value);
Services.prefs.getIntPref(PREFNAME);

For a pref name, lets go with `devtools.webaudioeditor.inspectorWidth`, which will have to be set in `./browser/app/profile/firefox.js`. By default, this should be the value of INSPECTOR_WIDTH in the current tools.

Let me know if you have any questions, and hop on our irc channel #devtools where me or someone else can help!

Comment 3

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

Testing will be tricky, but there are lots of tests currently (browser/devtools/webaudioeditor/test/) that should get you to the right spot. Make sure you add the new test file to the browser.ini in that directory

Comment 4

[Yash Mehrotra]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Hello Yash! Thanks for checking out this bug. Some background information:
> 
> The inspector view can be found in
> ./browser/devtools/webaudioeditor/views/inspector.js
> 
> We'll need to do 2 things:
> 
> * Set the inspector width to whatever value this preference is when it
> starts up (done in the initialize() function, currently setting to the
> hardcoded INSPECTOR_WIDTH value.
> * On resize (onResize() function), we should set the preference to the new
> width of the inspector (which is stored as this.el in the inspector view)
> 
> For setting and storing the pref, we can use the Services global:
> 
> Services.prefs.setIntPref(PREFNAME, value);
> Services.prefs.getIntPref(PREFNAME);
> 
> For a pref name, lets go with `devtools.webaudioeditor.inspectorWidth`,
> which will have to be set in `./browser/app/profile/firefox.js`. By default,
> this should be the value of INSPECTOR_WIDTH in the current tools.
> 
> Let me know if you have any questions, and hop on our irc channel #devtools
> where me or someone else can help!

Ok, Thanks Jordan. I think I understood most of what you said, I will be submitting a patch within 3-4 days.

Comment 5

[Yash Mehrotra]

Attachment: 1009056.diff

I have submitted a patch in accordance to what you said. I haven't tested it yet as I wanted to know whether this is good in theory or any other changes are to be made?

Comment 6

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

Yash, looking great! Just make sure that the pref that's being set is the width of the inspector, not the element itself.

Also an edge case to look for: is it possible to make the inspector so small that it's hard to see it? Maybe we should set a minimum width during the initialization, so if that this happens, when a user reopens it's set to 300 or so.

Comment 7

[Yash Mehrotra]

Attachment: Patch Updated

Hi, I updated the patch with some minor fixes and also included the edge case you were mentioning. Is it good enough or some other changes also need to be made ?

Comment 8

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

Comment on attachment 8562728 [details] [diff] [review]
Patch Updated

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

Looks great! Just some fixes in there, as it will not currently run due to `pref` being undefined. But this is the idea, and I think we're good for tests now. Should be straight forward to test, ensure that sliding the inspector changes the pref (may be hard to do in a test, so can fake by calling _onResize), and that when restarting the tool, the width of the inspector is the same as what is set in the pref. Let me know if you have any questions!

::: browser/devtools/webaudioeditor/views/inspector.js
@@ -4,5 @@
>  "use strict";
>  
> -// Store width as a preference rather than hardcode
> -// TODO bug 1009056
> -const INSPECTOR_WIDTH = 300;

Let's leave this const here as `MIN_INSPECTOR_WIDTH` so we don't have magic numbers later (with 300 just showing up)

@@ +27,5 @@
>    initialize: function () {
>      // Set up view controller
>      this.el = $("#web-audio-inspector");
>      this.splitter = $("#inspector-splitter");
> +    this.el.setAttribute("width", pref("devtools.webaudioeditor.inspectorWidth"));

`pref` is not defined here. You'll want Services.prefs.getIntPref and Services.prefs.setIntPref

@@ +144,5 @@
>      this.show();
>    },
>  
>    _onResize: function () {
> +    if (this.el.getAttribute("width") < 300 ) {

Use MIN_INSPECTOR_WIDTH here

Comment 9

[Yash Mehrotra]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)

> `pref` is not defined here. You'll want Services.prefs.getIntPref and
> Services.prefs.setIntPref
> 

For the above, is pref("foo","bar"); correct for firefox.js ?
I wasn't sure how the Services.prefs work, so now I am assuming I need to write - Services.prefs.setIntPref("devtools.webaudioeditor.inspectorWidth", MIN_INSPECTOR_WIDTH);

I will be submitting a new patch fixing this.

Comment 10

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

For firefox.js, that is correct -- this creates the preference in about:config.

For the code in the inspector, `Services.prefs` has methods to set and get different types of preferences like, setIntPref and getIntPref, which would be used here since we're dealing with an integer pref for the width.

Yes, anywhere you access or set the preference, you should use the Services.prefs methods. Thanks!

Comment 11

[Yash Mehrotra]

Attachment: 1009056.diff - Updated

Hi, updated it. Thanks for helping me out. Are any more changes required ?

Comment 12

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

Looks great! Do you think you can write a test for this, mentioned in comment #8? Adding a test file in ./browser/devtools/webaudioeditor/test/, adding it to browser.ini there, and instructions for running tests here:
https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests

Comment 13

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

Comment on attachment 8562898 [details] [diff] [review]
1009056.diff - Updated

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

Ping me for a review once there are tests, let me know if you need any help with them!

Comment 14

[Yash Mehrotra]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13)
Ok Jordan, thanks, I have started working on the tests. I am new to this so its taking a bit more time than usual, I will sending the test patch in 2-3 days. BTW, should I send a different patch for the test or all the changes in one patch ?

Comment 15

[Yash Mehrotra]

Attachment: browser_wa_inspector-width.js

I added a basic test, it changes the width, reloads, and then checks whether the width is the same or not. Are there any other additions that should be made for the test ? Also, if there are mistakes, please tell me where I need to make the changes.

Comment 16

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

Comment on attachment 8564571 [details]
browser_wa_inspector-width.js

Thanks Yash! Looks good. Few things:

* The tests and fix should all be in one patch
* You'll have to add the test to browser.ini
* The test probably has to wait for the nodes to register themselves and click on a node to open the inspector after a refresh to test, I don't think this test will pass
* Ensure the test passes, and no other tests break

Comment 17

[Yash Mehrotra]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)
> Comment on attachment 8564571 [details]
> browser_wa_inspector-width.js
> 
> Thanks Yash! Looks good. Few things:
> 
> * The tests and fix should all be in one patch
Ok, will do it.
> * You'll have to add the test to browser.ini
Yes, I added it to browser.ini but forgot to send that.
> * The test probably has to wait for the nodes to register themselves and
> click on a node to open the inspector after a refresh to test, I don't think
> this test will pass
Can you please elaborate on this part, I am not able to comprehend what you mean (code wise)
> * Ensure the test passes, and no other tests break
Ok, sure.

Comment 18

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

After reloading the page the second time in the test, you should wait for the nodes to be rendered, like you have earlier in the test:

[actors] = yield Promise.all([
  get3(gFront, "create-node"),
  waitForGraphRendered(panelWin, 3, 2)
]);

After that, you might need to open up the inspector to get the width (although I think maybe just the margin is changed when opening)

Comment 19

[Yash Mehrotra]

Attachment: browser_wa_inspector-width.js

The test file is added to the browser.ini so I didn't send that.
I made various to the file according to what you said. Please tell me if any other changes are required. Thanks!

Comment 20

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

Yash, that's the right idea

* currently, there are some syntax errors, and redeclaration of nodeIds (should leave the second `let` out, as you can only use that once per scope for a variable), so the test fails immediately.

* `new_inspector_width` should be camelCase

* Trying out the patch seems like everything's ok!

Next steps are to combine everything into one patch, and clean the tests up so they're passing for one last review

Comment 21

[Yash Mehrotra]

Attachment: 1009056.diff - Updated

Hi Jordan, I made the changes accordingly as you mentioned in comment #20.
All the changes are in one patch.

Comment 22

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

Comment on attachment 8566130 [details] [diff] [review]
1009056.diff - Updated

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

Still syntax errors in test:

17 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js | Exception thrown - at chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js:9 - SyntaxError: missing { before function body
SUITE-END | took 7s

Be sure to run tests for just this file:

./mach mochitest-devtools browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js

As well as the entire tool:

./mach mochitest-devtools browser/devtools/webaudioeditor/

Everything looks good though, just a matter of actually getting the tests to pass

Comment 23

[Yash Mehrotra]

Attachment: 1009056.diff

Sorry for the syntax errors. Updated the patch.
I wanted to ask you a question regarding firefox building. Sometimes when I update my tree (`hg pull -u`) and after building it, it shows many errors and does not build. To fix it, I change my nodeset to a previous revision (by hit and trial).

What is the best way to fix this, as due to this, 90% of times, I can't test my code.
Thanks!

Comment 24

[Tim Nguyen :ntim]

(In reply to Yash Mehrotra from comment #23)
> Created attachment 8566993 [details] [diff] [review]
> 1009056.diff
> 
> Sorry for the syntax errors. Updated the patch.
> I wanted to ask you a question regarding firefox building. Sometimes when I
> update my tree (`hg pull -u`) and after building it, it shows many errors
> and does not build. To fix it, I change my nodeset to a previous revision
> (by hit and trial).
> 
> What is the best way to fix this, as due to this, 90% of times, I can't test
> my code.
> Thanks!

You should make sure you pop all the patches out first before pulling (hg qpop -a).
If you get errors for building, you can try clobbering (mach clobber) then do a full build (mach build).
Btw, you can always reach #introduction on IRC if you need help.

Comment 25

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

Also when updating, sometimes you can get away with not having to rebuild everything and clobbering. I use this script for that: https://github.com/jsantell/home/blob/master/bin/reclobber

If making changes to just the web audio tool you can do 
./mach build browser/devtools/webaudioeditor

You shouldn't have to hg pull often as the web audio tools do not have a high rate of patches landing so you should be OK without it

Comment 26

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

Still errors running the tests -- if you're on IRC, I'm jsantell on there and can help you through the building/testing process

Comment 27

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

Tried to send this to you on IRC but you logged off and I'll be away for a few minutes, but we do run tests on an online server on all environments, example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=249641b765d3 But they need to pass locally first. If you'd like I can fix the syntax errors if you're having trouble running the tests and push to try, as the testing logic itself works

Comment 28

[Yash Mehrotra]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #27)
> Tried to send this to you on IRC but you logged off and I'll be away for a
> few minutes, but we do run tests on an online server on all environments,
> example:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=249641b765d3 But
> they need to pass locally first. If you'd like I can fix the syntax errors
> if you're having trouble running the tests and push to try, as the testing
> logic itself works

Thanks a lot, my University provides a **** internet connection and hence was getting disconnected from IRC multiple times.

So, I went through the let documentation on MDN. So, if I substitute let [actors] with var [actors] will it be all right?

Comment 29

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

Yes, or just omit "let" the second time and it becomes an assignment rather than a declaration

Comment 30

[Yash Mehrotra]

Attachment: 1009056.diff

I changed `let [actors]` to `[actors] = `
I hope this is good.

Comment 31

[Yash Mehrotra]

Attachment: 1009056.diff

hopefully the final patch.Thanks for the help.

Comment 32

[Yash Mehrotra]

Attachment: 1009056.diff

added r=jsantel

Comment 33

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

Comment on attachment 8567276 [details] [diff] [review]
1009056.diff

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

Looks great, all tests are passing for me locally! Sending it up to the try server -- if everything looks good there, we'll get this merged in. Thanks, Yash!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=630c5454e47d

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/116ca223e290