Skip to content
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

Implement GlyphMarginWidgets #184375

Merged
merged 8 commits into from Jun 16, 2023
Merged

Implement GlyphMarginWidgets #184375

merged 8 commits into from Jun 16, 2023

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Jun 6, 2023

Fixes #114776
Fixes #183400
Part of #179725

Test adoption for the arrow revert widget (not checked in with this PR):

glyph-widgets.mp4

@joyceerhl joyceerhl self-assigned this Jun 6, 2023
@joyceerhl joyceerhl requested a review from alexdima June 6, 2023 23:51
@joyceerhl joyceerhl marked this pull request as ready for review June 6, 2023 23:51
@VSCodeTriageBot VSCodeTriageBot added this to the June 2023 milestone Jun 6, 2023
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good and in the right direction! I pushed a change to do the lane count computation only once per frame. I also left a couple of comments, plus there are a couple of problems that I've spotted for which I didn't add comments, as I didn't digg deeper to see what are the root causes:

  • enable "typescript.referencesCodeLens.enabled": true
  • observe how the breakpoints margins are rendered one line above than before
  • observe also that sometimes the breakpoints cannot be removed:
Kapture.2023-06-13.at.15.04.55.mp4

src/vs/editor/browser/viewParts/glyphMargin/glyphMargin.ts Outdated Show resolved Hide resolved
src/vs/editor/browser/editorBrowser.ts Outdated Show resolved Hide resolved
@joyceerhl
Copy link
Contributor Author

Looked into the off-by-one issue some, it looks like whitespace added by codelenses, as well as the peek view widgets, are not included in the calculation for getVerticalOffsetForLineNumber even when accounting for viewzones:

renderedWidget.domNode.setTop(ctx.getVerticalOffsetForLineNumber(renderedWidget.lineNumber, true));

I probably need a different way to calculate the top offset for each decoration, will do some digging

@joyceerhl joyceerhl requested a review from alexdima June 15, 2023 22:40
@joyceerhl
Copy link
Contributor Author

Fixed!

glyph-widgets-fix.mp4

@joyceerhl joyceerhl enabled auto-merge (squash) June 15, 2023 22:42
@joyceerhl joyceerhl merged commit 26a530f into main Jun 16, 2023
6 checks passed
@joyceerhl joyceerhl deleted the dev/joyceerhl/youngest-crayfish branch June 16, 2023 13:18
@github-actions github-actions bot locked and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants