1150889 - Remember if the inspector sidebar panel is collapsed or expanded when the toolbox restarts

Status: RESOLVED WORKSFORME

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

Bug 994055 gives the inspector the ability to collapse/expand the sidebar panel.
This bug should add persistence (via a pref) of the state.

Comment 1

[Diogo Dauster Pontual]

I want to solve this bug. Is it possible?

Comment 2

[Patrick Brosset <:pbro>]

Thanks for your interest. I'm assigning the bug to you.

The main thing to add here is a new user preference (for info, all prefs are visible in firefox by entering about:config in the address bar) that will hold the status of the inspector sidebar.
There's a bunch of prefs already, open about:config and type "devtools.inspector" and you'll see all prefs we have today.
A suggestion for the name: devtools.inspector.expandedSidebar. It should be a boolean and default to true so that all users will see the sidebar open by default.
Here's where these prefs are defined: http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1389

Once that pref is added in the code, the next step will be to add some logic to the inspector so that when it is collapsed, that sets the pref to false, and when it is expanded again, that sets the pref to true.
Here's where this should happen: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#884

And finally, some code that collapses/expands the sidebar by default depending on the current value of the pref when the inspector is opened the first time.
This could probably be done in the setup phase, around here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#357

Let me know if this is enough pointers to get started.

Comment 3

[Diogo Dauster Pontual]

Attachment: expandedSideBar.patch

Comment 4

[Diogo Dauster Pontual]

Attachment: expandedSideBar.patch

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8604061 [details] [diff] [review]
expandedSideBar.patch

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

Thanks for the patch. This is the right approach but there are some changes needed. See my comments below.
Also, we usually add automated tests (mochitests) for new features so that we're sure they don't break later when changing the code again.
Can you create a new test in /browser/devtools/inspector/test ?
There is already a few pane-toggle tests in this directory. You should create a new one and copy paste the code from another one as a starting point.
The goal of this test should be to test that:
1) the sidebar opens in the right state when the pref is set to true and false before opening the inspector
2) the pref is correctly changes when clicking the collapse/expand button

I advise you to take a look at the other tests in this directory, there's a lot of code there that you should be able to reuse for your test.
Please get needinfo me here on this bug when/if you have more questions about this.

This may be interesting:
https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
https://wiki.mozilla.org/DevTools/mochitests_coding_standards

::: browser/devtools/inspector/inspector-panel.js
@@ +309,5 @@
>     * Build the sidebar.
>     */
>    setupSidebar: function InspectorPanel_setupSidebar() {
>      let tabbox = this.panelDoc.querySelector("#inspector-sidebar");
> +    let button = this.panelDoc.getElementById("inspector-pane-toggle");

Maybe use this instead:
this.panelDoc.querySelector("#inspector-pane-toggle");
not because it's better, but just because it's more consistent with the line just before.

@@ +346,5 @@
>                            "chrome://browser/content/devtools/animationinspector/animation-inspector.xhtml",
>                            "animationinspector" == defaultTab);
>      }
>  
> +    //Display the sideBar if the pref is true.

nit: empty space betweeb // and Display

@@ +348,5 @@
>      }
>  
> +    //Display the sideBar if the pref is true.
> +    if(Services.prefs.getBoolPref("devtools.inspector.expandedSidebar")){
> +      this.sidebar.show();

Not showing the sidebar here has consequences. For example, if you hide the sidebar, then close devtools, then open devtools again: an exception is thrown during the inspector initialization.

You should do something like this instead:

    // Display the sideBar.
    this.sidebar.show();

    // But hide it straight away if needed.
    if (!Services.prefs.getBoolPref("devtools.inspector.expandedSidebar")) {
      ViewHelpers.togglePane({
        visible: false,
        animated: false,
        delayed: false
      }, tabbox );
      button.setAttribute("pane-collapsed", "");
      button.setAttribute("tooltiptext",
        strings.GetStringFromName("inspector.expandPane"));
    }

@@ +352,5 @@
> +      this.sidebar.show();
> +    }else{
> +      button.setAttribute("pane-collapsed", "");
> +      button.setAttribute("tooltiptext",
> +      this.strings.GetStringFromName("inspector.expandPane"));

Replace `this.strings` with `strings`.

@@ +905,5 @@
>        animated: true,
>        delayed: true
>      }, sidePane);
>  
> +    

A new empty line was added here, and it has trailing whitespaces. Can you get rid of this new line?

@@ +916,5 @@
>        button.setAttribute("tooltiptext",
>          this.strings.GetStringFromName("inspector.collapsePane"));
>      }
> +
> +    //Store the new sidePane state

nit: empty space between // and Store.

Comment 6

[Patrick Brosset <:pbro>]

Un-assigning since there has been no activity for a while.

Comment 7

[Avik Pal [:avikpal]]

Attachment: sidebarPanelPref.patch

Comment 8

[Avik Pal [:avikpal]]

Hey Patrick,

I've added a patch for this bug- can you assign this to me?

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8718792 [details] [diff] [review]
sidebarPanelPref.patch

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

Thanks. This works nicely.
I think we just need some minor refactoring in the code and some test changes.

::: devtools/client/inspector/inspector-panel.js
@@ +386,5 @@
> +      }, tabbox);
> +      button.setAttribute("pane-collapsed", "");
> +      button.setAttribute("tooltiptext",
> +        strings.GetStringFromName("inspector.expandPane"));
> +    }

It would be even better if you could put this in common with the code that already exists in onPaneToggleButtonClicked by creating a new function:

togglePane: function(show, delayed, animated) {
  let sidePane = this.panelDoc.querySelector("#inspector-sidebar");
  let button = this._paneToggleButton;

  ViewHelpers.togglePane({
    isVisible: show,
    delayed: delayed,
    animated: animated
  }, sidePane);

  if (!show) {
    button.setAttribute("pane-collapsed", "");
    button.setAttribute("tooltiptext",
      strings.GetStringFromName("inspector.expandPane"));
  } else {
    button.removeAttribute("pane-collapsed");
    button.setAttribute("tooltiptext",
      strings.GetStringFromName("inspector.collapsePane"));
  }
}

Or something like this.
The advantage of doing this is that you can now call this from onPaneToggleButtonClicked *and* from setupSidebar (with the right arguments) and therefore make these 2 functions a lot shorter and simpler to read.

@@ +1029,5 @@
>      }
> +
> +    // Store the sidePane state
> +    Services.prefs.setBoolPref("devtools.inspector.expandedSidebar",
> +     !isVisible);

nit: please indent wrapped lines with 2 extra spaces.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
@@ +9,5 @@
> +  "<h1>browser_inspector_pane-toggle-04.js</h1>";
> +
> +add_task(function* () {
> +  // Test is often exceeding time-out threshold, similar to Bug 1137765
> +  requestLongerTimeout(2);

Please remove this longer timeout request. This is the kind of things we only add as a last resort when tests really do take too much time and there's no way to split them.
I'm guessing this was copied from browser_inspector_expand-collapse.js and you just forgot to remove it.

@@ +17,5 @@
> +
> +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> +
> +  ok(Services.prefs.getBoolPref("devtools.inspector.expandedSidebar"),
> +	"default preference should be true");

nit: please indent with spaces only, not tabs (you might want to install eslint locally, this is the linter we now use on the mozilla JS source code to automatically report these errors: https://wiki.mozilla.org/DevTools/CodingStandards)

@@ +34,5 @@
> +  info("Closing inspector.");
> +  yield toolbox.destroy();
> +
> +  ok(!Services.prefs.getBoolPref("devtools.inspector.expandedSidebar"),
> +	"correct preference set upon sidebar closing");

Same remark about spaces vs. tabs.

@@ +39,5 @@
> +
> +  info("Re-opening inspector.");
> +  inspector = (yield openInspector()).inspector;
> +
> +  ok(panel.hasAttribute("pane-collapsed"), "The panel is in collapsed state");

You need to re-initialize the panel variable after the inspector has been re-opened, otherwise it still refers to the DOM node in the previous panelDoc.

At the end of this test, you'll also need to reset the value of the devtools.inspector.expandedSidebar pref. Otherwise, you'll cause tests after this one to fail because they might assume that the sidebar is in its expanded state.
I think this should be done in devtools/client/inspector/test/head.js. In this file, look for this:

registerCleanupFunction(() => {
  Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
});

My suggestion is to clear the new pref you added in here.

Comment 10

[Avik Pal [:avikpal]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9)
> Comment on attachment 8718792 [details] [diff] [review]
> sidebarPanelPref.patch
> 
> Review of attachment 8718792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. This works nicely.
> I think we just need some minor refactoring in the code and some test
> changes.
> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +386,5 @@
> > +      }, tabbox);
> > +      button.setAttribute("pane-collapsed", "");
> > +      button.setAttribute("tooltiptext",
> > +        strings.GetStringFromName("inspector.expandPane"));
> > +    }
> 
> It would be even better if you could put this in common with the code that
> already exists in onPaneToggleButtonClicked by creating a new function:
> 
> togglePane: function(show, delayed, animated) {
>   let sidePane = this.panelDoc.querySelector("#inspector-sidebar");
>   let button = this._paneToggleButton;
> 
>   ViewHelpers.togglePane({
>     isVisible: show,
>     delayed: delayed,
>     animated: animated
>   }, sidePane);
> 
>   if (!show) {
>     button.setAttribute("pane-collapsed", "");
>     button.setAttribute("tooltiptext",
>       strings.GetStringFromName("inspector.expandPane"));
>   } else {
>     button.removeAttribute("pane-collapsed");
>     button.setAttribute("tooltiptext",
>       strings.GetStringFromName("inspector.collapsePane"));
>   }
> }
> 
> Or something like this.
> The advantage of doing this is that you can now call this from
> onPaneToggleButtonClicked *and* from setupSidebar (with the right arguments)
> and therefore make these 2 functions a lot shorter and simpler to read.
> 
> @@ +1029,5 @@
> >      }
> > +
> > +    // Store the sidePane state
> > +    Services.prefs.setBoolPref("devtools.inspector.expandedSidebar",
> > +     !isVisible);
> 
> nit: please indent wrapped lines with 2 extra spaces.
> 
> ::: devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
> @@ +9,5 @@
> > +  "<h1>browser_inspector_pane-toggle-04.js</h1>";
> > +
> > +add_task(function* () {
> > +  // Test is often exceeding time-out threshold, similar to Bug 1137765
> > +  requestLongerTimeout(2);
> 
> Please remove this longer timeout request. This is the kind of things we
> only add as a last resort when tests really do take too much time and
> there's no way to split them.
> I'm guessing this was copied from browser_inspector_expand-collapse.js and
> you just forgot to remove it.

browser_inspect_expand-collapse.js was written by me and I faced similar issue here. I'll take a look at it once again and if it persists probably need to split it up. 

> 
> @@ +17,5 @@
> > +
> > +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> > +
> > +  ok(Services.prefs.getBoolPref("devtools.inspector.expandedSidebar"),
> > +	"default preference should be true");
> 
> nit: please indent with spaces only, not tabs (you might want to install
> eslint locally, this is the linter we now use on the mozilla JS source code
> to automatically report these errors:

I 'had' eslint configured with sublime but I guess it got broke somehow- gotta take a look once again 

> https://wiki.mozilla.org/DevTools/CodingStandards)
> 
> @@ +34,5 @@
> > +  info("Closing inspector.");
> > +  yield toolbox.destroy();
> > +
> > +  ok(!Services.prefs.getBoolPref("devtools.inspector.expandedSidebar"),
> > +	"correct preference set upon sidebar closing");
> 
> Same remark about spaces vs. tabs.
> 
> @@ +39,5 @@
> > +
> > +  info("Re-opening inspector.");
> > +  inspector = (yield openInspector()).inspector;
> > +
> > +  ok(panel.hasAttribute("pane-collapsed"), "The panel is in collapsed state");
> 
> You need to re-initialize the panel variable after the inspector has been
> re-opened, otherwise it still refers to the DOM node in the previous
> panelDoc.
> 
> At the end of this test, you'll also need to reset the value of the
> devtools.inspector.expandedSidebar pref. Otherwise, you'll cause tests after
> this one to fail because they might assume that the sidebar is in its
> expanded state.
> I think this should be done in devtools/client/inspector/test/head.js. In
> this file, look for this:
> 
> registerCleanupFunction(() => {
>   Services.prefs.clearUserPref("devtools.inspector.activeSidebar");
> });
> 
> My suggestion is to clear the new pref you added in here.

Yep. Thanks for pointing this out. I'll make the changes and put up a patch for you to review soon.

Comment 11

[Avik Pal [:avikpal]]

[continued from my earlier reply]

I have a patch ready with your suggestions incorporated- but the test still fails without requestLongerTimeout. I'm not exactly sure whether I should split up the code; My take is splitting it up would hamper readability of the scenario-s being tested. Same has been true for browser_inspect_expand-collapse.js and several other inspector tests.

I left a comment in the earlier patch with a tracking bug so as to improve readability of the code- Let me know the workflow here- so as to keep its reference or not- incase we don't decide to split up the test. 

Let me know what you think of this and I'll put up a patch accordingly.

Comment 12

[Patrick Brosset <:pbro>]

I would prefer to keep this test as one test too, as you said, this would make readability better.
Did you push to try to confirm that you had failures without requestLongerTimeout?
If you did, then I'm fine to keep it in with a comment.

Comment 13

[Avik Pal [:avikpal]]

Patrick,

I don't have access to push to try server yet- should I go ahead and request for one?

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Avik Pal [:avikpal] from comment #13)
> I don't have access to push to try server yet- should I go ahead and request for one?
Pushing to try requires "commit access level 1": https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
Pushing to try is useful to verify that your changes do not break existing tests or that your new tests pass fine on all platforms. Of course it's still important to run your tests locally first, but running them on try offers a much more complete coverage.
Of course all tests are also run when a patch lands on fx-team, but if test failures are found there, then the patch is backed-out, which is annoying and time-consuming. So try is a great safety net to use.
So it really depends on whether you'd feel more comfortable/work quicker pushing your work-in-progress patches to try yourself, or ask for someone to push them for you occasionally.

Comment 15

[Avik Pal [:avikpal]]

Hey Patrick,

Apologies for being late- I was caught up with some other responsibilities and could only take a look today. Meanwhile I got the try server push access.

Today after checking out the latest codebase I started fiddling around and came across an exception occurring after http://lxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#374 what I could decipher from the stack trace and some debugging is that somehow an event (most likely a mutation of selected node) has prompted the inspector to close/destroy itself resulting in the exception- apparently inspector is logging many such exception in the latest tree state.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Avik Pal [:avikpal] from comment #15
> Today after checking out the latest codebase I started fiddling around and
> came across an exception occurring after
> http://lxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> inspector-panel.js#374 what I could decipher from the stack trace and some
> debugging is that somehow an event (most likely a mutation of selected node)
> has prompted the inspector to close/destroy itself resulting in the
> exception- apparently inspector is logging many such exception in the latest
> tree state.
I can't really help with this description. Does this happen when you run your test? Or just when using Firefox?
Do you have a steps to reproduce? Can you paste the stacktrace here?
Let me know how I can help you land this patch.

Comment 17

[Avik Pal [:avikpal]]

Attachment: sidebarPanelPref.patch

The previous error/exception that I faced earlier may be due to a recent change in devtools- that of unhandled promise rejection being enabled for devtools which made existing tests fail for a period of time.

So I checked out the latest codebase and now my patch shows test timeout irrespective of whether I use requestLongerTimeout() or not- I checked several existing devtools tests and they also got timed out. What am I missing here?

P.S. I haven't pushed this to try to take a look at how it behaves over there- I want to make sure that something isn't missing from my end.

Comment 18

[Avik Pal [:avikpal]]

It seems that I somehow messed up my source tree- I'll update once I get a clean patch.

Comment 19

[Avik Pal [:avikpal]]

Attachment: sidebarPanelPref.patch

So with this patch a couple of tests fails due to unhandled promise rejections- as I pointed out in an earlier comment.

What's the workflow here? shall I go ahead and file a bug to be tracked under Bug 1240804 or should it be fixed within purview of this bug itself?

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8739683 [details] [diff] [review]
sidebarPanelPref.patch

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

The code looks good. But since you said the test failed with a time out, I'm going to cancel the review for now, and keep the needinfo flag so I remember to take a look at the test locally and try and make it pass. 
Sorry I can't do this today though, I have a number of other reviews to do and for some reasons, my tests don't run today... I need to look into this.

::: devtools/client/inspector/inspector-panel.js
@@ +341,5 @@
>     * Build the sidebar.
>     */
>    setupSidebar: function() {
>      let tabbox = this.panelDoc.querySelector("#inspector-sidebar");
> +    let button = this.panelDoc.querySelector("#inspector-pane-toggle");

The button variable is not used in setupSidebar, please remove it.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
@@ +34,5 @@
> +  info("Closing inspector.");
> +  yield toolbox.destroy();
> +
> +  ok(!Services.prefs.getBoolPref("devtools.inspector.expandedSidebar"),
> +	"correct preference set upon sidebar closing");

Mixed spaces/tabs here. Please only use spaces to indent.
You can run eslint locally with ./mach eslint path/to/file

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8739683 [details] [diff] [review]
sidebarPanelPref.patch

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

I think I found the issue.
Also, a new test that has the same name as your test just landed in the code, and so there's now a conflict. Since I wanted to try your patch locally, I had to solve the conflict, and therefore, instead of you having to solve it too, I'll just upload my patch in a second.

::: devtools/client/inspector/inspector-panel.js
@@ +373,5 @@
>      }
>  
>      this.sidebar.show(defaultTab);
> +    if (!Services.prefs.getBoolPref("devtools.inspector.expandedSidebar")) {
> +      this.togglePane(false, false, false);

the problem with calling this here is that by this time, the variable this._paneToggleButton hasn't been initialized yet! But we're trying to use it in togglePane.
It will only be initialized when setupSidebarToggle is called, on the next line.

This most probably explains the test breakage you're seeing. And you most probably have JS errors in the browser console (ctrl+shift+J) when you try the feature (try to collapse the sidebar, close devtools, and open it again).

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8739683 [details] [diff] [review]
sidebarPanelPref.patch

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

::: devtools/client/inspector/inspector-panel.js
@@ +384,5 @@
> +    let sidePane = this.panelDoc.querySelector("#inspector-sidebar");
> +    let button = this._paneToggleButton;
> +
> +    ViewHelpers.togglePane({
> +      isVisible: show,

Actually, I found another problem here. The togglePane method does not expect a property named isVisible, but visible!
So this was never going to work.

Comment 23

[Patrick Brosset <:pbro>]

Attachment: Bug_1150889_-__Remember_if_the_inspector_sidebar_p.diff

Here's the corrected patch. Please do take a look at it, and make any changes you think should be made and request a final review.

Comment 24

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES

Comment 25

[Avik Pal [:avikpal]]

(In reply to Patrick Brosset [:pbro] from comment #22)
> Comment on attachment 8739683 [details] [diff] [review]
> sidebarPanelPref.patch
> 
> Review of attachment 8739683 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +384,5 @@
> > +    let sidePane = this.panelDoc.querySelector("#inspector-sidebar");
> > +    let button = this._paneToggleButton;
> > +
> > +    ViewHelpers.togglePane({
> > +      isVisible: show,
> 
> Actually, I found another problem here. The togglePane method does not
> expect a property named isVisible, but visible!
> So this was never going to work.

I do see some errors being logged on the JS console but don't why even with the error it works locally for me.

Nonetheless I'll take a look at your patch and let you know- I do apologise for bothering you with silly mistakes like these.

Comment 26

[Patrick Brosset <:pbro>]

Unfortunately this was left off 3 years ago.
Also, since then, the sidebar in the inspector has changed a lot. We do have persistence for the collapsed state now.
So I'll close this.