New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Column breakpoints UI #7095

Merged
merged 8 commits into from Oct 25, 2018

Conversation

Projects
None yet
3 participants
@audreyfang
Contributor

audreyfang commented Oct 8, 2018

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

column-breakpoint

@darkwing darkwing added the pr-wip label Oct 9, 2018

@AnshulMalik

Looking Awesome :)

let sites;
if (!callSites) {
return null;
}
const { line } = editor.codeMirror.getCursor();

This comment has been minimized.

@AnshulMalik

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.

This comment has been minimized.

@audreyfang

audreyfang Oct 12, 2018

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?

This comment has been minimized.

@AnshulMalik

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 :)

This comment has been minimized.

@darkwing

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 💯

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 9, 2018

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;

This comment has been minimized.

@darkwing

darkwing Oct 10, 2018

Contributor

I'm guessing this is for debugger?

This comment has been minimized.

@audreyfang

audreyfang Oct 12, 2018

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.

This comment has been minimized.

@darkwing

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.

@@ -25,8 +26,7 @@
}
.call-site-bp::before {
content: "";
position: absolute;
content: ">>";

This comment has been minimized.

@darkwing

darkwing Oct 10, 2018

Contributor

You should be able to use the same breakpoint svg image as the sidebar.

location.start.line <= closestFunc.location.end.line
);
} else {
callSitesFiltered = callSites;

This comment has been minimized.

@darkwing

darkwing Oct 10, 2018

Contributor

You don't need the else; just initialize callSitesFiltered to callSites:

let callSitesFiltered = callSites;
if ( ....
@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 17, 2018

Wow, this is awesome! 👍 A few ideas:

  1. Let's add cursor: pointer when the user hovers over a column breakpoint in the editor; the pointer tells the user that a click is actionable.

  2. I'm noticing that clicking on a function that was already clicked doesn't remove the the breakpoint; you probably know that but just an FYI that we need that changed.

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 17, 2018

I pushed a tiny commit to your branch @audreyfang ; please git pull so you have it :)

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 22, 2018

Update as to what's left here @audreyfang ?

@audreyfang

This comment has been minimized.

Contributor

audreyfang commented Oct 22, 2018

@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.

@audreyfang

This comment has been minimized.

Contributor

audreyfang commented Oct 23, 2018

@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,

This comment has been minimized.

@darkwing

darkwing Oct 23, 2018

Contributor

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);

This comment has been minimized.

@darkwing

darkwing Oct 23, 2018

Contributor

👍

This comment has been minimized.

@darkwing

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.

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 23, 2018

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:

  1. A breakpoint is added at the next line
  2. The column's arrow very briefly stays dark (yay!) but then goes back to inactive state (the root cause is probably the same as 1)
  3. I can't add multiple column breakpoints for the same line; instead the existing breakpoint's location is changed.

@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!

@AnshulMalik

This comment has been minimized.

Member

AnshulMalik commented Oct 23, 2018

It's the issue with source-map, either the source map generated from prettifying is not right(I think this is the case). Or source-map package is producing wrong results for generatedLocation.

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 24, 2018

@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?

@audreyfang

This comment has been minimized.

Contributor

audreyfang commented Oct 24, 2018

@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.

@darkwing darkwing changed the title from WIP: Column breakpoints to Column breakpoints UI Oct 24, 2018

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 24, 2018

@audreyfang I think you'll only need to change the pref in assets/panel/prefs.js; I think the other prefs file only controls the development environment (launchpad) preference value. Also, please reset the assets/module-manifest.json file. I think we can merge after that 👍

@darkwing darkwing merged commit 363f48c into devtools-html:master Oct 25, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details

darkwing added a commit that referenced this pull request Oct 26, 2018

darkwing added a commit that referenced this pull request Oct 26, 2018

darkwing added a commit to darkwing/debugger.html that referenced this pull request Oct 26, 2018

darkwing added a commit that referenced this pull request Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment