testing: add initial editor decorations #202048
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the first pass at decorations in-editor. This PR doesn't actually register the contribution, as it's not ready for selfhosting yet. This PR creates decorations that look like this. The idea is that coverage decorations in the glyph margin will always be visibile when there's coverage, and users can get coverage in their code via hover or shortcut, with the intention of making coverage unobtrusive and easy to run all the time.  The notable thing is that there is now a third glyph margin row. I reworked some of the editor code to handle this.  Some open questions: - The glyph margin coverage wants doesn't need to be full-width, should we add a new 'leftmost' glyph lane instead that's thinner? - Adding breakpoints in files with coverage is a little annoying since the breakpoint hint widget can expand the glyph margin on lines with coverage, and jump back over otherwise. Probably we should never decrease the number of lanes shown whenever the cursor is over the glyph margin. 
connor4312
force-pushed
the
connor4312/test-coverage-decorations-1
branch
from
317aa68
to
0e743a2
Compare
chrmarti
approved these changes
Jan 9, 2024
alexdima
approved these changes
Jan 9, 2024
There was a problem hiding this comment.
The changes look good, but I wasn't sure how to test the changes myself? I'm slightly concerned by the jumping around shown in the last gif. Maybe we can also iron that out? Maybe once coverage information is available, the lane count permanently increases for the file.
connor4312
added a commit
that referenced
this pull request
Jan 9, 2024
In my previous PR #202048, I added another glyph margin lane. However I didn't realize in some cases the support I added was incomplete since the rendering code did not fully handle an unbounded number of lanes. Also, I noticed in existing issues with hover and click handling because some parts of VS Code still assume there's at most a single glyph widget visible. It is hard to determine which glyph widget is interacted with without checking what classes are applied to the glyph element, which is what most existing code does. And we probably shouldn't do that in e.g. the MouseHandler to determine what glyph is being clicked on. So in this PR I added a `GlyphMarginLanesModel` that contains the information necessary for rendering. I also want to expose it in a place that the MouseHandler or MouseTarget can get at, but I wasn't sure the best way to do that, any suggestions @alexdima? Implementation-wise I opted for a flag Uint8Array. A 1024 line file with decorations on the last line, it would take 384 bytes to hold decoration positions. I also tried out a Range array based implementation, but I think this is faster (for cases where there are lots of decorations) and much simpler, in exchange for higher memory usage in edge cases (very long files with decorations near the end.)
connor4312
added a commit
that referenced
this pull request
Jan 9, 2024
In my previous PR #202048, I added another glyph margin lane. However I didn't realize in some cases the support I added was incomplete since the rendering code did not fully handle an unbounded number of lanes. Also, I noticed in existing issues with hover and click handling because some parts of VS Code still assume there's at most a single glyph widget visible. It is hard to determine which glyph widget is interacted with without checking what classes are applied to the glyph element, which is what most existing code does. And we probably shouldn't do that in e.g. the MouseHandler to determine what glyph is being clicked on. So in this PR I added a `GlyphMarginLanesModel` that contains the information necessary for rendering. I also want to expose it in a place that the MouseHandler or MouseTarget can get at, but I wasn't sure the best way to do that, any suggestions @alexdima? Implementation-wise I opted for a flag Uint8Array. A 1024 line file with decorations on the last line, it would take 384 bytes to hold decoration positions. I also tried out a Range array based implementation, but I think this is faster (for cases where there are lots of decorations) and much simpler, in exchange for higher memory usage in edge cases (very long files with decorations near the end.)
connor4312
added a commit
that referenced
this pull request
Jan 9, 2024
In my previous PR #202048, I added another glyph margin lane. However I didn't realize in some cases the support I added was incomplete since the rendering code did not fully handle an unbounded number of lanes. Also, I noticed in existing issues with hover and click handling because some parts of VS Code still assume there's at most a single glyph widget visible. It is hard to determine which glyph widget is interacted with without checking what classes are applied to the glyph element, which is what most existing code does. And we probably shouldn't do that in e.g. the MouseHandler to determine what glyph is being clicked on. So in this PR I added a `GlyphMarginLanesModel` that contains the information necessary for rendering. I also want to expose it in a place that the MouseHandler or MouseTarget can get at, but I wasn't sure the best way to do that, any suggestions @alexdima? Implementation-wise I opted for a flag Uint8Array. A 1024 line file with decorations on the last line, it would take 384 bytes to hold decoration positions. I also tried out a Range array based implementation, but I think this is faster (for cases where there are lots of decorations) and much simpler, in exchange for higher memory usage in edge cases (very long files with decorations near the end.)
connor4312
added a commit
that referenced
this pull request
Jan 10, 2024
* monaco: improve handling of glyph margins In my previous PR #202048, I added another glyph margin lane. However I didn't realize in some cases the support I added was incomplete since the rendering code did not fully handle an unbounded number of lanes. Also, I noticed in existing issues with hover and click handling because some parts of VS Code still assume there's at most a single glyph widget visible. It is hard to determine which glyph widget is interacted with without checking what classes are applied to the glyph element, which is what most existing code does. And we probably shouldn't do that in e.g. the MouseHandler to determine what glyph is being clicked on. So in this PR I added a `GlyphMarginLanesModel` that contains the information necessary for rendering. I also want to expose it in a place that the MouseHandler or MouseTarget can get at, but I wasn't sure the best way to do that, any suggestions @alexdima? Implementation-wise I opted for a flag Uint8Array. A 1024 line file with decorations on the last line, it would take 384 bytes to hold decoration positions. I also tried out a Range array based implementation, but I think this is faster (for cases where there are lots of decorations) and much simpler, in exchange for higher memory usage in edge cases (very long files with decorations near the end.) * allow lanes to be persisted to avoid jumping bp icons
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first pass at decorations in-editor. This PR doesn't
actually register the contribution, as it's not ready for selfhosting
yet. This PR creates decorations that look like this. The idea is that
coverage decorations in the glyph margin will always be visibile when
there's coverage, and users can get coverage in their code via hover or
shortcut, with the intention of making coverage unobtrusive and easy to
run all the time.
The notable thing is that there is now a third glyph margin row. I
reworked some of the editor code to handle this.
Some open questions:
The glyph margin coverage wants doesn't need to be full-width, should
we add a new 'leftmost' glyph lane instead that's thinner?
Adding breakpoints in files with coverage is a little annoying since
the breakpoint hint widget can expand the glyph margin on lines with
coverage, and jump back over otherwise. Probably we should never
decrease the number of lanes shown whenever the cursor is over the
glyph margin.