-
Notifications
You must be signed in to change notification settings - Fork 34.9k
Description
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:
- Anything more than 3 decorations in the glyph margin starts to look unreasonable
- 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:
- How should we render decorations if there are multiple?
- 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
- Have one dedicated lane for breakpoints (common case), and show all other decorations in a second lane
- For overflow behavior, sort by each decoration's
zIndex
to figure out which decoration to render as the placeholder in the overflow - For common cases like testing and debug, the decorations can specify how to merge them to avoid overflow
- Have dedicated lanes for decoration types
- Many but not all decorations are actionable, how do we decide what action to take?
- Have decorations provide an
onClick
handler, if there's only one decoration registering anonClick
handler we should execute that handler, otherwise show the context menu - 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
- Have decorations provide an
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:
- 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 useleft
andright
arrows to navigate between decorations. There should also be descriptive alt text for the decorations whether or not they are connected toonClick
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)
- 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 inTextEditorDecorationType
, or an eventvscode.window.onDidSelectTextEditorDecoration
- This could be a
- 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)
cc @alexdima