Skip to content

debug: debug.hideNonDebugHovers #48747

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

Merged
merged 2 commits into from
Apr 26, 2018
Merged

debug: debug.hideNonDebugHovers #48747

merged 2 commits into from
Apr 26, 2018

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented Apr 26, 2018

fixes #48733

Introduces a settings debug.hideNonDebugHovers. Feedback on the name and description are very welcome.

@alexandrudima as you gave me the pointer I am simply provideHover with a 300 ms delay so it does not get spammed. Note that I do not do anything if debug is not active and in a stopped state since then I do not hide the regular hover.

@isidorn isidorn added this to the April 2018 milestone Apr 26, 2018
@weinand
Copy link
Contributor

weinand commented Apr 26, 2018

The semantics of debug.hideNonDebugHovers violates our "falsy" rule: a missing debug.hideNonDebugHovers has not the same meaning as a flag with value false.

What's about debug.showAllHovers instead?

@isidorn
Copy link
Contributor Author

isidorn commented Apr 26, 2018

@weinand I try to respect that rule for DAP attributes, since the configurationService has nice api to give me the default value I am not so worried from the implementation side.
However for consistency we can change it. Why I was pushing for the negation is that showAllHovers as a setting would be a bit strange that the hover is not actually shown due to our implementation. Thus I put the negative spin - not sure if that is smart though

I am open for changing it

@alexdima
Copy link
Member

@isidorn

How about:

  • debug.suppressLanguageHover
  • debug.suppressOtherHovers
  • debug.disableOtherHovers
  • debug.enableOtherHovers

I don't know...

@isidorn
Copy link
Contributor Author

isidorn commented Apr 26, 2018

@alexandrudima thanks for the suggestions, after discussing with @weinand we landed on
debug.enableAllHovers which by default is false
@alexandrudima let me know if the code looks good to you and I will merge it in

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor feedback.

@@ -340,6 +341,17 @@ export class DebugEditorContribution implements IDebugEditorContribution {
return new RunOnceScheduler(() => this.hoverWidget.hide(), HOVER_DELAY);
}

@memoize
private get provideNonDebugHoverScheduler(): RunOnceScheduler {
return new RunOnceScheduler(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Where is this disposed?

private get provideNonDebugHoverScheduler(): RunOnceScheduler {
return new RunOnceScheduler(() => {
const model = this.editor.getModel();
const supports = HoverProviderRegistry.ordered(model);
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply call getHover (from getHover.ts) ?

@@ -362,6 +374,10 @@ export class DebugEditorContribution implements IDebugEditorContribution {
return;
}

if (!this.configurationService.getValue<IDebugConfiguration>('debug').hideNonDebugHovers) {
this.nonDebugHoverPosition = mouseEvent.target.position;
this.provideNonDebugHoverScheduler.schedule();
Copy link
Member

Choose a reason for hiding this comment

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

Should we also .cancel() this somewhere?

@isidorn
Copy link
Contributor Author

isidorn commented Apr 26, 2018

@alexandrudima @weinand thanks a lot for feedback, I have addressed it. Merging in

@isidorn isidorn merged commit 0b0b0f8 into master Apr 26, 2018
@isidorn isidorn deleted the isidorn/nonDebugHovers branch April 26, 2018 12:54
@DanTup
Copy link
Contributor

DanTup commented May 29, 2018

What does this setting do? I assumed it would call by normal hover provider (so that I can see the nice documentation on methods when hovering, even in a debug session) but even with the setting set to true I'm not seeing them (breakpoint in my hover provider is not hit). I'm a bit confused by the description: "Regular hovers will not be shown even if this setting is true". I'm not sure whether my hover is a "regular hover" and if it is, I'm not sure what this setting is actually enabling.

@weinand weinand assigned isidorn and unassigned weinand May 29, 2018
@DanTup
Copy link
Contributor

DanTup commented May 29, 2018

Actually, it is calling my provider now - but it's still not rendering anything. Are there some rules about when it will render? Should it combine the info from the debugger and the normal hovers? (and what does "Regular hovers will not be shown even if this setting is true" mean?).

@isidorn
Copy link
Contributor Author

isidorn commented May 29, 2018

Hi @DanTup while a user is debugging all other hovers will not be rendered.
We might change that in the future, but this is the current limiation of our hover implementation that the hovers are unaware of each other and would be rendered on top of one another if we enabled that.

That setting is just such that the extension hover provider still gets called with the position even though the hover would not be rendered. It sounds a bit weird but was a specific use case for one extenstion.

So what you are seeing is designed behavior for now. Thanks

@DanTup
Copy link
Contributor

DanTup commented May 29, 2018

That setting is just such that the extension hover provider still gets called with the position even though the hover would not be rendered. It sounds a bit weird but was a specific use case for one extenstion.

Aha, gotcha. Thanks for the explanation! :)

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have a way to enable calls to hover providers when debugging
4 participants