Column breakpoints UI #7095
Column breakpoints UI #7095
Conversation
|
Looking Awesome :) |
| let sites; | ||
| if (!callSites) { | ||
| return null; | ||
| } | ||
|
|
||
| const { line } = editor.codeMirror.getCursor(); |
AnshulMalik
Oct 9, 2018
Member
I think getCursor gives us the position of the blinking cursor, and not mouse position, I am not sure though.
I think getCursor gives us the position of the blinking cursor, and not mouse position, I am not sure though.
audreyfang
Oct 12, 2018
Author
Contributor
It does give the position of the blinking cursor, which was my intention for this first iteration. Do you happen to know of any way to get the mouse hover position?
It does give the position of the blinking cursor, which was my intention for this first iteration. Do you happen to know of any way to get the mouse hover position?
AnshulMalik
Oct 12, 2018
•
Member
I did read the docs but couldn't find the mouse position, I think we need to come up with our own way to capture mouse position.
Or may be I missed something, you should check their docs as well :)
I did read the docs but couldn't find the mouse position, I think we need to come up with our own way to capture mouse position.
Or may be I missed something, you should check their docs as well :)
darkwing
Oct 12, 2018
Contributor
Let's table the mouse position experiment for now. Let's just focus on clicking the icon adding the column breakpoint, and then having different states like enabled, disabled, off, as well as dark mode 💯
Let's table the mouse position experiment for now. Let's just focus on clicking the icon adding the column breakpoint, and then having different states like enabled, disabled, off, as well as dark mode
|
Interesting that you've gone the CSS route instead of adding an element as a marker -- I look forward to seeing where this goes. The important test for it is ensuring that clicking in that area of the marker does in fact activate/deactivate the column breakpoint -- CSS can be weird like that sometimes. |
| width: 100%; | ||
| height: calc(100% - 2px); | ||
| border-bottom: 2px solid #aed3ef; | ||
| } | ||
|
|
||
| .call-site-bp { | ||
| position: relative; | ||
| border-bottom: 2px solid red; |
darkwing
Oct 10, 2018
Contributor
I'm guessing this is for debugger?
I'm guessing this is for debugger?
audreyfang
Oct 12, 2018
Author
Contributor
Not completely sure I understand the question, but the red border is the current way of indicating that a breakpoint is selected. I added it here so that both the icon and text continue to be underlined.
Not completely sure I understand the question, but the red border is the current way of indicating that a breakpoint is selected. I added it here so that both the icon and text continue to be underlined.
darkwing
Oct 12, 2018
Contributor
Ahh, I see. We probably want to find a different color, but that's a simple change so don't hang up on it.
Ahh, I see. We probably want to find a different color, but that's a simple change so don't hang up on it.
| @@ -25,8 +26,7 @@ | |||
| } | |||
|
|
|||
| .call-site-bp::before { | |||
| content: ""; | |||
| position: absolute; | |||
| content: ">>"; | |||
darkwing
Oct 10, 2018
Contributor
You should be able to use the same breakpoint svg image as the sidebar.
You should be able to use the same breakpoint svg image as the sidebar.
| location.start.line <= closestFunc.location.end.line | ||
| ); | ||
| } else { | ||
| callSitesFiltered = callSites; |
darkwing
Oct 10, 2018
Contributor
You don't need the else; just initialize callSitesFiltered to callSites:
let callSitesFiltered = callSites;
if ( ....
You don't need the else; just initialize callSitesFiltered to callSites:
let callSitesFiltered = callSites;
if ( ....
|
Wow, this is awesome!
|
|
I pushed a tiny commit to your branch @audreyfang ; please |
|
Update as to what's left here @audreyfang ? |
|
@darkwing I need to make a few minor fixes to the ui colours, look at your suggestions from last week and the failing tests. I should have another commit up later tonight. |
|
@darkwing Do you have a specific example for when clicking on a breakpoint doesn't remove it? It works for me in most cases, although I do seem to have some issues with one particular function in the file I'm using. |
| @@ -20574,7 +20574,8 @@ | |||
| "../../parse5/lib/serializer/serializer_stream.js": 1752, | |||
darkwing
Oct 23, 2018
Contributor
This doesn't belong in this PR, we'll remove it before merging.
This doesn't belong in this PR, we'll remove it before merging.
| @@ -50,7 +50,7 @@ pref("devtools.debugger.skip-pausing", false); | |||
| pref("devtools.debugger.features.wasm", true); | |||
| pref("devtools.debugger.features.shortcuts", true); | |||
| pref("devtools.debugger.features.root", true); | |||
| pref("devtools.debugger.features.column-breakpoints", false); | |||
| pref("devtools.debugger.features.column-breakpoints", true); | |||
darkwing
Oct 23, 2018
Contributor
👍
darkwing
Oct 23, 2018
Contributor
Actually, as things stand, we'll want to change this back to false before merging. I just pressed option in a minified file and my browser hung, so we'll need to measure performance implications here. Keep it true for now, if only for the sake of everyone testing and tests running.
Actually, as things stand, we'll want to change this back to false before merging. I just pressed option in a minified file and my browser hung, so we'll need to measure performance implications here. Keep it true for now, if only for the sake of everyone testing and tests running.
|
OMG I love how this looks so far. Awesome awesome awesome! I'm seeing a really weird issue and I'm wondering if @loganfsmyth or @AnshulMalik could help. I prettified the following URL during testing: https://davidwalsh.name/wp-content/themes/punky/js/main.js I press option and see a bunch of lovely column breakpoint locations (yay!). When I click one, however, the following issues occur:
@loganfsmyth Can you look into the breakpoint location issue? It's acting a bit odd. @audreyfang In the mean time, could you explore adding mochitest for this functionality? i.e. clicking a column breakpoint on and off does what's expected? I expect you'll probably have questions here which we can discuss in Slack or 1:1 video. Great work so far! |
|
It's the issue with source-map, either the source map generated from prettifying is not right(I think this is the case). Or |
|
@audreyfang I think that this is far enough along that we can merge and iterate on; i.e. the styling is awesome. Are there any blockers to merging this UI change? |
|
@darkwing Will the column breakpoints remain enabled by default? If so, I need to update the jest tests to reflect that. If not, then this should be fine to merge. |
|
@audreyfang I think you'll only need to change the pref in |
Summary of Changes
Very rough start to the column breakpoint changes @darkwing and I discussed earlier. When alt/option is pressed column breakpoints are shown only for the function where the cursor is currently located.
This is still quite buggy but works in some cases and I will also be further looking into changing the >> to actual icons.
Screenshots/Videos