Closed Bug 762979 Opened 10 years ago Closed 6 years ago

Implement shorlander's line gutter mockup for the source editor

Categories

(DevTools :: Source Editor, defect, P2)

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: msucan, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [sourceeditor])

Attachments

(7 files, 12 obsolete files)

183.34 KB, image/png
Details
364.12 KB, image/png
Details
6.91 KB, application/zip
Details
14.68 KB, image/png
Details
13.26 KB, patch
ntim
: review+
Details | Diff | Splinter Review
8.39 KB, patch
ntim
: review+
Details | Diff | Splinter Review
5.34 KB, patch
ntim
: review+
Details | Diff | Splinter Review
We need to implement the mockup shorlander did for the source editor line gutter.

See attachment 611937 [details] from bug 740482.
Moving to Source Editor component.

Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
Blocks: 753301
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Why aren't we doing this? It's much better than what we have now.
I just thought it was out of date enough that it no longer applied. Darrin?
Status: RESOLVED → REOPENED
Flags: needinfo?(dhenein)
Resolution: WONTFIX → ---
No longer blocks: 759351
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).
Flags: needinfo?(dhenein)
Assignee: nobody → gabriel.luong
Attached image 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).
Flags: needinfo?(shorlander)
Flags: needinfo?(dhenein)
Note that those original icons likely need to be updated to the current devtools color themes.
Also, we might want to consider a different breakpoint icon colour for conditional breakpoints.
Attached patch 762979.patch [WIP] (obsolete) — Splinter Review
Flags: needinfo?(mmaslaney)
Attached image Debugger Gutter Updated - 01 (obsolete) —
I updated the mockup to align with the new visual styling and added an option for hover. If this works I can supply assets.
Flags: needinfo?(shorlander)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(dhenein)
I think this looks awesome. +1
Flags: needinfo?(dhenein)
We might want to style conditional breakpoints differently.
Would this also be the same for the light theme?
Flags: needinfo?(shorlander)
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.
Attached image Debugger Gutter Updated - 02 (obsolete) —
Current status: Updated with Light theme version and conditional and normal breakpoints.

Might need a little more work on contrast.
Attachment #8494543 - Attachment is obsolete: true
Inverted the breakpoint text on the light theme. Should fix contrast concerns.
Flags: needinfo?(shorlander)
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.
Flags: needinfo?(gabriel.luong)
(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.
Flags: needinfo?(gabriel.luong)
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.
Flags: needinfo?(gabriel.luong)
Attached patch Patch v1 (obsolete) — Splinter Review
This fully implements shorlander's mockup, except for conditional breakpoints.
Assignee: gabriel.luong → ntim.bugs
Attachment #8472604 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8702121 - Flags: review?(vporof)
Attachment #8702121 - Flags: review?(bgrinstead)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8702121 - Attachment is obsolete: true
Attachment #8702121 - Flags: review?(vporof)
Attachment #8702121 - Flags: review?(bgrinstead)
Attachment #8702125 - Flags: review?(vporof)
Attachment #8702125 - Flags: review?(bgrinstead)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #8702125 - Attachment is obsolete: true
Attachment #8702125 - Flags: review?(vporof)
Attachment #8702125 - Flags: review?(bgrinstead)
Attachment #8702127 - Flags: review?(vporof)
Attachment #8702127 - Flags: review?(bgrinstead)
Attachment #8702128 - Flags: review?(bgrinstead)
Attachment #8702128 - Attachment is obsolete: true
Attachment #8702128 - Flags: review?(bgrinstead)
Attachment #8702127 - Attachment is obsolete: true
Attachment #8702127 - Flags: review?(vporof)
Attachment #8702127 - Flags: review?(bgrinstead)
Flags: needinfo?(gabriel.luong)
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 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 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/
(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 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/
Attachment #8702131 - Attachment is obsolete: true
Attachment #8702131 - Flags: review?(bgrinstead)
(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 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 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/
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 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 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 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 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/
(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
Duplicate of this bug: 1045271
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!
Attachment #8702130 - Flags: review?(vporof) → review+
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.
(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 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/.
Attachment #8702130 - Flags: ui-review?(hholmes)
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.
Attachment #8702130 - Flags: review?(bgrinstead)
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
Attachment #8702155 - Flags: review?(bgrinstead) → review+
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).
Attachment #8702155 - Flags: ui-review?(hholmes)
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #28)

> "There are still the error breakpoints"

What are error breakpoints?
(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.
Attachment #8702130 - Flags: review?(bgrinstead)
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/
Attachment #8702155 - Attachment description: MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins → MozReview Request: Bug 762979 - Implement conditional breakpoint gutter style. r=bgrins, jlongster
Attachment #8702155 - Flags: review?(jlong)
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 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
Attachment #8702130 - Flags: review?(bgrinstead)
Attachment #8704342 - Flags: review?(bgrinstead) → review+
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 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
Attachment #8702155 - Flags: ui-review?(hholmes)
Comment on attachment 8704342 [details]
MozReview Request: Bug 762979 - Update toggle breakpoints icon to match new gutter style. r=bgrins

Resetting the flag
Attachment #8704342 - Flags: ui-review?(hholmes)
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
Attachment #8702155 - Flags: review+
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
Attachment #8702155 - Flags: review?(jlong)
Attachment #8704342 - Flags: ui-review?(hholmes) → ui-review+
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.)
Attachment #8702130 - Flags: ui-review?(hholmes) → feedback+
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/
Attachment #8702130 - Flags: feedback+ → review?(bgrinstead)
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/
Attachment #8702155 - Flags: review?(jlong)
Attachment #8702155 - Flags: review?(bgrinstead)
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 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 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 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/
Attachment #8702130 - Flags: review?(bgrinstead) → review+
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).
Attachment #8702155 - Flags: review?(bgrinstead)
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.
Blocks: 1206430
Attachment #8702155 - Flags: review?(jlong) → review+
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.
Mozreview is now restricted to L3 commit access. Gonna attach the patches using the classic form instead.
Attachment #8702130 - Attachment is obsolete: true
Attachment #8702155 - Attachment is obsolete: true
Attachment #8704342 - Attachment is obsolete: true
Attachment #8713859 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2f6635510c6
https://hg.mozilla.org/mozilla-central/rev/1cba85b380ba
https://hg.mozilla.org/mozilla-central/rev/d8ca40cccd0c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
This was a long time coming, congrats everyone!
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
Keywords: dev-doc-needed
See bug 1245030 to fix those problems
Duplicate of this bug: 812173
Depends on: 1285440
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.