762979 - Implement shorlander's line gutter mockup for the source editor

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P2)

Description

[Mihai Sucan [:msucan]]

We need to implement the mockup shorlander did for the source editor line gutter.

See attachment 611937 [details] from bug 740482.

Comment 1

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

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 2

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

Why aren't we doing this? It's much better than what we have now.

Comment 3

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

I just thought it was out of date enough that it no longer applied. Darrin?

Comment 4

[Darrin Henein [:darrin]]

Hmm... I was still hoping to do something like this actually. We may adjust the assets to match the new themes but the feature is still valid IMO.

Right now our breakpoint marker and current/selected line marker overlap, and the breakpoint marker isn't in line with the commonly used paradigm (arrow/box like the mockup).

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Prototype Line Gutter

As a prototype, I used the line gutter icon from chrome to illustrate how this might look in the debugger source editor. In bug 1045271, we are also looking to add a line hover when the cursor is hovering over the line number gutters (See line 47 in screenshot). 

To implement shorlander's line gutter, I need the updated icons for the line gutter and line hover styles (this might highlight the line being debugged, and the line that the breakpoint would apply to).

Comment 6

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

Nice!

Comment 7

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

Note that those original icons likely need to be updated to the current devtools color themes.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Also, we might want to consider a different breakpoint icon colour for conditional breakpoints.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 762979.patch [WIP]

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: Debugger Gutter Updated - 01

I updated the mockup to align with the new visual styling and added an option for hover. If this works I can supply assets.

Comment 11

[Darrin Henein [:darrin]]

I think this looks awesome. +1

Comment 12

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

We might want to style conditional breakpoints differently.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Would this also be the same for the light theme?

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

I think the conditional breakpoint could be the orange as we see in the mockup and the active breakpoint would be green. Just thinking about the traffic light colours orange/yellow would signal a warning, and green would be go.

Comment 15

[Stephen Horlander [:shorlander]]

Attachment: Debugger Gutter Updated - 02

Current status: Updated with Light theme version and conditional and normal breakpoints.

Might need a little more work on contrast.

Comment 16

[Stephen Horlander [:shorlander]]

Attachment: Debugger Gutter Updated - 03

Inverted the breakpoint text on the light theme. Should fix contrast concerns.

Comment 17

[Stephen Horlander [:shorlander]]

Attachment: Breakpoint Arrow Asset - 01

Comment 18

[Zimon Dai [:zimonD]]

From bug983469

We may give conditional breakpoint which throws exceptions a different color (red maybe)

Also the condition popup should be opened when the debuger reaches this breakpoint and caught an exception.

Looking forward for suggestions, I could work on a patch for condition evaluation and error throwing.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Zimon Dai [:zimonD] from comment #18)
> From bug983469
> 
> We may give conditional breakpoint which throws exceptions a different color
> (red maybe)
> 
> Also the condition popup should be opened when the debuger reaches this
> breakpoint and caught an exception.
> 
> Looking forward for suggestions, I could work on a patch for condition
> evaluation and error throwing.

As Nick said in https://bugzilla.mozilla.org/show_bug.cgi?id=983469#c6, let's hold off with the breakpoint UI changes until the initial patch lands. Judging from the patch I suspect it still requires test cases. I do like the idea of having the condition appearing and having some way to indicate that. I would recommending filing a follow up bug to 983469 to see what others think about the changes and get any additional feedback.

Comment 20

[Zimon Dai [:zimonD]]

Hey Gabriel, as bug983469 is almost resolved and landing, maybe it's time to discuss about BP UI changes. Here's bug 1135435 for that. It would be nice if conditional BP UI changes could be considered in this new mockup.

Comment 21

[Stephen Horlander [:shorlander]]

Attachment: breakpoint-icons-svg.zip

Converted breakpoint icons to SVG

Comment 22

[Tim Nguyen :ntim]

Attachment: Patch v1

This fully implements shorlander's mockup, except for conditional breakpoints.

Comment 23

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Comment 24

[Tim Nguyen :ntim]

Attachment: Patch v1.2

Comment 25

[Tim Nguyen :ntim]

Attachment: Followup patch: update toggle breakpoint icon

Comment 26

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review commit: https://reviewboard.mozilla.org/r/29069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29069/

Comment 27

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Review commit: https://reviewboard.mozilla.org/r/29071/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29071/

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Hey Tim,

Thanks for finishing up my patch. Overall, it looks good. There are still the error breakpoints that needs to be added (on top of the conditional breakpoints) and then we can remove that weird 16px width remaining on the left of the line gutter. I never got the assets for the 2 I mentioned so that's why this bug was always in limbo. It would be worthwhile to bug helen to make sure the current breakpoint assets and colours fit the debugger redesign.

Comment 29

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/1-2/

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 8702131 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29071/diff/1-2/

Comment 31

[Tim Nguyen :ntim]

(In reply to Gabriel Luong [:gl] (ΦωΦ） from comment #28)
> Hey Tim,
> 
> Thanks for finishing up my patch. Overall, it looks good. There are still
> the error breakpoints that needs to be added (on top of the conditional
> breakpoints).
Seems like error breakpoints don't exist the debugger, or maybe that's what mentioned comment 20. The error gutter only seems used by the shader editor for now.

Anyway, I've added conditional breakpoints in the latest patch.

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/2-3/

Comment 33

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review commit: https://reviewboard.mozilla.org/r/29073/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29073/

Comment 34

[Tim Nguyen :ntim]

Try push (Thank you Otto Länd!) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7568bec83f00

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Tim Nguyen [:ntim] from comment #31)
> (In reply to Gabriel Luong [:gl] (ΦωΦ） from comment #28)
> > Hey Tim,
> > 
> > Thanks for finishing up my patch. Overall, it looks good. There are still
> > the error breakpoints that needs to be added (on top of the conditional
> > breakpoints).
> Seems like error breakpoints don't exist the debugger, or maybe that's what
> mentioned comment 20. The error gutter only seems used by the shader editor
> for now.
> 
> Anyway, I've added conditional breakpoints in the latest patch.

Cool, it seems like everything got added. I think we can remove the 16px width to the left of the line numbers of the debugger.

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/3-4/

Comment 37

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/1-2/

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

https://reviewboard.mozilla.org/r/29069/#review25889

::: devtools/client/debugger/content/reducers/breakpoints.js:124
(Diff revision 4)
> +    if (action.condition !== "") {

This can be simplified to:
if (action.condition)

"" is always false.

::: devtools/client/sourceeditor/debugger.js:154
(Diff revision 4)
> +    if (cond && cond !== "") {

Same as previous comment. It can be simplified to:
if (cond)

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/4-5/

Comment 40

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/2-3/

Comment 41

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/5-6/

Comment 42

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/3-4/

Comment 43

[Tim Nguyen :ntim]

(In reply to Gabriel Luong [:gl] (ΦωΦ） from comment #38)
> https://reviewboard.mozilla.org/r/29069/#review25889
> 
> ::: devtools/client/debugger/content/reducers/breakpoints.js:124
> (Diff revision 4)
> > +    if (action.condition !== "") {
> 
> This can be simplified to:
> if (action.condition)
> 
> "" is always false.
> 
> ::: devtools/client/sourceeditor/debugger.js:154
> (Diff revision 4)
> > +    if (cond && cond !== "") {
> 
> Same as previous comment. It can be simplified to:
> if (cond)

Thanks ! addressed your comments

Comment 45

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

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

https://reviewboard.mozilla.org/r/29069/#review26123

beautiful patch, very clean!

Comment 46

[Gabriel Luong [:gl] (ΦωΦ)]

https://reviewboard.mozilla.org/r/29069/#review26125

::: devtools/client/sourceeditor/codemirror/mozilla.css:54
(Diff revision 6)
> -  background-image: url("chrome://devtools/skin/images/editor-breakpoint.png");
> +  background-size: calc(100% - 2px) 12px;

Can we do better than calculating the size of the line number? AFAIK we still have an unused gutter next to the line number that can probably be removed in the debugger, but kept in the shader editor.

Comment 47

[Tim Nguyen :ntim]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #46)
> https://reviewboard.mozilla.org/r/29069/#review26125
> 
> ::: devtools/client/sourceeditor/codemirror/mozilla.css:54
> (Diff revision 6)
> > -  background-image: url("chrome://devtools/skin/images/editor-breakpoint.png");
> > +  background-size: calc(100% - 2px) 12px;
> 
> Can we do better than calculating the size of the line number? 
100% is the entire gutter width (not the size of the line number). 2px is just to give it some spacing on the left.
12px is the height of the image.

I could use 100% instead of calc(100% - 2px), but I think it doesn't look very nice to have the marker sticking with the left of the editor.

I could also use border-image, but it really just makes things more complicated (I'd have to set the border-width, which changes the layout, make sure it doesn't look too stretched, ...).

If you have a better solution, I'd be glad to use it though.

> AFAIK we still have an unused gutter next to the line number that can probably be
> removed in the debugger, but kept in the shader editor.
Doesn't this patch remove it ? (It does for me locally).

The only spacing left should be the spacing used to compensate long numbers.
___1|
___2|
...
___9|
__10|
...
1000|

Comment 48

[Brian Grinstead [:bgrins]]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Helen, can you please do a ux review of these changes to breakpoint styling in the debugger?  You can download a patch with all of the changes here: https://reviewboard.mozilla.org/r/29067/diff/raw/.

Comment 49

[Brian Grinstead [:bgrins]]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

https://reviewboard.mozilla.org/r/29069/#review26235

Can you please split this into two patches where the first does the base restyling and the second adds styles for conditional breakpoints?  Sorry, I know it's extra work but it would make this easier to do a code and ux review as they are two separate logical chunks of work that could be landed independantly.  No need to split the assets up - all of the states could be added in the first patch.

My 2 cents about the UX - I don't really like the grey breakpoint effect that happens when hovering a line.  It confuses me especially when disabling an existing breakpoint, or adding a new one that gets slid.  It looks sort of like a disabled breakpoint, and since we already have a line hover effect it's not adding a lot of value to me.

Comment 50

[Brian Grinstead [:bgrins]]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

https://reviewboard.mozilla.org/r/29073/#review26239

Look goods, thanks

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Also flagging this one for ux review.  The patch linked from Comment 48 should also include this change (which is an icon swap for the 'disable all breakpoints' button at the bottom of the debugger UI).

Comment 52

[James Long (:jlongster)]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #28)

> "There are still the error breakpoints"

What are error breakpoints?

Comment 53

[Tim Nguyen :ntim]

(In reply to James Long (:jlongster) from comment #52)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #28)
> 
> > "There are still the error breakpoints"
> 
> What are error breakpoints?
See comment 35, :gl was referring to whatever functionality that used error.png as marker (that's what he told me on IRC).

(In reply to Brian Grinstead [:bgrins] from comment #49)
> Comment on attachment 8702130 [details]
> MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup
> for the source editor. r=vporof, bgrins
> 
> https://reviewboard.mozilla.org/r/29069/#review26235
> 
> Can you please split this into two patches where the first does the base
> restyling and the second adds styles for conditional breakpoints?  Sorry, I
> know it's extra work but it would make this easier to do a code and ux
> review as they are two separate logical chunks of work that could be landed
> independantly.  No need to split the assets up - all of the states could be
> added in the first patch.
Sure, I should be able to find some time this week.

> My 2 cents about the UX - I don't really like the grey breakpoint effect
> that happens when hovering a line.  It confuses me especially when disabling
> an existing breakpoint, or adding a new one that gets slid.  It looks sort
> of like a disabled breakpoint, and since we already have a line hover effect
> it's not adding a lot of value to me.
Note that the line hover effect was added by this patch and was not present before.

Comment 54

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/6-7/

Comment 55

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/4-5/

Comment 56

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Review commit: https://reviewboard.mozilla.org/r/29637/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29637/

Comment 57

[Brian Grinstead [:bgrins]]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

https://reviewboard.mozilla.org/r/29069/#review26465

Thanks for splitting this up!

::: devtools/client/sourceeditor/codemirror/mozilla.css:65
(Diff revision 7)
> -  transition: transform .25s;
> +  background-image: var(--breakpoint-hover-background);

Again, not a fan of this hover breakpoint image showing up (nor the text color changing).  We should wait for more UX feedback but I wouldn't be opposed to pre-emptively removing it (this color could eventually even be used for a disabled breakpoint icon if/when we add that)

::: devtools/client/sourceeditor/debugger.js:228
(Diff revision 7)
> -  ed.addMarker(line, "breakpoints", "debugLocation");
> +  ed.addLineClass(line, "debugLocation");

Any reason for going with debugLocation as the name here?  It causes a lot of noise in the diff and `debug-line` seems perfectly descriptive since it's a lineClass

::: devtools/client/sourceeditor/debugger.js:282
(Diff revision 7)
> -  moveBreakpoint, getBreakpoints, setDebugLocation, getDebugLocation,
> +  setBreakpointCondition, removeBreakpointCondition, getBreakpoints,

With just this patch applied, getting a `ReferenceError: setBreakpointCondition is not defined`, so it and removeBreakpointCondition should be moved into part 2

Comment 58

[Brian Grinstead [:bgrins]]

Comment on attachment 8704342 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

https://reviewboard.mozilla.org/r/29637/#review26469

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

looks like mozreview messed up which patch had the ui-review flag

Comment 60

[Brian Grinstead [:bgrins]]

Comment on attachment 8704342 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Resetting the flag

Comment 61

[Brian Grinstead [:bgrins]]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Looks like mozreview messed up the review flag.  Clearing my request, although see Comment 57 about moving setBreakpointCondition and removeBreakpointCondition into this patch

Comment 62

[James Long (:jlongster)]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

https://reviewboard.mozilla.org/r/29073/#review27097

The approach generally looks good, but needs a little restructuring in the reducer. r? me again with the new patch.

::: devtools/client/debugger/content/reducers/breakpoints.js:129
(Diff revision 5)
> +    }

This blocks should live in one of the `if` blocks above. Otherwise this is being run on every single async status update: it will emit this once when the request starts, and then again once it's done or there was an error. This is especially faulty if there was an error, as you can see in that case above we remove the breakpoint.

Please move this within the ` if (action.status === 'start') {` block so that we only run it when the async request starts. Also, only emit a single type of event: `breakpoint-condition-updated`, and the view can check if the condition exists or not and apply the right style. No need to have two different events.

::: devtools/client/debugger/debugger-view.js:389
(Diff revision 5)
> +  },

See the comment in the reducer. Collapse these into a single function (something like `renderBreakpointCondition`) that is called from a single reducer event. And check if the condition exists on the breakpoint, and call the appropriate add/remove function on the editor.

I try to be as declarative as possible here. We're going to be replacing this view code with React and then we don't need any of these "reducer event" functions, but it'll be easier to transition if we have fewer functions that render based on the current state

Comment 63

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: different-blues.png

Comment 64

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

The gutter spacing notwithstanding (we'd chatted about how the try-build I looked at was a little old over IRC), this looks really good to me!

I did have a small question about the blues (see attachment different-blues.png). Seems like the breakpoints are using a slightly different colored blue than the highlight color we're using everywhere else; is this just because of the background it's on, or was this intentional? (I actually personally like the lighter blue you're using, ntim, and think it would potentially make a nicer-looking highlight color everywhere else, but wondering if we should also stay consistent.)

Comment 65

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/7-8/

Comment 66

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/5-6/

Comment 67

[Tim Nguyen :ntim]

Comment on attachment 8704342 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29637/diff/1-2/

Comment 68

[Tim Nguyen :ntim]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29069/diff/8-9/

Comment 69

[Tim Nguyen :ntim]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29073/diff/6-7/

Comment 70

[Tim Nguyen :ntim]

Comment on attachment 8704342 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29637/diff/2-3/

Comment 71

[Brian Grinstead [:bgrins]]

Comment on attachment 8702130 [details]
MozReview Request: Bug 762979 - Implement shorlander's line gutter mockup for the source editor. r=vporof, bgrins

https://reviewboard.mozilla.org/r/29069/#review28767

Looks good to me - I'd suggest we wait to land until next week after the merge day (so we'll have time to address any follow ups).

Comment 72

[Brian Grinstead [:bgrins]]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

https://reviewboard.mozilla.org/r/29073/#review28769

Going to let James review this one (although see the comment below)

::: devtools/client/sourceeditor/codemirror/mozilla.css:69
(Diff revision 7)
> +.breakpoint.conditional .CodeMirror-linenumber {

I'm guessing the conditional color shouldn't end up overriding the active color (which it does due to class specificity here).  In other words when I'm paused at a conditional bp I'd expect it and the line to be the green active color.

Comment 73

[James Long (:jlongster)]

Comment on attachment 8702155 [details]
MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster

https://reviewboard.mozilla.org/r/29073/#review29695

Looks good, with one tweak! Thanks for doing this!

::: devtools/client/debugger/content/reducers/breakpoints.js:106
(Diff revision 7)
> -      return mergeIn(state, ['breakpoints', id], {
> +      state = mergeIn(state, ['breakpoints', id], {

You don't need to change the `return`s. Just put the `emitChange("breakpoint-condition-updated", bp);` above all of this (and after `const bp =...`). The UI is not notified of changes until all reducers are run; it does not immediately take affect. This is a super important (and beneficial!) aspect of all of this. That way, the UI atomically renders and we know it's always in sync.

Comment 74

[Tim Nguyen :ntim]

Attachment: Part 1 - Implement shorlander's line gutter mockup (r=bgrins, vporof)

Mozreview is now restricted to L3 commit access. Gonna attach the patches using the classic form instead.

Comment 75

[Tim Nguyen :ntim]

Attachment: Part 2 - Implement conditional gutter style (r=jlongster)

Comment 76

[Tim Nguyen :ntim]

Attachment: Part 3 - Update the toggle breakpoints icon to reflect new breakpoint style (r=bgrins)

Comment 77

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f2f6635510c6
https://hg.mozilla.org/integration/fx-team/rev/1cba85b380ba
https://hg.mozilla.org/integration/fx-team/rev/d8ca40cccd0c

Comment 78

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f2f6635510c6
https://hg.mozilla.org/mozilla-central/rev/1cba85b380ba
https://hg.mozilla.org/mozilla-central/rev/d8ca40cccd0c

Comment 79

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

This was a long time coming, congrats everyone!

Comment 80

[Sebastian Zartner [:sebo]]

I generally like the new breakpoints UI. Though there are some UX issues with it:

- It's now impossible to see in the gutter whether a breakpoint is set for a line when the execution pointer stands at that line.
- Conditional breakpoints color the line's background, normal ones don't making the UI inconsistent.
- Deactivated breakpoints are still missing from the gutter, making it hard to see where they are placed.

Sebastian

Comment 81

[James Long (:jlongster)]

See bug 1245030 to fix those problems