766054 - [debugger] experiment with collapsed panels

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Paul Rouget [:paul]]

I am not sure about this, but might be interesting to try:

It would be great to hide the 2 side panels if they are empty, and only unfold them when the user add a breakpoint.

I think it can be interesting if we use the debugger also as a "Script Viewer".
See bug see bug 766001.

Comment 1

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

I'll try this out.

Comment 2

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

Super easy way of doing this: https://gist.github.com/94681c90d9e158b56f2b
Of course, some transitions would be sweet.

I think we should combine the resizing operation with collapsing. For example, if the user resizes a panel to less than 50px, then we can safely assume he doesn't want it in his way, so we collapse it.

In any case, however, after collapsing, we need a way of uncollapsing the panel. This means putting a button somewhere, although I'm not very sure where in the current UI.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #2)
> Super easy way of doing this: https://gist.github.com/94681c90d9e158b56f2b
> Of course, some transitions would be sweet.
> 
> I think we should combine the resizing operation with collapsing. For
> example, if the user resizes a panel to less than 50px, then we can safely
> assume he doesn't want it in his way, so we collapse it.
> 
> In any case, however, after collapsing, we need a way of uncollapsing the
> panel. This means putting a button somewhere, although I'm not very sure
> where in the current UI.

I believe the most-used pattern today is a splitter-that-looks-more-like-a-button with an arrow. Clicking that returns the splitter to the default place.

Comment 4

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

(In reply to Panos Astithas [:past] from comment #3)
> 
> I believe the most-used pattern today is a
> splitter-that-looks-more-like-a-button with an arrow. Clicking that returns
> the splitter to the default place.

Indeed. However, there's something about those that makes me shiver. The way I see it, having thick(ish) long rectangles at the edges of the debugger is ugly. How about: two icons in the top toolbar, right-aligned. They would also allow collapsing the panels with only one click.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #4)
> (In reply to Panos Astithas [:past] from comment #3)
> > 
> > I believe the most-used pattern today is a
> > splitter-that-looks-more-like-a-button with an arrow. Clicking that returns
> > the splitter to the default place.
> 
> Indeed. However, there's something about those that makes me shiver. The way
> I see it, having thick(ish) long rectangles at the edges of the debugger is
> ugly. How about: two icons in the top toolbar, right-aligned. They would
> also allow collapsing the panels with only one click.

A couple of view toggle buttons, eh? Maybe, dunno. Let's hear what others think.

Comment 6

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

Attachment: v1

This pretty much works.
We need icons for those buttons.

Comment 7

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

Attachment: empty toggle buttons

Any ideas? :)

Comment 8

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

Attachment: v2

Toggle states and pane visibility now persist across sessions.

Comment 9

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

Automatically showing and hiding panes is really weird and I feel it's bad UX; for example, usually both the stackframes and variables panes (left + right) appear at the same time when a breakpoint is hit -> things fly around -> user confused.

The current implementation feels more natural.

Comment 10

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

Attachment: v2.1

Added test.
All we need now is two icons.

Comment 11

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

Green: https://tbpl.mozilla.org/?tree=Try&rev=1d8c9f1a8c6d

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 644224 [details] [diff] [review]
v2.1

interesting, this is nice behavior.

Comment 13

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #12)
> Comment on attachment 644224 [details] [diff] [review]
> v2.1
> 
> interesting, this is nice behavior.

I think this'll work great for bug 766001!

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

I like it. If anyone thinks the two new buttons are too much visual noise, we could add them as options in a gear-like options menu.

Comment 15

[Paul Rouget [:paul]]

Can we only have 2 states:
- 3 panes open
- only central pane open

Not sure about that: the toggle button could be on the sourceeditor (top right corner). A kind of "fullscreen" button (or "see details" button).

Comment 16

[Stephen Horlander [:shorlander]]

Attachment: Side Panel Toggles

(In reply to Panos Astithas [:past] from comment #14)
> I like it. If anyone thinks the two new buttons are too much visual noise,
> we could add them as options in a gear-like options menu.

I am not sure if they are too much visual noise. It might be a good idea to visually distinguish them from the actual debugger controls. It would be strange to have both buttons together on one side. They should be probably be contextual, i.e, icon for the left panel on the left and the icon for the right panel on the right.

If we are thinking about adding any more options like this then a gear menu might make more sense.

Comment 17

[Stephen Horlander [:shorlander]]

Comment on attachment 643761 [details]
empty toggle buttons

See comment 16

Comment 18

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

(In reply to Paul Rouget [:paul] from comment #15)
> Can we only have 2 states:
> - 3 panes open
> - only central pane open
> 
> Not sure about that: the toggle button could be on the sourceeditor (top
> right corner). A kind of "fullscreen" button (or "see details" button).

I like this idea a lot.
I can't think of a valid reason to have only one of the stackframes or variables panes collapsed. In that case, would a single, lionish-style ↔ button make more sense?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Stephen Horlander from comment #16)
> (In reply to Panos Astithas [:past] from comment #14)
> > I like it. If anyone thinks the two new buttons are too much visual noise,
> > we could add them as options in a gear-like options menu.
> 
> I am not sure if they are too much visual noise. It might be a good idea to
> visually distinguish them from the actual debugger controls. It would be
> strange to have both buttons together on one side. They should be probably
> be contextual, i.e, icon for the left panel on the left and the icon for the
> right panel on the right.

Nice icons! And yes, putting them on the sides feels more intuitive.

> If we are thinking about adding any more options like this then a gear menu
> might make more sense.

We already have the "Pause on exceptions" option that we plan to move to a gear menu at some point.

I think I like Paul's idea of a "fullscreen" button better, though. The main use of this feature is a "view source" replacement (or to simply focus on the code), so collapsing both sides at the same time makes sense. And this could be done from either a Lion-esque button or a gear menu option.

Comment 20

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

(In reply to Panos Astithas [:past] from comment #
> 
> I think I like Paul's idea of a "fullscreen" button better, though. The main
> use of this feature is a "view source" replacement (or to simply focus on
> the code), so collapsing both sides at the same time makes sense. And this
> could be done from either a Lion-esque button or a gear menu option.

Going an extra step via a gear menu to hide/show the panels does not feel comfortable.

I think that given the most likely use case (starting the debugger as a view-source), you'd want to first show the panes for extra debugging needs at some point. This is not an *option*, but a quick functionality switch.

Comment 21

[Kevin Dangoor]

(In reply to Victor Porof [:vp] from comment #20)
> I think that given the most likely use case (starting the debugger as a
> view-source), you'd want to first show the panes for extra debugging needs
> at some point. This is not an *option*, but a quick functionality switch.

There's also been the idea kicked around of merging Scratchpad with the Debugger. Clearly, there are details that would need to be worked out... but the basic idea of being able to edit and run code (sometimes with debugging controls, sometimes not) seems to have merit and supports the idea of focusing on the code.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Just to get this moving again (and hopefully move it out of my review queue!), I think I like Victor's reasoning about having a single "maximize" button for the source view. Kevin backs this up with the notion of "focusing on the code".

I like Stephen's icons for the two side-panels, but think that still means extra fiddling with toggles. I have to hit 2 things to reveal the sidepanels again. Or 2 things to merely expand my source area.

I wonder if we could combine the two icons into a single one that has arrows on both sides to indicate that we're expanding and collapsing the *middle* vs. each thing on the side?

e.g., >||< and <||>.

That could get us down to one toolbar button that doesn't eat up toolbar space in the source area. It could live on the main toolbar.

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 644224 [details] [diff] [review]
v2.1

canceling review while waiting on a single button implementation.

Comment 24

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> Comment on attachment 644224 [details] [diff] [review]
> v2.1
> 
> canceling review while waiting on a single button implementation.

I'll add the updated patch as soon as I get the single-button-image-thing.

Comment 25

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

Ping shorlander for pixels awesomeness.

Comment 26

[Stephen Horlander [:shorlander]]

Attachment: Expand Icon (OSX)

Comment 27

[Stephen Horlander [:shorlander]]

Attachment: Collapse Icon (OSX)

Comment 28

[Stephen Horlander [:shorlander]]

Attachment: Expand Icon (Windows)

Comment 29

[Stephen Horlander [:shorlander]]

Attachment: Collapse Icon (Windows)

Comment 30

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

Attachment: v3

Thanks shorlander!

Comment 31

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

Btw, Rob, can you please take a glance at how this looks on Windows? The CSS looks right to me though.

Comment 32

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

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

Comment 33

[Rob Campbell [:rc] (:robcee)]

(In reply to Victor Porof [:vp] from comment #31)
> Btw, Rob, can you please take a glance at how this looks on Windows? The CSS
> looks right to me though.

I'll see if I can find a windows box.

Comment 34

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 654558 [details] [diff] [review]
v3

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

mostly just the repeated typo in debugger-view and the pref consts in debugger-controller.js. Pretty OK. R+ with those.

::: browser/devtools/debugger/debugger-controller.js
@@ +1724,5 @@
> +   * @return boolean
> +   */
> +  get stackframesPaneVisible() {
> +    if (this._sfrmVisible === undefined) {
> +      this._sfrmVisible = Services.prefs.getBoolPref("devtools.debugger.ui.stackframes-pane-visible");

can you break this line up a bit? It's gotten long.

you might put that pref in a const for starters.

also, what does "ssfremVisibol" mean? Use your words! (darnit, there's already a _sfrmWidth in here. I guess it can pass, but I'm leaving the comment for the humor value).

@@ +1734,5 @@
> +   * Sets the preferred stackframes pane visibility state.
> +   * @param boolean value
> +   */
> +  set stackframesPaneVisible(value) {
> +    Services.prefs.setBoolPref("devtools.debugger.ui.stackframes-pane-visible", value);

totally gonna reuse that const here.

@@ +1764,5 @@
> +   * @return boolean
> +   */
> +  get variablesPaneVisible() {
> +    if (this._varsVisible === undefined) {
> +      this._varsVisible = Services.prefs.getBoolPref("devtools.debugger.ui.variables-pane-visible");

use a const plz.

@@ +1774,5 @@
> +   * Sets the preferred variables pane visibility state.
> +   * @param boolean value
> +   */
> +  set variablesPaneVisible(value) {
> +    Services.prefs.setBoolPref("devtools.debugger.ui.variables-pane-visible", value);

const is the new oontz.

::: browser/devtools/debugger/debugger-view.js
@@ +33,5 @@
> +  cacheView: function DV_cacheView() {
> +    this._onTogglePanesButtonPressed = this._onTogglePanesButtonPressed.bind(this);
> +
> +    this._togglePanesButton = document.getElementById("toggle-panes");
> +    this._stackframesAndBrekpoints = document.getElementById("stackframes+breakpoints");

this._stackframesAndBrekpoints -> this._stackframesAndBreakpoints
                                                         ^- AAA

@@ +45,4 @@
>     * Initializes UI properties for all the displayed panes.
>     */
>    initializePanes: function DV_initializePanes() {
> +    this._togglePanesButton.addEventListener("click", this._onTogglePanesButtonPressed);

let's move this into a click handler in the xul document. Seems silly to do dynamically what we can do declaratively.

@@ +46,5 @@
>     */
>    initializePanes: function DV_initializePanes() {
> +    this._togglePanesButton.addEventListener("click", this._onTogglePanesButtonPressed);
> +
> +    this._stackframesAndBrekpoints.setAttribute("width", Prefs.stackframesWidth);

oh no you din't copy and paste that type around here.

@@ +81,5 @@
>    /**
>     * Removes the displayed panes and saves any necessary state.
>     */
>    destroyPanes: function DV_destroyPanes() {
> +    this._togglePanesButton.removeEventListener("click", this._onTogglePanesButtonPressed);

this can go with a proper onclick handler in the xul doc.

@@ +83,5 @@
>     */
>    destroyPanes: function DV_destroyPanes() {
> +    this._togglePanesButton.removeEventListener("click", this._onTogglePanesButtonPressed);
> +
> +    Prefs.stackframesWidth = this._stackframesAndBrekpoints.getAttribute("width");

-__-

@@ +88,5 @@
> +    Prefs.variablesWidth = this._variables.getAttribute("width");
> +
> +    this._breakpoints.parentNode.removeChild(this._breakpoints);
> +    this._stackframes.parentNode.removeChild(this._stackframes);
> +    this._stackframesAndBrekpoints.parentNode.removeChild(this._stackframesAndBrekpoints);

I'm gonna take your editor away from you.

@@ +90,5 @@
> +    this._breakpoints.parentNode.removeChild(this._breakpoints);
> +    this._stackframes.parentNode.removeChild(this._stackframes);
> +    this._stackframesAndBrekpoints.parentNode.removeChild(this._stackframesAndBrekpoints);
> +    this._variables.parentNode.removeChild(this._variables);
> +    this._globalsearch.parentNode.removeChild(this._globalsearch);

this should probably be camelCased since we do that everywhere. globalSearch.

@@ +115,5 @@
> +   * Called when the panes toggle button is clicked.
> +   */
> +  _onTogglePanesButtonPressed: function DV__onTogglePanesButtonPressed(e) {
> +    this.showStackframesAndBreakpointsPane(
> +      !e.target.getAttribute("stackframesAndBrekpointsHidden"), true);

*sob*

@@ +137,5 @@
> +   */
> +  showStackframesAndBreakpointsPane:
> +  function DV_showStackframesAndBreakpointsPane(aVisibleFlag, aAnimatedFlag) {
> +    if (aAnimatedFlag) {
> +      this._stackframesAndBrekpoints.setAttribute("animated", "");

brekpoint. I'm going to cease mentioning this further. It has been fun though I fear it may have become annoying. Sincerely, rob.

@@ +141,5 @@
> +      this._stackframesAndBrekpoints.setAttribute("animated", "");
> +    } else {
> +      this._stackframesAndBrekpoints.removeAttribute("animated");
> +    }
> +    if (aVisibleFlag) {

so this is acting as a toggle right? and aVisibleFlag is referring to whether the panels are visible or not, not "how you want them to be".

i.e., showStackframesAndBreakpointsPane(true, true) would mean "the panels are currently visible, hide them and animate".

@@ +146,5 @@
> +      this._stackframesAndBrekpoints.style.marginLeft = "0";
> +      this._togglePanesButton.setAttribute("stackframesAndBrekpointsHidden", "true");
> +    } else {
> +      let margin = parseInt(this._stackframesAndBrekpoints.getAttribute("width")) + 1;
> +      this._stackframesAndBrekpoints.style.marginLeft = -margin + "px";

wondering if we could do this in CSS through some combination of attribute selectors and the calc function. Worth it? Too silly? I dunno.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

http://i.imgur.com/5HuQJ.png

Comment 36

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #34)
> Comment on attachment 654558 [details] [diff] [review]
> v3
> 
> you might put that pref in a const for starters.

I bet you mean that pref string, not value.

> 
> also, what does "ssfremVisibol" mean? Use your words! (darnit, there's
> already a _sfrmWidth in here. I guess it can pass, but I'm leaving the
> comment for the humor value).
> 

It's a secret keyword. Just don't say it three times in a row.
Really.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +33,5 @@
> > +  cacheView: function DV_cacheView() {
> > +    this._onTogglePanesButtonPressed = this._onTogglePanesButtonPressed.bind(this);
> > +
> > +    this._togglePanesButton = document.getElementById("toggle-panes");
> > +    this._stackframesAndBrekpoints = document.getElementById("stackframes+breakpoints");
> 
> this._stackframesAndBrekpoints -> this._stackframesAndBreakpoints
>                                                          ^- AAA
> 

See, I told you not to say that thing three times in a row. Now you turned into a clang.

> 
> @@ +88,5 @@
> > +    Prefs.variablesWidth = this._variables.getAttribute("width");
> > +
> > +    this._breakpoints.parentNode.removeChild(this._breakpoints);
> > +    this._stackframes.parentNode.removeChild(this._stackframes);
> > +    this._stackframesAndBrekpoints.parentNode.removeChild(this._stackframesAndBrekpoints);
> 
> I'm gonna take your editor away from you.
> 

* disables derp mode *

Comment 37

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

Attachment: v3.1

Addressed comments. Removed borders and box-shadow. Stopped being silly.

Comment 38

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

Attachment: v3.2

Updated test.

Comment 39

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

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

Comment 40

[Dave Camp (:dcamp)]

https://hg.mozilla.org/mozilla-central/rev/350499bc1b85