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

Workbench hover to align and correctly apply delay setting #204871

Merged
merged 16 commits into from Feb 15, 2024

Conversation

benibenj
Copy link
Contributor

@benibenj benibenj commented Feb 9, 2024

This PR refactors the usage of the hover service across various components. Instead of directly using the hover service, a new WorkbenchHoverDelegate is created and used. This delegate encapsulates the hover service and provides a consistent interface for components to use. This change simplifies the code and makes it easier to manage hover behavior across the workbench. Compared to the previous implementation, the delay will adapt when the configuration is changed without having to reload.

@benibenj benibenj self-assigned this Feb 9, 2024
@benibenj benibenj enabled auto-merge (squash) February 9, 2024 22:56
@benibenj benibenj requested a review from bpasero February 9, 2024 22:57
@VSCodeTriageBot VSCodeTriageBot added this to the February 2024 milestone Feb 9, 2024
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for driving this.

I think there are a lot more places where the custom hover is installed where maybe we could think about more reuse. To give some examples:

get delay() {
if (Date.now() - this.lastHoverHideTime < 200) {
return 0; // show instantly when a hover was recently shown
}
return this.configurationService.getValue<number>('workbench.hover.delay');
}

export function setupCustomHover(hoverDelegate: IHoverDelegate, htmlElement: HTMLElement, content: IHoverContent, options?: IUpdatableHoverOptions): ICustomHover {

src/vs/workbench/browser/hover.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/hover.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/hover.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/hover.ts Outdated Show resolved Hide resolved
@benibenj
Copy link
Contributor Author

benibenj commented Feb 13, 2024

Adopted it in further cases. I extended it to also be able to enable if the hover should instantly be shown after hovering over another element. Also the consumer can now add options to override the defaults.
There are still more cases that could be considered, but I think this is a good start as the others are a bit more complicated. Planning on extending the custom hover across further areas in debt week

src/vs/workbench/contrib/timeline/browser/timelinePane.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/remote/browser/tunnelView.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/selectBox/selectBoxCustom.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/table/tableWidget.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/toggle/toggle.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/hover/hoverDelegate.ts Show resolved Hide resolved
src/vs/base/browser/ui/hover/hoverDelegate.ts Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have feedback, but will file a new issue.

@benibenj benibenj merged commit e4e853f into main Feb 15, 2024
6 checks passed
@benibenj benibenj deleted the benibenj/obliged-sparrow branch February 15, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants