Polish explorer tooltip rendering #174085

lramos15 · 2023-02-10T18:05:49Z

Polishes tooltip rendering to match more what windows does

Tyriar

Do we need another setting for this? Would adopting the custom hover help to avoid it obscuring the other items? Obscuring other list items seems to be the main complaint.

Is the delay too short?

lramos15 · 2023-02-10T18:30:08Z

@Tyriar It sounds like there's two concerns here.

It blocks items (which I don't see on my Mac personally, but the screenshots do show it).
It provides little actual value to many due to them already knowing the full path which they have open. Breadcrumbs also provides this info.

Tyriar · 2023-02-10T18:42:49Z

It provides little actual value to many due to them already knowing the full path which they have open

It can provide value though, adding a setting to disable it outright feels like a cop-out. Will making it less intrusive solve the problem most of the way so we can avoid the extra setting?

Users could have problems with all sorts of hovers across the product, we shouldn't be adding individual settings to disable the hovers (I'm guilty of this too with terminal.integrated.showLinkHover which I somewhat regret adding).

Tyriar · 2023-02-10T18:46:41Z

And if it indeed does not provide value, should we remove the hover outright? We could then explore doing something like this on hover to help with truncated items:

lramos15 · 2023-02-10T19:52:31Z

And if it indeed does not provide value, should we remove the hover outright? We could then explore doing something like this on hover to help with truncated items:

This seems like the best future, but quite complicated. Maybe as a stop gap we switch to custom hovers that always render outside the explorer view?

grantsimongrant · 2023-02-12T03:29:20Z

Fixes #173236

I utilized null as an explicit lack of value here to minimize the diff, but that may create confusion surrounding when null vs undefined is used as it's not as simple as just doing !value anymore. Let me know if you think I should instead add an explicit option for disabling the tooltip.

I wish it to be used for all hovers, not just explorer hovers. Two simple solutions, add an option to disable all hovers, or set a shortcut key for all hovers, both are acceptable for me.

grantsimongrant · 2023-02-12T03:36:45Z

And if it indeed does not provide value, should we remove the hover outright? We could then explore doing something like this on hover to help with truncated items:

This seems like the best future, but quite complicated. Maybe as a stop gap we switch to custom hovers that always render outside the explorer view?

It is easier to customize whether to enable all hovers than to delete all hovers. In addition, we can use this method to achieve custom effects, increasing the upper limit of the hover delay to a few minutes, or even an hour.

Tyriar · 2023-02-13T12:53:39Z

Two simple solutions, add an option to disable all hovers, or set a shortcut key for all hovers, both are acceptable for me.

Both of these are unfortunately not as easy as you might expect. At least until we move all hovers onto the custom hovers (as opposed to our current mix of title attribute and custom hovers)

src/vs/workbench/services/hover/browser/hoverService.ts

src/vs/workbench/contrib/files/browser/views/explorerViewer.ts

Tyriar · 2023-02-14T15:58:28Z

src/vs/workbench/contrib/files/browser/views/explorerViewer.ts

+				...options,
+				target: indentGuideElement,
+				compact: true,
+				additionalClasses: ['explorer-item-hover'],


The icon would be a nice addition and I see you said we can defer that, but I think aligning the position and font exactly with underneath would be really good to go out with this release. It's one of the things that makes the Windows Explorer feature so nice imo.

Hopefully this is close enough. Spent quite some time trying to get the icon to work, but even cloning the row doesn't make it work because it's a psuedo element with a psuedo style. I would have to figure out how to get that pseudo element created in the hover as well.

Looks better 👍

You could maybe align it easy via getComputedStyle and then applying all the relevant font values to the hover element?

* Place explorer tooltip rendering behind a setting * Implement windows hover behavior * Pass the actual mouse event rather than a synthetic click * Remove explorer setting * Remove more code that was added for the setting * Fix bad merge * Address PR comments

gregdelozier · 2023-02-17T18:05:52Z

I don't know how to make a screenshot of it, but the "Contains emphasized items" tooltip is so large that it interferes with mouse context menu operations. It is not fixed. Please consider a setting to disable these.

lramos15 · 2023-02-17T18:12:26Z

@gregdelozier That will be fixed for insiders and this tooltip will not go out in stable as there is still some polish to be done here.

dersia · 2023-02-23T09:03:56Z

I'd like to add that this is super annoying, I can't even fast-scroll through the files, because the scroll gesture gets caught by the hover and it stops scrolling, I have to look for some empty space in the explorer so I can continue scrolling. Sometimes I wish for a tooltip, if I need to see the full filename and if it is too long, but I would expect for the tooltip to work the same way tooltips work anywhere else, by staying on the item for a few seconds.

I would rather turn this off and have no tooltips than to be interrupted in my work because of the tooltip. Is there a way to turn this feature off until it is fixed?

nosnaj · 2023-02-25T15:19:07Z

I never noticed the original tooltip issue, until this supposed fix made me notice it in a worse way. Now I can't scroll quickly and pulling up the context menu becomes troublesome.

lramos15 · 2023-02-27T14:27:17Z

I'd like to add that this is super annoying, I can't even fast-scroll through the files, because the scroll gesture gets caught by the hover and it stops scrolling, I have to look for some empty space in the explorer so I can continue scrolling. Sometimes I wish for a tooltip, if I need to see the full filename and if it is too long, but I would expect for the tooltip to work the same way tooltips work anywhere else, by staying on the item for a few seconds.

I would rather turn this off and have no tooltips than to be interrupted in my work because of the tooltip. Is there a way to turn this feature off until it is fixed?

This should be off now, still working on improving it before looking to ship this.

Place explorer tooltip rendering behind a setting

a3bef1e

lramos15 self-assigned this Feb 10, 2023

lramos15 enabled auto-merge (squash) February 10, 2023 18:05

vscodenpa added this to the February 2023 milestone Feb 10, 2023

Tyriar reviewed Feb 10, 2023

View reviewed changes

lramos15 added 3 commits February 13, 2023 13:02

Implement windows hover behavior

bf798df

Pass the actual mouse event rather than a synthetic click

c6545e0

Remove explorer setting

d469adf

lramos15 changed the title ~~Place explorer tooltip rendering behind a setting~~ Polish explorer tooltip rendering Feb 13, 2023

lramos15 requested a review from Tyriar February 13, 2023 21:02

lramos15 added 2 commits February 13, 2023 14:14

Remove more code that was added for the setting

e661e75

Merge branch 'main' into lramos15/dead-chicken

663f207

Tyriar requested changes Feb 14, 2023

View reviewed changes

lramos15 added 3 commits February 14, 2023 09:46

Merge branch 'main' into lramos15/dead-chicken

123a6bf

Fix bad merge

57938a3

Address PR comments

ffefdf6

lramos15 requested a review from Tyriar February 14, 2023 18:51

Tyriar approved these changes Feb 14, 2023

View reviewed changes

lramos15 merged commit 4119dea into main Feb 14, 2023

lramos15 deleted the lramos15/dead-chicken branch February 14, 2023 20:34

github-actions bot locked and limited conversation to collaborators Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish explorer tooltip rendering #174085

Polish explorer tooltip rendering #174085

lramos15 commented Feb 10, 2023 •

edited

Loading

Tyriar left a comment •

edited

Loading

lramos15 commented Feb 10, 2023

Tyriar commented Feb 10, 2023

Tyriar commented Feb 10, 2023

lramos15 commented Feb 10, 2023

grantsimongrant commented Feb 12, 2023 •

edited

Loading

grantsimongrant commented Feb 12, 2023

Tyriar commented Feb 13, 2023

Tyriar Feb 14, 2023

lramos15 Feb 14, 2023 •

edited

Loading

Tyriar Feb 14, 2023

gregdelozier commented Feb 17, 2023

lramos15 commented Feb 17, 2023

dersia commented Feb 23, 2023

nosnaj commented Feb 25, 2023

lramos15 commented Feb 27, 2023

Polish explorer tooltip rendering #174085

Polish explorer tooltip rendering #174085

Conversation

lramos15 commented Feb 10, 2023 • edited Loading

Tyriar left a comment • edited Loading

Choose a reason for hiding this comment

lramos15 commented Feb 10, 2023

Tyriar commented Feb 10, 2023

Tyriar commented Feb 10, 2023

lramos15 commented Feb 10, 2023

grantsimongrant commented Feb 12, 2023 • edited Loading

grantsimongrant commented Feb 12, 2023

Tyriar commented Feb 13, 2023

Tyriar Feb 14, 2023

Choose a reason for hiding this comment

lramos15 Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Tyriar Feb 14, 2023

Choose a reason for hiding this comment

gregdelozier commented Feb 17, 2023

lramos15 commented Feb 17, 2023

dersia commented Feb 23, 2023

nosnaj commented Feb 25, 2023

lramos15 commented Feb 27, 2023

lramos15 commented Feb 10, 2023 •

edited

Loading

Tyriar left a comment •

edited

Loading

grantsimongrant commented Feb 12, 2023 •

edited

Loading

lramos15 Feb 14, 2023 •

edited

Loading