Support multiple decorations in editor glyph margin

[joyceerhl]

In order to support share link decorations in the glyph margin for my share contribution proposal, I started down the path of reconciling multiple decorations with #179657. I missed an existing issue with discussions #114776, which Connor pointed me to in standup today. I don't think my PR can be merged as is, from standup feedback, there are two main concerns with the approach I took, which does the naive thing of expanding the glyph margin to accommodate decorations:

1. Anything more than 3 decorations in the glyph margin starts to look unreasonable
2. The current behavior doesn't render decorations at predictable offsets, which impacts muscle memory e.g. for setting breakpoints (and begs for some kind of 'lane' treatment per decoration type)

Therefore I'm opening this issue to discuss the approach we take, which should address the following issues (I'm creating a new issue to avoid mass-pinging all existing issue subscribers):

• Decorations with gutter icons hide breakpoint icons #5923
• Introduce GlyphWidgets and tackle overlapping decorations in gutter #114776
• OnClick event on Gutter #5455

Summary of existing discussion

Summarizing some alternative proposals from the above issue thread #114776 and from standup, there are two main questions to solve:

1. How should we render decorations if there are multiple?
   1. Have dedicated lanes for decoration types
      • We would merge all decorations into a single lane if no line has more than one decoration type, to preserve the nice behavior that test coverage extensions offer
      • This still potentially suffers from pathological cases with lots of decorations so requires some kind of overflow behavior
   2. Have one dedicated lane for breakpoints (common case), and show all other decorations in a second lane
   3. For overflow behavior, sort by each decoration's zIndex to figure out which decoration to render as the placeholder in the overflow
   4. For common cases like testing and debug, the decorations can specify how to merge them to avoid overflow
2. Many but not all decorations are actionable, how do we decide what action to take?
   1. Have decorations provide an onClick handler, if there's only one decoration registering an onClick handler we should execute that handler, otherwise show the context menu
   2. Show overflowed decorations on hover (since the user still needs to be able to see non actionable decorations) and allow to click on decorations to execute their onClick handlers if any

Proposed approach

I'd like to rework my PR to incorporate the proposals that the team has suggested. Here's what I'm thinking of doing, please let me know what you think:

1. Create at most two lanes in the glyph margin
   • One lane is reserved for breakpoints (the hint as well as the actual breakpoint/logpoint)
   • Another lane is reserved for all other decorations, actionable or otherwise
     • This includes the diff revert icon, testing decorations, and extension-contributed decorations
     • If there is more than one decoration to render, we will decide which one to show based on the decoration's specified zIndex
       • Any lower priority decorations will be shown in a hover that still allows users to select or click the decorations, this is similar to what Atom did
       • It should be possible to tab to the overflowing decoration and hit tab to trigger the hover, then use left and right arrows to navigate between decorations. There should also be descriptive alt text for the decorations whether or not they are connected to onClick handlers
   • One question is whether to create space for one or two lanes if there are no decorations other than breakpoints to be rendered. I propose that we should create two lanes in order to avoid the glyph margin resizing in order to create the lane for a breakpoint hover hint if the user hovers over a non-breakpoint decoration and wants to set a breakpoint (though if this ends up behaving poorly I'm also happy to just have one lane)
2. Add support for optional onClick handlers in decorations and adopt them in core-contributed decorations
   • This is to support interacting with overflowed decorations in the proposed hover runs associated actions, which aligns with user expectations and achieves parity with what breakpoints have (today breakpoints and testing have separate context menu handlers)
     • Breakpoint clicks will work the way they currently do
     • If there are overflowing decorations and only the highest-priority decoration has a click handler, we will run that decoration's click handler when the user clicks that decoration
     • If there are overflowing decorations and a decoration which is not the highest-priority decoration has a click handler, we will display the hover
   • Expose support for running extension-contributed actions in decorations
     • This could be a command attribute in TextEditorDecorationType, or an event vscode.window.onDidSelectTextEditorDecoration

cc @alexdima

[gjsjohnmurray]

When there is more than one decoration provided for the second lane of a line I think there should be a visual indicator to tell me it is worth hovering over or navigating to so the hidden decoration(s) appear(s).

[alexdima]

@joyceerhl I really like your proposal. Given this might be a lot of work, and a lot of changes, my suggestion would be to stage the work in steps and split it up across multiple PRs.

[joyceerhl]

@gjsjohnmurray good point, I think the dominant proposal for that is to render a badge, ellipses or a 🔽 next to an overflowing decoration to indicate that there are overflowing items. I'll pick one of those options in the initial exploration.

cc @connor4312 @rebornix @isidorn for additional feedback since you've had input on this issue in the past/in standup

[rebornix]

Insightful writeup and the proposal is great 👍

One question is whether to create space for one or two lanes if there are no decorations other than breakpoints to be rendered. I propose that we should create two lanes in order to avoid the glyph margin resizing in order to create the lane for a breakpoint hover hint

Not sure if we want a wider gutter all of a sudden for all editors. Maybe we can check if there is debug adapter registered to control if breakpoint lane should be created, for example, it doesn't need to be created in markdown files in vanilla VS Code, otherwise we will have an empty lane which doesn't do anything.

[isidorn]

Thanks for the writeup. I agree with @rebornix - let's try to minimize doubling the gutter and only do that when it is really needed.

Reading the above, option 2. ii) sounds interesting to me - "Show overflowed decorations on hover". With that we would minimize the additional real estate we need. What is downside of that approach compared to the two gutters one?

[joyceerhl]

@isidorn yeah, I proposed that we do both (have a dedicated breakpoint lane if it makes sense to render one, as well as render overflowing decorations via a hover). We need to solve the overflow problem regardless, and the dedicated lane allows that we can still show the breakpoint hover hint if there's already a decoration on that line (e.g. if you had test coverage decorations for every line in the file, you'd still be able to set breakpoints easily).

[joyceerhl]

Given this might be a lot of work, and a lot of changes, my suggestion would be to stage the work in steps and split it up across multiple PRs.

I'll split the proposed work into the following sequence of changes:

1. Support rendering decorations by zIndex and adopt for debug and testing to address Decorations with gutter icons hide breakpoint icons #5923
2. Support merging decorations @connor4312 mentioned preferring the dedicated debug lane, so I won't implement this unless we have another usecase for specifying merge icons besides test and debug
3. Support showing overflowed decorations in hover to address Introduce GlyphWidgets and tackle overlapping decorations in gutter #114776
4. Support click handlers to address OnClick event on Gutter #5455
5. Support dedicated lane for debug

PR for the first step is out: #179910

[eamodio]

This sounds awesome! Can't wait for it all to land 😁

As an FYI, JetBrains Rider just added support to show the breakpoints over the line numbers to save horizontal real estate. https://www.jetbrains.com/help/rider/New_UI.html#gutter

[alefragnani]

That's great news! Eager to use and contribute to it.

I follow @connor4312 preference for a dedicated debug lane, and as @eamodio commented, Rider UI approach to put breakpoint over the line number is interesting.

My only fear is about embracing zIndex to stack decorators. I wonder if it would end up extensions fighting with each other for the higher value. But I also understand that a wider gutter would not be great for some users as well (not my case, TBH). Maybe this could be a setting, leaving the user the decision on how to see the extension-contributed decorators in gutter. If you follow a wider gutter approach, maybe a new Contribution Point could defined, for extension authors contribute to gutter decorations. Doing so, VS Code could know upfront how many decorations it would have to deal with, and prepare the gutter according to it.

Thank you