815280 - Add a enable/disable all breakpoints button to the sources toolbar

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

Just a thought: we currently have the right-click-on-a-breakpoint in the pane and "Disable all breakpoints" in the context menu, but I think it may be a good idea to expose this as a persistent button in the toolbar, since this is a very commonly used toggle imo.

Comment 1

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

yeah, it's an important toggle for sure. +1.

Comment 2

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

YOU GOT IT NICK!

filter on BLACKEAGLE.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip 1

Initial crack at this. Adds buttons to the toolbar to disable all breakpoints and enable all breakpoints. They are only shown if there are breakpoints to enable or disable respectively.

Notes:

* No tests yet.

* Sometimes when only half the breakpoints are enabled, SourcesView.prototype.disableAllBreakpoints throws an error because item is undefined, which makes no sense to me. Any idea what is going on?

* Need icons for the new buttons.

* CSS changes only for OSX; figured it wasn't worth copying the changes over until I got some feedback

* Hiding toolbar buttons makes the css look crappy. Suggestions welcome :)

Comment 4

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

Comment on attachment 761177 [details] [diff] [review]
wip 1

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

Exquisite work so far.

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> 
> * Sometimes when only half the breakpoints are enabled,
> SourcesView.prototype.disableAllBreakpoints throws an error because item is
> undefined, which makes no sense to me. Any idea what is going on?
> 

The _breakpointsCache was entirely retarded. It is removed in bug 823577. Those code paths you touched there are also, IMHO, not tested enough, so I'll write a few more tests to make sure things work as they should. If it's ok with you, I suggest postponing work on this bug until that one lands.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +180,5 @@
> +   * Listener handling the disable all breakpoints button click event.
> +   */
> +  _onDisableAllBreakpointsPressed: function() {
> +    DebuggerView.Sources.disableAllBreakpoints();
> +    this.updateBreakpointButtons();

The updateBreakpointButtons calls here and below may be redundant, because they trigger _showBreakpoint and _hideBreakpoint calls respectively, but I may be wrong.

@@ +195,5 @@
> +  /**
> +   * Shows/hides the disable/enable all breakpoints buttons.
> +   */
> +  updateBreakpointButtons: function() {
> +    let showDisable = !!Object.keys(DebuggerController.Breakpoints.store).length

Nit: I would suggest having a hasBreakpoints method on DebuggerView.Sources, in tandem with hasDisabledBreakpoints.

Also, Semicolon.

::: browser/themes/osx/devtools/debugger.css
@@ +279,5 @@
>  #step-out {
>    list-style-image: url("chrome://browser/skin/devtools/debugger-step-out.png");
>  }
>  
> +.devtools-toolbar > hbox > toolbarbutton {

#debugger-controls > toolbarbutton,
#debugger-breakpoint-controls > toolbarbutton {
 ...
}

@@ +288,5 @@
>    -moz-border-end-width: 1px;
>    outline-offset: -3px;
>  }
>  
> +.devtools-toolbar > hbox > toolbarbutton:last-of-type {

Ditto.

@@ +293,4 @@
>    -moz-border-end-width: 0;
>  }
>  
> +.devtools-toolbar > hbox {

Ditto.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Patch bit rotted, and I'm not getting to this in the near future. Feel free to take this, someone.

Comment 6

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

I'll take a look at this.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This should be in the sources mini toolbar, intro'd in part 2 of bug 762761.

Comment 8

[Brian Grinstead [:bgrins]]

First, I'm glad to see this feature getting added to the toolbar.

Second, I think there is a nice concept of deactivating/reactivating a breakpoint that would be great to use rather than disabling/enabling.  The difference is that if you had 3 breakpoints, and one was disabled:

* When you clicked 'deactivate all', all three would become greyed out.  Then when you clicked 'reactivate all', all three would become active again *with the same enabled/disabled state they had before*.  
* When you clicked 'disabled all', all three become unchecked.  Then when you clicked 'enable all', all three become enabled, and the breakpoint that you previously had unchecked manually is now checked again.

This is maybe a subtle difference in behavior and may warrant a new bug, but I thought I would throw it out there.

Comment 9

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

(In reply to Brian Grinstead [:bgrins] from comment #8)
> 
> * When you clicked 'deactivate all', all three would become greyed out. 
> Then when you clicked 'reactivate all', all three would become active again
> *with the same enabled/disabled state they had before*.  

I like this.

> * When you clicked 'disabled all', all three become unchecked.  Then when
> you clicked 'enable all', all three become enabled, and the breakpoint that
> you previously had unchecked manually is now checked again.
> 

We can keep the item in the context menu for this.

> This is maybe a subtle difference in behavior and may warrant a new bug, but
> I thought I would throw it out there.

Love the idea, I'll do it in this bug.

Comment 10

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

I don't understand the distinction between "active" and "enabled" breakpoints or the use cases where it would be useful, to be honest. Keep in mind that more options with subtle differences increase the cognitive load for the user, which in turn makes it harder to understand and reason about the program behavior. Less is more.

Comment 11

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

Here's what I got from this:

The current behavior is as following. Suppose you have 3 breakpoints:
> [x] first
> [ ] second
> [x] third
The first and third are enabled, the second is disabled. Right clicking and selecting "disable all breakpoints" would yield the following result:
> [ ] first
> [ ] second
> [ ] third
... thus disabling all breakpoints. Right clicking and selecting "enable all breakpoints" would result in this:
> [x] first
> [x] second
> [x] third
... which might not always be helpful, for example when you want to resume execution past all breakpoints, but revert to the previous breakpoint states afterwards.

What Brian suggest is going back to the initial state:
> [x] first
> [x] second
> [ ] third
... via the new button in the toolbar, which would deactivate, not disable all breakpoints. Thus the state is preserved. In many cases, this behavior is more helpful.

However, I do agree with you Panos that a new third state (enabled vs. disabled vs. inactive) might be tricky. For example, when all breakpoints are inactive, if one adds a new breakpoint, is it in an active/enabled state by default? Should we instead disallow adding breakpoints while all breakpoints are inactive?

Comment 12

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

(In reply to Victor Porof [:vp] from comment #11)
> What Brian suggest is going back to the initial state:
> > [x] first
> > [x] second
> > [ ] third

I, of course, mean this instead:
> [x] first
> [ ] second
> [x] third

Comment 13

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> This should be in the sources mini toolbar, intro'd in part 2 of bug 762761.

The trouble with the mini toolbar is that the actions there act per-source, while disabling all breakpoints is done for all sources.

At the very least, it should not be grouped with the [blackbox and prettify] buttons.

Comment 14

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

(In reply to Panos Astithas [:past] from comment #10)
> I don't understand the distinction between "active" and "enabled"
> breakpoints or the use cases where it would be useful, to be honest. Keep in
> mind that more options with subtle differences increase the cognitive load
> for the user, which in turn makes it harder to understand and reason about
> the program behavior. Less is more.

After thinking about this for a while now, I'm leaning towards your opinion as well. Although Brian's idea in comment 8 can potentially be beneficial in many scenarios, it raises a lot of complications (last paragraph in comment 11). Let's discuss more about that in a followup and keep this bug simple.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Victor Porof [:vp] from comment #13)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> it should not be grouped with the [blackbox and prettify]
> buttons.

Exactly.

Comment 16

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

Attachment: dbg-toggle-breakpoints.patch

Just need an icon for the breakpoints toggle button.

Comment 17

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

Attachment: icon request: breakpoints toggle button

Comment 18

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

(In reply to Victor Porof [:vp] from comment #17)
> Created attachment 8343670 [details]
> icon request: breakpoints toggle button

(the icon should be the same format as debugger-blackbox.png in browser/themes/*/devtools)

Comment 19

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

Comment on attachment 8343668 [details] [diff] [review]
dbg-toggle-breakpoints.patch

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

couple of little nits inside. R+.

::: browser/devtools/debugger/debugger-panes.js
@@ +227,5 @@
>    /**
> +   * Returns all breakpoints for all sources.
> +   */
> +  getAllBreakpoints: function(aStore = []) {
> +    return this.getOtherBreakpoints(undefined, aStore);

you could return an empty array here if getOtherBreakpoints is empty.

@@ +419,5 @@
>        .then(this.updateToolbarButtonsState);
>    },
>  
>    /**
> +   * Toggles all breakpoints enabled/disabled.

for all sources.

@@ +423,5 @@
> +   * Toggles all breakpoints enabled/disabled.
> +   */
> +  toggleBreakpoints: function() {
> +    let breakpoints = this.getAllBreakpoints();
> +    let hasBreakpoints = !!breakpoints.length;

you could check that breakpoints.length > 0.

@@ +783,5 @@
>     */
>    _onStopBlackBoxing: function() {
> +    const { source } = this.selectedItem.attachment;
> +
> +    DebuggerController.SourceScripts.blackBox(source, false)

not strictly necessary for this patch but consistent with other methods.

::: browser/devtools/debugger/debugger.xul
@@ +347,5 @@
>                </hbox>
> +              <toolbarbutton id="toggle-breakpoints"
> +                             class="devtools-toolbarbutton"
> +                             tooltiptext="&debuggerUI.sources.toggleBreakpoints;"
> +                             command="toggleBreakpointsCommand"/>

en lieu of a proper icon, we could set the label to something UTFey.

☒

or

◎ with a line through it.

or even

☃

(because snowman)

Comment 20

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> Comment on attachment 8343668 [details] [diff] [review]
> dbg-toggle-breakpoints.patch
> 
> Review of attachment 8343668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> couple of little nits inside. R+.
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +227,5 @@
> >    /**
> > +   * Returns all breakpoints for all sources.
> > +   */
> > +  getAllBreakpoints: function(aStore = []) {
> > +    return this.getOtherBreakpoints(undefined, aStore);
> 
> you could return an empty array here if getOtherBreakpoints is empty.
> 

That already happens.

> @@ +423,5 @@
> > +   * Toggles all breakpoints enabled/disabled.
> > +   */
> > +  toggleBreakpoints: function() {
> > +    let breakpoints = this.getAllBreakpoints();
> > +    let hasBreakpoints = !!breakpoints.length;
> 
> you could check that breakpoints.length > 0.
> 

Ok, that seems more humane.

Comment 21

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

Attachment: v2

Addressed comments. Still need an icon for the button though.

Comment 22

[Darrin Henein [:darrin]]

Attachment: debugger-breakpoint-toggle icon

Comment 23

[Darrin Henein [:darrin]]

Attachment: debugger-breakpoint-toggle icon @2x

Comment 24

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

Thank you!

Comment 25

[Darrin Henein [:darrin]]

(In reply to Victor Porof [:vp] from comment #24)
> Thank you!

Can you attach a screenshot at some point for ui-review? Thanks!

Comment 26

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

Attachment: works like this

Comment 27

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

Attachment: looks like this

Comment 28

[Darrin Henein [:darrin]]

Comment on attachment 8345878 [details]
looks like this

Awesome, thanks! Looks good. I think it's clear that it is a breakpoint related button, though this will become more clear once we get through 753301 (I will provide a new icon at that point to match the arrow style).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad1aec75ad50

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8345877 [details]
works like this

This is so mesmerizing to watch.