Implement shorlander's line gutter mockup for the source editor

RESOLVED FIXED in Firefox 47

Status

DevTools
Source Editor
P2
normal
RESOLVED FIXED
6 years ago
5 days ago

People

(Reporter: msucan, Assigned: ntim)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Trunk
Firefox 47
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [sourceeditor])

Attachments

(7 attachments, 12 obsolete attachments)

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
(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Blocks: 753301
No longer blocks: 788165
Status: NEW → RESOLVED
Last Resolved: 4 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)

Updated

4 years ago
Assignee: nobody → gabriel.luong
Created attachment 8467314 [details]
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).
Nice!

Updated

4 years ago
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.
Created attachment 8472604 [details] [diff] [review]
762979.patch [WIP]

Updated

4 years ago
Flags: needinfo?(mmaslaney)
Created attachment 8494543 [details]
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.
Flags: needinfo?(shorlander)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(dhenein)
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?

Updated

4 years ago
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.
Created attachment 8538874 [details]
Debugger Gutter Updated - 02

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
Created attachment 8557242 [details]
Debugger Gutter Updated - 03

Inverted the breakpoint text on the light theme. Should fix contrast concerns.
Flags: needinfo?(shorlander)
Attachment #8538874 - Attachment is obsolete: true
Created attachment 8557243 [details]
Breakpoint Arrow Asset - 01

Comment 18

3 years ago
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.

Updated

3 years ago
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)

Comment 20

3 years ago
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)
Created attachment 8626313 [details]
breakpoint-icons-svg.zip

Converted breakpoint icons to SVG
Attachment #8557243 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8702121 [details] [diff] [review]
Patch v1

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)
(Assignee)

Comment 23

3 years ago
Created attachment 8702125 [details] [diff] [review]
Patch v1.1
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)
(Assignee)

Comment 24

3 years ago
Created attachment 8702127 [details] [diff] [review]
Patch v1.2
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)
(Assignee)

Comment 25

3 years ago
Created attachment 8702128 [details] [diff] [review]
Followup patch: update toggle breakpoint icon
Attachment #8702128 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8702128 - Attachment is obsolete: true
Attachment #8702128 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8702127 - Attachment is obsolete: true
Attachment #8702127 - Flags: review?(vporof)
Attachment #8702127 - Flags: review?(bgrinstead)
(Assignee)

Comment 26

3 years ago
Created attachment 8702130 [details]
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/
Attachment #8702130 - Flags: review?(vporof)
Attachment #8702130 - Flags: review?(bgrinstead)
(Assignee)

Comment 27

3 years ago
Created attachment 8702131 [details]
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/
Attachment #8702131 - Flags: review?(bgrinstead)

Updated

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

Comment 29

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

Comment 30

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

Comment 31

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

Comment 32

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

Updated

3 years ago
Attachment #8702131 - Attachment is obsolete: true
Attachment #8702131 - Flags: review?(bgrinstead)
(Assignee)

Comment 33

3 years ago
Created attachment 8702155 [details]
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/
Attachment #8702155 - Flags: review?(bgrinstead)
(Assignee)

Comment 34

3 years ago
Try push (Thank you Otto Länd!) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7568bec83f00
(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.
(Assignee)

Comment 36

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

Comment 37

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

Comment 39

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

Comment 40

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

Comment 41

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

Comment 42

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

Comment 43

3 years ago
(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

Updated

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

Comment 47

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

Comment 53

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

Updated

3 years ago
Attachment #8702130 - Flags: review?(bgrinstead)
(Assignee)

Comment 54

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

Updated

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

Comment 55

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

Comment 56

3 years ago
Created attachment 8704342 [details]
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/
Attachment #8704342 - 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

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+
(Assignee)

Comment 65

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

Comment 66

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

Comment 67

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

Comment 68

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

Comment 69

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

Comment 70

2 years ago
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.

Updated

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

Comment 74

2 years ago
Created attachment 8713859 [details] [diff] [review]
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.
Attachment #8702130 - Attachment is obsolete: true
Attachment #8702155 - Attachment is obsolete: true
Attachment #8704342 - Attachment is obsolete: true
Attachment #8713859 - Flags: review+
(Assignee)

Comment 75

2 years ago
Created attachment 8713860 [details] [diff] [review]
Part 2 - Implement conditional gutter style (r=jlongster)
Attachment #8713860 - Flags: review+
(Assignee)

Comment 76

2 years ago
Created attachment 8713862 [details] [diff] [review]
Part 3 - Update the toggle breakpoints icon to reflect new breakpoint style (r=bgrins)
Attachment #8713862 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 78

2 years ago
bugherder
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
Last Resolved: 4 years ago2 years ago
status-firefox47: --- → fixed
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

Updated

2 years ago
Depends on: 1285440

Updated

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