Add menu item to disable/enable hover popups

[jwortmann]

Here is an idea.
Closes #2315
Closes #1967
Closes #1840

It adds a menu item under the "View" main menu which can be used to temporarily disable LSP hover popups. This takes effect per window, like several of the other built-in items from the "View" menu (Side Bar, Minimap, Status Bar, ...).

If desired, it's also possible to setup a key binding for the lsp_toggle_hover_popups command, but this command is only available if there is a session (LSP server) currently running for the active view, which can provide hover popups.

@rusproject If you like to try it out and report back if this is a good or bad design or whether there are any bugs, testing of this PR by setting up LSP from this Git branch would be welcome.

---

This PR should also fix a bug that the View's "show_definitions" setting was unconditionally (re)set to true, instead of the actual user value from the global preferences file, when (all) LSP servers with hoverProvider are being disabled. Proper implementation now added via reset_show_definitions method.

[rusproject]

Hello! I tried it out a bit, and yes, it does indeed cover the functionality I was requesting in #2315. That's great news! Huge thanks to you!

I've bound the lsp_toggle_hover_popups command to F7, so now I can switch with just one keypress.
I will continue using this branch and will report any issues I come across in the next few days.

Regarding the design, this is a workable solution. I do need to use it a bit longer, but I can foresee one possible inconvenience, which is why I placed the switch hotkey option as the second point in #2315:

You don't always remember the current state. Additionally, given that the current implementation enables/disables hovers per window, it's hard to remember the state of it in each window. An indicator in the status bar could be helpful, but only if it is in a bright, screaming color that could be spotted out of the corner of the eye (and it should be persistent, similar to a branch indicator).

I need to see how it goes, but I can already say for sure that it closes much-needed functionality. After further testing, this pull request should definitely be merged into the main branch. In terms of covering my use case, I would like to emphasize that it would still be beneficial if there was an option to bind the LSP hover popup trigger to a hotkey. This way, both the built-in hover popup and the LSP server popup could be accessed when needed without the need to toggle any settings.

Nonetheless, if the last mentioned thing is challenging or impossible to implement, here are a couple of other ideas that could cover the points in #2315 in a slightly more convenient manner (the last suggestion is an enhancement on top of your newly introduced solution):

• Simply displaying both hovers (LSP and Sublime Text's built-in) simultaneously: for example, one below the mouse pointer and the other one above it. The challenge here is finding the right placement to prevent overlap under some conditions.
• In the current implementation (with a command bound to a hotkey), it would be nice to observe real-time changes in the popup when the command is invoked while the other popup is already rendered. Let's say, you hover the mouse over a function and the LSP-rendered popup appears. Then, when you press the lsp_toggle_hover_popups hotkey:
  • As it currently works: the popup remains the same, and nothing changes. To view the other popup, you need to move the mouse pointer slightly.
  • The desired behavior: you press the hotkey for the lsp_toggle_hover_popups command. The rendered popup is immediately replaced by the other popup if available (or it disappears if no popups are available).

[jwortmann]

An indicator in the status bar could be helpful, but only if it is in a bright, screaming color that could be spotted out of the corner of the eye (and it should be persistent, similar to a branch indicator).

I would be very much against this, imo there are already too many things in the status bar enabled by default.

It could be possible to make the toggle work globally, instead of per window, but iirc the "LSP: Show Inlay Hints" toggle is also per window (and also most of the built-in options under the "View" menu). So I made this one work per window too, for consistency. The LSP popups are enabled by default for new windows or after a restart of ST, because I think that in most situations and for most people the LSP popups are welcome.

beneficial if there was an option to bind the LSP hover popup trigger to a hotkey

This should already be possible via lsp_hover command, and there is even an example key binding (commented out) when you open Preferences: LSP Key Bindings from the command palette.

• Simply displaying both hovers (LSP and Sublime Text's built-in) simultaneously: for example, one below the mouse pointer and the other one above it. The challenge here is finding the right placement to prevent overlap under some conditions.

Not possible, there can always only be a single popup visible. The "show_definitions" popup is implemented in Default/symbol.py, so theoretically we could copy this code and append the popup contents for example at the bottom of the LSP popup. But then we would need to maintain this code to not get outdated with the built-in implementation after ST updates, and I think it wouldn't really look good, so I would probably vote against this idea.

• The desired behavior: you press the hotkey for the lsp_toggle_hover_popups command. The rendered popup is immediately replaced by the other popup if available (or it disappears if no popups are available).

Hm, perhaps this could be possible to implement, but with the ST API you can only check whether there is some popup visible, but not which one or where it comes from. Also we can't retrigger the on_hover EventListener method, so it would only work in one direction (built-in show_definitions popup to LSP popup, but not the other way). So I don't think that it's feasible or would be a big improvement to have this.

[jwortmann]

One thing I realized is that when the LSP popups are disabled, this will also prevent the "Follow Link"/"Open in Browser" popup for documentLinkProvider, which may or may not be part of a regular hover popup. But with the current implementation of having those links shown as part of the regular popups, I think there is no way around that.

Also since the LSP client doesn't have any information about what the hovered text (in this case a link) is, we can't really know if ST built-in "show_definitions" would show a popup for the hovered text. So we couldn't decide whether it is desired or not to show a popup/tooltip over links, which could potentially block the built-in popups.

[rusproject]

An indicator in the status bar

I would be very much against this, imo there are already too many things in the status bar enabled by default.

Yeah, people love Sublime Text for its subtleness and not for the screaming colors indicators, I agree. I wouldn't like that either; I just don't have other ideas about how to make it obvious at any time what the current state of this important option is, since it's related to some functionality that is not invoked in an obvious way. To understand the current state, you need to point a mouse in the right place and wait for a second, and a couple of conditions must be met before any hover could eventually be rendered...

All those mouse-hover related things sometimes behave unpredictably (no matter LSP or built-in). For example, you may hover over a method repeatedly, thinking you missed the right mouse position or it's due to your PC stuttering or being slow, and suddenly you realize that you're doing this in another window (outside of the project), and that's why no references to the symbol are available and no popup could be displayed.

Actually, this all is very strange - we've come to the point where we're not able to invoke some important code editor features with just our keyboard/hotkey (I'm talking about the built-in Sublime hover popup primarily). Do programmers really HAVE TO use the mouse?

So I made this one work per window too, for consistency

That's a good design. However, for my exact use case, if I disable this feature, I expect it to be disabled everywhere. Sometimes I explore two versions of the project side by side in two windows, so if I disable LSP hover to find the exhaustive list of references in one window, I expect the same behavior in the other. Without the ability to toggle hovers globally, you have to perform twice as many actions (disable in both, then when you're satisfied, enable in both). Probably this is why there are two settings for the LSP Server: "Disable LSP Server Globally" and "Disable LSP Server in Project" - users make use of both options depending on the situation. So, perhaps it would be a better design to have several commands: toggle popups globally and toggle them per window/view.

The LSP popups are enabled [...] after a restart of ST

That's a bad news. I probably didn't notice that it worked this way, as I had hover capability disabled for the server permanently. But if that's the case, it's not too convenient... Why would someone want to have something enabled automatically after restarting, if they manually disabled it before closing ST? Perhaps this one needs to be reconsidered...

I think that in most situations and for most people the LSP popups are welcome.

I would agree if the LSP popups weren't replacing the most beloved Sublime Text feature - "show_definitions" with all the symbol references and definitions right at your fingertips... Personally, I can't sacrifice this; I could live without LSP popups, but not without "show_definitions". Actually, the lack of this feature is the primary reason why I didn't switch to VS Code, despite it being more suitable for my project in many other ways. "show_definitions" assists me easily refactor legacy code base; without it I'm blind.

This should already be possible via lsp_hover command, and there is even an example key binding

The main goal is to be able to invoke LSP hover with a hotkey if and only if the built-in Sublime hover is enabled. However, here's the controversy: to have Sublime hover enabled, you must disable LSP hover capability (or it will automatically switch the "show definitions" setting to false every time), and with the disabled LSP hover, the lsp_hover command, obviously, doesn't work.

__

I was working with the current branch checked out for the whole day today, and I noticed an undesired change compared to the main LSP branch:

Errors_are_shown__desired_behavior.mp4

current_branch_behavior.mp4

• Main branch: When you have the hover capability disabled for a specific language server, hovers for method/function definitions are disabled. However, if you encounter an error/warning, the LSP server still shows the popup with that error/warning.
• In the current branch: If you disable hover by toggling it with a command, the popups with error/warning descriptions aren't shown. This cuts one more LSP-feature when you have to work with built-in ST hovers instead of LSP's hovers most of the time as I do.

Another problem is that ST is ignoring "show_definitions": true in this branch.
Previously, if I set "disabled_capabilities": { "hoverProvider": true } for a specific language server and simultaneously enabled ST hovers by setting "show_definitions": true (which is how I worked with LSP+ST for the last two years), LSP hovers were disabled, and ST hovers worked. However, now, when I do the same, even after a restart, ST hovers remain disabled. You can only enable them by re-enabling the disabled capability and then toggling this capability with a command.

[jwortmann]

Why would someone want to have something enabled automatically after restarting, if they manually disabled it before closing ST? Perhaps this one needs to be reconsidered...

To have it persist after restart, it would need it to be stored somewhere, i.e. we need a new global LSP setting "show_hover_popups": true/false (like for inlay hints). But unlike the inlay hints (which can be quite distracting and have some bugs/limitations due to ST phantom API), the LSP hover popups work quite well in my opinion and in most situations, so I would think that 99% of users have them always enabled and don't need such a setting. But if you disagree and there are other users or contributors who disagree as well, I'm open to reconsider.

The decision to make the toggle work per window was mainly driven to be consistent with the inlay hints toggle.
There was a lengthly discussion about what is the best choice in #2023 and actually my opinion was to make it globally, but it became implemented per window in the end, so unless we want to change the behavior for the inlay hints toggle too, I'd say the popup toggle should follow the same behavior. Otherwise it will become very confusing if one feature can be toggled per window, another one globally, etc.

The main goal is to be able to invoke LSP hover with a hotkey if and only if the built-in Sublime hover is enabled. However, here's the controversy: to have Sublime hover enabled, you must disable LSP hover capability (or it will automatically switch the "show definitions" setting to false every time), and with the disabled LSP hover, the lsp_hover command, obviously, doesn't work.

I'm a bit confused if I understand correctly what you mean, but this should actually be the exact point that this PR tries to solve. Make sure to remove the "disabled_capabilities": { "hoverProvider": true } from your configuration. Then you can disable the LSP popups via the new "LSP: Show Hover Popups" menu item, so that Sublime's built-in popups will be shown, and then you can still invoke the LSP popups on demand with a key binding for lsp_hover. Please test again, I can confirm that it works on my side.

• Main branch: When you have the hover capability disabled for a specific language server, hovers for method/function definitions are disabled. However, if you encounter an error/warning, the LSP server still shows the popup with that error/warning.

• In the current branch: If you disable hover by toggling it with a command, the popups with error/warning descriptions aren't shown. This cuts one more LSP-feature when you have to work with built-in ST hovers instead of LSP's hovers most of the time as I do.

Yes, diagnostics are part of the hover popups and they will not be shown when LSP popups are toggled off. There is no way around that. The only solution I could think of would need heavy changes in ST core to rewrite the on_hover EventListener in a way that plugins can register as some kind of "hover provider", and ST collects the hover contents from all plugins, and then combines them into a single popup (e.g. separated with a <hr>). I think this is how VS Code does it. But this will never be implemented in ST, because it cannot be done in a backwards compatible way and without breaking all kind of packages, which at the moment can/do call View.show_popup whenever they want.

And your first recording (with hover capability disabled) even looks a bit buggy to me, because there is a short flicker of the built-in ST popup before the diagnostics popup will be shown, or it's random which one is shown. This is because both the built-in Default/symbol.py as well as LSP will call View.show_popup, and the slower one just forces the popup to be redrawn. That's not a good UX from my point of view.

Another problem is that ST is ignoring "show_definitions": true in this branch.

I can't reproduce that on my side. In fact, it does reset the view's setting to the original value (unless there is some bug in that code, which I have missed):
https://github.com/jwortmann/LSP/blob/5e9215562e0f0d19ca6d46760527df867a66d1e5/plugin/hover.py#L392

You can only enable them by re-enabling the disabled capability and then toggling this capability with a command.

Yes, this is the point of this PR :D
No need anymore to mess with the disabled_capabilities, just use the new toggle from this PR via menu or key binding (though I can't see why it would set the view's setting to false even when you have the hoverProvider disabled).

[rusproject]

And your first recording (with hover capability disabled) even looks a bit buggy to me, because there is a short flicker of the built-in ST popup before the diagnostics popup will be shown, or it's random which one is shown. This is because both the built-in Default/symbol.py as well as LSP will call View.show_popup, and the slower one just forces the popup to be redrawn. That's not a good UX from my point of view.

True, flickering can be seen, but it happens quite rarely, and still, this behavior helped to have both sublime "show_definitions" and the LSP diagnostics available at the same time, with my previous kind of setup. By the way, it's not random; the diagnostics always pops up last, for sure. I used it like this for two years.

Please test again, I can confirm that it works on my side.

Sorry, you're right, on this branch it works just fine!
I made of stupid so I didn't realize that you were actually solving this issue. I thought your PR was about adding just one ability - toggling LSP hover popups on or off. I thought that all other things would remain as they are in main, i.e., if the LSP hover popups are disabled (previously, I was disabling them with "disabled_capabilities": { "hoverProvider": true }), then all popups wouldn't work in all cases, including the one when you try to invoke a popup with a command lsp_hover.
Now I understand that your branch actually solves this; it adds the ability to invoke an LSP popup with a command/hotkey even when the mouse hover popups are disabled with a new lsp_toggle_hover_popups command.

This way, it definitely satisfies my needs and fits my use cases, and I can disregard all other thoughts and ideas that I mentioned. Even regarding the absence of error/warning diagnostics popups when LSP-mouse-hover-popups are disabled: now I can view the same message if I invoke lsp_hover with a command/hotkey. That's awesome enough!

So, to be understood correctly, here's my workflow setup right now on the current branch:

• I removed "disabled_capabilities": { "hoverProvider": true }; I don't need it anymore.
• I bound a hotkey { "keys": ["f6"], "command": "lsp_toggle_hover_popups" } for this new command.
• I disabled the LSP-mouse-hover popups once by invoking the new lsp_toggle_hover_popups function (truly need this to be disabled most of the time since I often need to see built-in ST hover popups, which show a more comprehensive list of symbol Definitions & References).
• I bound a hotkey { "keys": ["f7"], "command": "lsp_hover" }: now it's suitable for me, since it really still works if the LSP-mouse-hover-popups are disabled by the new toggle.
• Now I always have ST built-in popups available with mouse hover, and I also have the LSP popups available with the F7 key binding.
• I'm good 😎

One little suggestion: since the lsp_hover command does invoke the LSP popup even when the LSP hover is disabled, to avoid possible confusion (just like I was before you clarified things), it could possibly be renamed to something like lsp_popup or similar.

Everything else is good, I will stay on this branch with the setup described and eagerly wait until it will be merged into the master.

Cheers!

[rusproject]

omg😁 just realized that this one:

I disabled the LSP-mouse-hover popups once by invoking the new lsp_toggle_hover_popups function (truly need this to be disabled most of the time since I often need to see built-in ST hover popups, which show a more comprehensive list of symbol Definitions & References).

is not actually satisfied since I need to re-disable it each time after restarting the ST.

Well, I understand the arguments, and I don't know what would be better for most of the users (global or window-based disabling). So, the only option that comes to my mind is to think about adding two features: toggle LSP hover for the view/window AND toggle it globally. This way, no one will be harmed :)

[jwortmann]

Are there any other opinions or objections about this PR?

I think there are three possible design choices:

1. Keep and merge it as it is now.
2. Make the toggle work globally. But I would only do this if the inlay hints toggle also becomes a global toggle, because I like consistency.
3. Add a new setting to enable/disable the popup functionality, so that it can be stored permanently and users who want to have them off by default don't need to activate the toggle on new windows or after ST restart.

I would actually be in favor of (2) and think that the global toggle for the functionalities (popups and inlay hints) would be a better design, but since the inlay hints toggle was implemented per window in the end I guess I'm in the minority here. But it's not very important to me and people probably don't like changes.
The alternative idea to create two different toggles, one global and one per view/window would be too excessive for this functionality in my opinion.

For (3) I think that it's not really needed, even if it means a bit on inconvenience for a (probably very small) amount of users.
Edit: The linked issue #1840 also requests this, so I guess I'm open for discussion about this or to change it.

Regarding the constants.py file, I could do a follow up refactor PR to move various constants from views.py and other files into that file.

[rchl]

Just gonna address the last comment since there is way too much discussions here to be able to follow all that.

I prefer per-window switch for inlay hints rather than global. That's because while that feature is annoying due ST issues, there are some projects where it can be more useful.

So if we are are consistent, this should also be per-window.

If we'd want to have a global switch for it then I'd just make the code respect show_popup_on_hover.

[rchl]

I don't think I like the idea of having all constants in a single file. I think the constants are easier to manage when they are together with relevant piece of the code.

Maybe there is a place for some global constants but I wouldn't make it a rule.

[jwortmann]

Maybe we can mark all local / file-specific constants with a leading underscore and only move constants which are shared between multiple files into a common place like this?

We basically already have a part of this in views.py, but it's questionable to me why that file is named views and I think it would be better to only have imports from .protocol and .typing in the file which collects all the shared constants.

[jwortmann]

If we'd want to have a global switch for it then I'd just make the code respect show_popup_on_hover.

A built-in setting with this name doesn't seem to exist (I think it was a bit of a misunderstanding in one of the issues). Or should it suggest to intruduce a new LSP setting with that name?

I would be okay with the per-window toggle for the features, even though it's not the optimal solution in my opinion.

[rchl]

So I was going to do a final review pass on it but then I've noticed that this setting doesn't work like the corresponding Inlay Hints toggle. That is, it is not stored per window. Was this conscious decision to make it different from the Inlay Hints toggle?

[jwortmann]

I haven't looked at this PR for a while, but I think it should be intended to work per window at the moment. If it doesn't, then that would be a bug. But at my side it seems to work as expected; when I disable the menu item no LSP popups are shown in the window, even when I switch to files with a different server running. And when I open another window, I can disable/enable the popups independently of the current state from the first window. Do you have some steps how I could reproduce a case where it doesn't work?

[rchl]

The setting works per-window but it's not stored in window settings like the inlay hints equivalent so it doesn't survive ST restart.

[jwortmann]

Interesting, I didn't know you could do that. PR updated and I think it should work now.