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

Render multiple glyph margin decorations #179657

Closed
wants to merge 5 commits into from

Conversation

joyceerhl
Copy link
Collaborator

@joyceerhl joyceerhl commented Apr 11, 2023

Fixes #5923

For #176316, I'd like to render a share link decoration in the gutter if a whole line has been selected. This currently doesn't play nicely with breakpoint decorations so I took a stab at expanding the glyph margin to accommodate rendering multiple decorations.

In future we could also add the ability to associate decoration click targets with specific commands (and also update the testing/breakpoint decorations to handle click events), but I wanted to address just the rendering issue as a first pass.

Recording 2023-04-10 at 20 02 14

@joyceerhl joyceerhl self-assigned this Apr 11, 2023
@joyceerhl joyceerhl requested a review from alexdima April 11, 2023 03:32
@vscodenpa vscodenpa added this to the April 2023 milestone Apr 11, 2023
@alexdima
Copy link
Member

alexdima commented Apr 11, 2023

The call to model.getAllDecorations() from the view model is concerning for me because fetching all the decorations could be very expensive perf-wise. And we will now be doing this almost all the time (each keystroke, etc). To be faster, I think we need to enrich the IntervalTree/IntervalNode and include only decorations that affect the margin straight in the search function and perhaps in the metadata of a node. That would avoid needing to do the offset -> range conversion for all decorations on the model all the time. My hunch is that the vast majority of decorations are not margin decorations.

Could you perhaps explore enriching the IntervalNode.metadata to have a bit flag if the node renders a margin decoration, this can be done similar to getNodeIsForValidation / setNodeIsForValidation. Then, we can enrich the search to be able to include only nodes that render glyph margins, and eventually expose that as a separate method on ITextModel, similar to getAllDecorations, maybe getAllMarginDecorations.

Another thing that might happen is that users might want to opt out of this growing of the margin area, so maybe we need to add an editor option to limit the max glyph margin lane count.

@joyceerhl
Copy link
Collaborator Author

joyceerhl commented Apr 11, 2023

Could you perhaps explore enriching the IntervalNode.metadata to have a bit flag if the node renders a margin decoration, this can be done similar to getNodeIsForValidation / setNodeIsForValidation. Then, we can enrich the search to be able to include only nodes that render glyph margins, and eventually expose that as a separate method on ITextModel, similar to getAllDecorations, maybe getAllMarginDecorations.

👍 Done in c322dd0

Another thing that might happen is that users might want to opt out of this growing of the margin area, so maybe we need to add an editor option to limit the max glyph margin lane count.

I added editor.glyphMarginDecorationsLimit, here is a demo of how it works. When there are more decorations than can be rendered, we stop rendering decorations, which is pretty much the same behavior that we have today since today only one div gets rendered and all the classnames are merged:

glyph-decorations-setting.mp4

@joyceerhl
Copy link
Collaborator Author

From standup today there was some feedback on the approach this PR takes, and I also learned about some prior discussions here. I opened #179725 to get additional feedback on a new proposal and will rework this PR once we have settled on a better approach.

@joyceerhl joyceerhl marked this pull request as draft April 11, 2023 22:24
@joyceerhl
Copy link
Collaborator Author

Superseded by #180013 and #179910

@joyceerhl joyceerhl closed this Apr 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorations with gutter icons hide breakpoint icons
3 participants