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

Introduce GlyphWidgets and tackle overlapping decorations in gutter #114776

Closed
isidorn opened this issue Jan 22, 2021 · 18 comments · Fixed by #184375
Closed

Introduce GlyphWidgets and tackle overlapping decorations in gutter #114776

isidorn opened this issue Jan 22, 2021 · 18 comments · Fixed by #184375
Assignees
Labels
editor-contrib Editor collection of extras important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders under-discussion Issue is under discussion for relevance, priority, approach

Comments

@isidorn
Copy link
Contributor

isidorn commented Jan 22, 2021

Related discussion #25174

So far we have never tackled the problem of mutliple decorations on the same line.
@alexdima and me have started a discussion and here's are things that came up:

Overlapping decorations

What to render in the Glyph Margin once there are multiple decorations, here are some options:

  1. Try to combine multilpe icons into one. Downside of this is that this will probably not look good if we automatically merge icons
  2. We render the first icon that came and in the corner we put N where N is the number of decorations there
  3. We render a generic decoration icon, so we render the Generic one + N (like a badge)

Once we have something rendered, here are two options how the interaction could look llike:

  1. Clicking on it triggers the context menu which for each decoration has the decoration label as text and no icons
  2. Hover expands the multiple decorations and each decoration gets rendered, on each decoration the user can click / drag

GlyphWidgets

One thing is that we might need to add a new concept to the editor where glyph margins are not just decorations since you need so much interaction with them. One option is to add the concept of “glyphWidget” similar to “contentWidget” where you pass in a dom node to the editor. That would give you a chance to add dom event listeners on your own and then the editor can just be responsible with positioning them.
The glyphWidgets would give more control to the decoration author, one use case would be #88227

@isidorn isidorn added editor-contrib Editor collection of extras under-discussion Issue is under discussion for relevance, priority, approach labels Jan 22, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Jan 22, 2021

@connor4312 it would be cool if you could share your ideas behind the test decorations. What would they be and how would they fit in our above ideas.
@misolori let me know if you have some good ideas for a "generic decoration icon", or what are your thoughts on what would visually fit when there are multiple decorations on a line.

@connor4312
Copy link
Member

I'm guessing Alex shared my use case, which is being able to show breakpoints on lines where the test runner icon is also present

image

For me implementing this experience with GlyphWidgets would be preferable since I need multiple icons and colors here. The badge idea for option (3) sounds nice, and perhaps glyph widgets could control this themselves with something like

interface IGlyphWidget {
  render(c: HTMLElement): void;
  // called if multiple glyphs are on a line, can optionally return a new widget to use instead of the original widgets
  merge?(other: IGlyphWidget): IGlyphWidget | undefined; 
}

I'm having trouble picturing what the "hover expands" scenario would look like, though I see that having the actual breakpoint widget available somewhere is important for the drag+drop scenario.

I think the context menu is good (and is what I am doing manually with test decorations and the breakpoint menu today.)

For another reference, I looked at what intellij does yesterday: they just permanently expand the gutter when extra icons are added. This works but feels bad to me:

@miguelsolorio
Copy link
Contributor

One simple thing we could do if we just wanted to highlight multiple items is using the ... ellipsis that we already use for context menus:

image

Adding a number in a 16x16 would be really difficult to be readable so I'm not sure if that would be good.

@connor4312
Copy link
Member

My concern with that approach is that it removes the ability to see the gutter icons at a glance

@miguelsolorio
Copy link
Contributor

Another idea (taking the combining icon approach) is making half the glyph area clickable for one action and the other half for setting a breakpoint. Once we have two glyphs, we show a context menu for the various actions. Here's a simple prototype:

Screen.Recording.2021-01-22.at.9.56.50.AM.mov

@isidorn
Copy link
Contributor Author

isidorn commented Jan 22, 2021

@misolori I think the half half idea does not really scale, since if there are 3 it would not work.

If you think it is resonable to expect that mutliple decorations can be merged and this looks good then we should go down that approach. I personally think this will automatically not look good, and we should have some icons prepared for some known overlapps. So for example when a test decoration overlaps with a breakpoint we show our previously designed icon by using the glyhWidget.merge API. If we do not have a preset icon, then we should show have some default one...

intellij approach of expadning the whole margin looks ugly to me.

@miguelsolorio
Copy link
Contributor

So we either need to find a generic icon that means "more than 1 glyph" but for @connor4312 that doesn't work. We can't show all glyphs as we don't have space for it.

@connor4312
Copy link
Member

connor4312 commented Jan 23, 2021

Then yea, showing highest z-index with some indicator (like 🔽 ) beside it, and have click actions--which are not formalized for the glyph margin right now, but could be--always open the context menu.

This would require that the breakpoint drag action be startable from the context menu, which I think is less intuitive but probably fine for the edge case.

Without expanding the margin (which I don't think is a good idea) there'll never be my happy case of seeing all icons at a glance, so option 2 is the best alternative 😛

@isidorn
Copy link
Contributor Author

isidorn commented Jan 25, 2021

Thanks for ideas! Let's assign this to February, and I plan to bring it up in our first UX meeting in that milestone so we can continue the discussion there.

@connor4312 Agree, being able to drag and drop a breakpoint when there are multiple decorations there is a bit of an edge case and I would not really worry about it at the moment. Agree that option 2 is a good start, GlyphWidgets could have weight and the one with the highest weight would win. When there is more than 1 decoraiton we render the indicator as you mention, and click always opens the context menu.
I think we will also need a concept of an "actionable" decoration or not. And if only there are more than 1 actionable decorations we would show a context menu on click. Example: yellow callstack decoration is not actionable, breakpoint is actionable so when both are on the line clicking would just execute the action of the breakpoint (as it does today).

@eamodio
Copy link
Contributor

eamodio commented Jan 29, 2021

FYI, this is very related to: #47559 and also #5455

@isidorn
Copy link
Contributor Author

isidorn commented Mar 22, 2021

We did not have cycles this milestone, pushing out to April for now.

@isidorn isidorn modified the milestones: April 2021, May 2021 Apr 8, 2021
@isidorn isidorn modified the milestones: May 2021, June 2021 May 19, 2021
@connor4312 connor4312 added this to the July 2021 milestone Jun 28, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Jul 28, 2021

Not happening this milestone -> On deck. Connor feel free to move to august if you would like to tackle next mielstone.

@isidorn isidorn modified the milestones: July 2021, On Deck Jul 28, 2021
@karthiknadig
Copy link
Member

karthiknadig commented Aug 12, 2021

Just adding here a potential bug with coloring:
image

This happens if you Add Breakpoint after successfully running a test.

When we hit the breakpoint, the running spinner also spins the breakpoint indicator:
image

@isidorn isidorn assigned roblourens and unassigned isidorn Aug 18, 2021
@lukas9393
Copy link

If you use the vscode-go plugin and set coverage decorator to gutter, it is not possible to set a breakpoint in the areas where coverage is shown.

"go.coverageDecorator": {
  "type": "gutter",
}
overlapping.decorations.in.gutter.mov

Maybe the extension should not offer the configuration gutter, as long as VSCode does not allow overlapping decorations in gutter (for this it is of course the wrong repository). However, I wanted to mention the case here so it can be included.

@JamyDev
Copy link

JamyDev commented Oct 21, 2022

@isidorn Any update on this? We're facing the same issue as @lukas9393 and had to switch to highlight to show coverage as a temporary workaround.

@isidorn
Copy link
Contributor Author

isidorn commented Oct 24, 2022

No updates, sorry.

@r3m0t
Copy link
Contributor

r3m0t commented Jan 17, 2023

Is this considered part of the same bug? When a test is running, the custom decoration (gutterIconPath) spins with the "test running" symbol.

You can see it with this extension - https://github.com/ryanluker/vscode-coverage-gutters

Here's my code:

dark: {
                gutterIconPath: gutterSetting
                    ? vscode.Uri.joinPath(this._ctx.extensionUri, "media", "gutter-icon-dark.svg")
                    : "",
                backgroundColor: lineSetting ? "rgba(0, 122, 30, 0.4)" : "",
            },

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-contrib Editor collection of extras important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.