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

RESOLVED FIXED in Firefox 29

Status

P3
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 29
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
yeah, it's an important toggle for sure. +1.
YOU GOT IT NICK!

filter on BLACKEAGLE.
Assignee: nobody → nfitzgerald
Priority: -- → P3
Created attachment 761177 [details] [diff] [review]
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 :)
Attachment #761177 - Flags: feedback?(vporof)
(Assignee)

Comment 4

5 years ago
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.
Attachment #761177 - Flags: feedback?(vporof) → feedback+
Patch bit rotted, and I'm not getting to this in the near future. Feel free to take this, someone.
Assignee: nfitzgerald → nobody
(Assignee)

Comment 6

5 years ago
I'll take a look at this.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
This should be in the sources mini toolbar, intro'd in part 2 of bug 762761.
Depends on: 762761
Summary: Have a more obvious way to disable all breakpoints → Add a enable/disable all breakpoints button to the sources toolbar
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.
(Assignee)

Comment 9

5 years ago
(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.
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.
(Assignee)

Comment 11

5 years ago
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?
(Assignee)

Comment 12

5 years ago
(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
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
(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.
(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.
(Assignee)

Comment 16

5 years ago
Created attachment 8343668 [details] [diff] [review]
dbg-toggle-breakpoints.patch

Just need an icon for the breakpoints toggle button.
Attachment #761177 - Attachment is obsolete: true
Attachment #8343668 - Flags: review?(rcampbell)
(Assignee)

Comment 17

5 years ago
Created attachment 8343670 [details]
icon request: breakpoints toggle button
Attachment #8343670 - Flags: ui-review?(dhenein)
(Assignee)

Comment 18

5 years ago
(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 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)
Attachment #8343668 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Created attachment 8343917 [details] [diff] [review]
v2

Addressed comments. Still need an icon for the button though.
Attachment #8343668 - Attachment is obsolete: true
Attachment #8343917 - Flags: review+
Created attachment 8345474 [details]
debugger-breakpoint-toggle icon
Created attachment 8345475 [details]
debugger-breakpoint-toggle icon @2x
Attachment #8343670 - Flags: ui-review?(dhenein) → ui-review+
(Assignee)

Comment 24

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

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

Comment 26

5 years ago
Created attachment 8345877 [details]
works like this
Attachment #8343670 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 8345878 [details]
looks like this
Attachment #8345878 - Flags: ui-review?(dhenein)
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).
Attachment #8345878 - Flags: ui-review?(dhenein) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/ad1aec75ad50
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8345877 [details]
works like this

This is so mesmerizing to watch.

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.