Toggle inlay hints

[predragnikolic]

This is a separate PR that adds toggling of inlay hints.

This is a draft - I am not sure of what the user expected behavior for this feature is.

If inlay hints are visible, and the user has multiple ST windows open, with different sessions open in those windows.

When he run the Inlay Hint toggle command, what is expected? a) that inlay hints for one session in one window are hidden. b) that inlay hints for all sessions in one window are hidden. c) that inlay hints for all sessions in all windows are hidden.

For now I implemented it like a) but will change it based on your feedback.

[predragnikolic]

You can toggle inlay hints with: LSP: Toggle Inlay Hints from the command palette.

or via a keybinding:

    {
        "command": "lsp_toggle_inlay_hints",
        "keys": [
            "UNBOUND"
        ],
        "context": [
            {
                "key": "lsp.session_with_capability",
                "operator": "equal",
                "operand": "inlayHintProvider"
            }
        ]
    },

[predragnikolic]

For me the most logical choices is c) than a).

b) doesn't make much sens if options a or c are considered.

[jwortmann]

From the code (for session in sessions:) I think you have implemented b) and not a). But I haven't looked how the inlay hints are implemented if there are more than one session active which provides inlay hints. Probably then only the hints from one of the sessions are shown?

I would think that option c) makes the most sense, i.e. if I use the key binding to toggle the hints then I would expect this to happen everywhere.

Note that the state for inlay hints from multiple sessions in your current implementation can get out of sync (I believe). For example if one session uses dynamic registration for inlay hints (or initializes a bit later) while for another session the state is currently toggled off, then you would have one session with state "on" and the other session with state "off". Therefore I think it would be better for the show_inlay_hints variable to be not an instance per session, but instead a single global variable that all sessions use.

[predragnikolic]

From the code (for session in sessions:) I think you have implemented b) and not a)

sessions refers to sessions running in the view (not sessions in the window).

I would think that option c) makes the most sense

I also think that, but will wait till more people express their opinion.

[rchl]

When he run the Inlay Hint toggle command, what is expected? a) that inlay hints for one session in one window are hidden. b) that inlay hints for all sessions in one window are hidden. c) that inlay hints for all sessions in all windows are hidden.

I would think that option c) makes the most sense, i.e. if I use the key binding to toggle the hints then I would expect this to happen everywhere.

But if you are using the keybinding, you most likely care about the current file/project you are working on, IMO. So toggling in all windows would likely be a waste of resources. If you really want to permanently toggle inline hints everywhere then you could just toggle the main setting.

(Though I would think that in that case it would have to toggle state for all sessions in the current window, not only the current file as otherwise state could get out of sync between sessions.)

So at this point I'm leaning towards B.

Also, a separate issue but maybe worth thinking about... Maybe it would get confusing when it comes to knowing the actual state of that functionality when toggling this invisible state. I wonder if having some permanent status info when the global option is overridden would be useful. Not something that has to be considered for this PR but maybe an improvement in the feature.

[predragnikolic]

Here are the branches for each option: :) a) that inlay hints for one session in one window are hidden. -> this branch b) that inlay hints for all sessions in one window are hidden. -> https://github.com/sublimelsp/LSP/tree/inlay-hints/toggle-inlay-hints-for-all-sessions-in-current-window c) that inlay hints for all sessions in all windows are hidden. -> https://github.com/sublimelsp/LSP/tree/inlay-hints/toggle-inlay-hints-for-all-sessions-in-all-windows

I am not very picky about any option. I would be happy with any of those options. Next step is to decide what behavior we will go with, and I will focus on that branch. and open a PR for that option.

[jwortmann]

If you really want to permanently toggle inline hints everywhere then you could just toggle the main setting.

I thought the main point of this PR was to prevent restarting of the language server when you toggle the inlay hints. So just to toggle the main setting is not possible for this.

Besides that, it isn't even possible in a useful way to limit the effect to a single window. Because you can drag a tab outside of the window to create a new window, and this tab will still be attached to the same session. Therefore option b) would end up to toggle the inlay hints in some windows, while some other windows would not be affected. That seems like a total mess to me and just a matter of time until bug reports will be filed that toggling of inlay hints seems to work randomly and in an inconsistent way.

So I still have the opinion that option c) makes the most sense.

---

Another thing which might be worth to consider, would be to make this feature more general by not limiting it to inlay hints, but instead maybe a temporary toggle of a particular capability/provider (similar to my suggestion for the initial attempt with the setting toggle). Then an example key binding could be implemented by default for inlayHintProvider. The corresponding capability is already checked before attempting to run the inlay hint request at https://github.com/sublimelsp/LSP/blob/314ec85a1919c6efcc2e61e3be3352cbd8505d63/plugin/session_buffer.py#L640-L644 and the same should be the case for the other requests as well. You would just need to make sure that the corresponding feature is triggered to clear/update the view, when a capability is toggled this way.

This could for example also help some users who want to temporary disable the LSP hover popup in order to see the builtin ST popup (#1967) - might need some more cleanup in LSP to not interfere with ST view settings though.

[rwols]

Can you merge with latest master?

I tried using inlay hints with clangd. It's too intrusive and distracting for me. However I do like it for sometimes looking at large function calls:

I'm guessing there's two types of users:

• People that always have inlay hints on. They can set that boolean setting to true and forget about it.
• People that have inlay hints mostly off, but want to peek at function calls every now and then.

When he run the Inlay Hint toggle command, what is expected? a) that inlay hints for one session in one window are hidden. b) that inlay hints for all sessions in one window are hidden. c) that inlay hints for all sessions in all windows are hidden.

Assuming that users that have inlay hints off by default only want to peek at the inlay hints for a few seconds, I would expect the " toggle inlay hints" command to toggle the inlay hints only for the current view. So I would say:

d) toggle inlay hints for all sessions for the current buffer

Given this, I would expect this toggle to be a setting of the view; something like view.settings().get("lsp_inlay_hints") which returns a boolean whether they're enabled/disabled. Or you could use a toggle boolean in the view event listener.

[rchl]

Given this, I would expect this toggle to be a setting of the view; something like view.settings().get("lsp_inlay_hints") which returns a boolean whether they're enabled/disabled. Or you could use a toggle boolean in the view event listener.

So the override would be stored in the current view but the toggle would actually affect active sessions, even not in the current view? That doesn't sound right.

What would then happen if the user switches to another view with same sessions and toggle the setting again? It would sort of be out of sync then and would do nothing on the first toggle.

---

After last arguments from @jwortmann, I'm willing to agree to c) being OK. My argument was that it would likely be wasteful (depending on the use case) but in practice it's probably not a big deal... So I guess I don't really care about that argument that much.

---

As for having a command for toggling capability... Doesn't sound too bad but sounds like a lot more effort to implement.

[rwols]

So the override would be stored in the current view but the toggle would actually affect active sessions, even not in the current view? That doesn't sound right.

I'm unsure how you're picturing it. The toggle would only toggle phantoms in the current view, does that make sense? If I want to view inlay hints for some functions in a view, I would only need them for that single view.

If you enabled the phantoms in one view, then switch over to another view, then run the toggle command in that other view, yet more phantoms would appear in that other view (independent of the phantoms that were already visible in the first view).

[rchl]

I'm unsure how you're picturing it. The toggle would only toggle phantoms in the current view, does that make sense? If I want to view inlay hints for some functions in a view, I would only need them for that single view.

I see... I thought you meant toggle for sessions but store state in the view.

The problem with this whole feature in general is that we are not sure what the use cases will be.

What if someone reports an issue the day after this is released requesting it to be handled per-session or even globally because he/she has a good use case for that or just prefers that way? Should we anticipate that and plan ahead so that it's possible to change without too much hassle? The ST command could easily take additional argument to handle that but maybe the code should be made in a way that would allow us to support all cases with all logic in one central place.

[predragnikolic]

Ok, I did some further experimentation.

I introduced a new argument for the toggle command:

    {
        "command": "lsp_toggle_inlay_hints",
        "args": {
        "args": {
            "toggle": "current_view" // values: "current_view", "current_window", "all_windows"
        },
        "keys": [
            "UNBOUND"
        ],
        "context": [
            {
                "key": "lsp.session_with_capability",
                "operator": "equal",
                "operand": "inlayHintProvider"
            }
        ]
    },

Now I think that having a "toggle" argument to the command is still not perfect because then the keybinding command will not be in sync with the LSP: Toggle Inlay Hints command from the command palette.

For example a user can assign "toggle": "all_windows" to the keybinding, but the LSP: Toggle Inlay Hints will still use "toggle": "current_view"

To solve that issue, we could introduce a new setting for configuring the behaviour.

---

But IMO, I think it would be best to stick with one way of doing things and avoid adding new settings.

[predragnikolic]

My goal is to not add a lot of code for this feature and to avoid adding new settings as I'm not sure if I will cover all possible use-cases for this feature.

I would offer the b) option - toggle inlay hints for the current window. IMO it is a good enough option that will suit most people needs.

---

If others open issues regarding the behaviour, only then I would take action to expand this feature.

[jwortmann]

So are we sure that the command name is a good choice instead of a more generic one which could be reused for other features too?

For example

    {
        "command": "lsp_toggle_capability",
        "args": {
            "capability": "inlayHintProvider"
        },
    },

If implementing it is too much work for now, you could just use the current code like it is now and only adjust the command name, and throw an error if the given capability is not "inlayHintProvider". That would be a temporary solution but leave the generic implementation for later. Because if the "lsp_toggle_inlay_hints" command is introduced now, it is difficult to remove it later in case we decide that it would be useful to have this toggle functionality also for other features. For example for code lenses with "show_code_lens": "phantom" setting it could be similarly useful.

[predragnikolic]

it is difficult to remove it later in case we decide that it would be useful to have this toggle functionality also for other features

I would not rush with merging this. @jwortmann if you want could you create a draft and than we could compare the approaches?

I am going on vacation so I do not think that I will work on any PR-s this week and the next one.

---

Here is my just my thought on toggling by (server) capabilities. It looks doable and appealing.

The things that I would investigate:

• Is it the client responsibility to toggle server capabilities?

Even if the client toggles server capabilities, the server can register and unregister server capabilities when ever the server wants. (but I do not think that the server will register and unregister capabilities in a middle of a session :) but theoretically it can happen, but very very unlikely in the case of inlayHintsProvider)

• It is easy to toggle boolean values, but sometimes the capabilites are of type object with properties. Will it be problematic to toggle if they are of type object?

       /**
	 * The server provides inlay hints.
	 *
	 * @since 3.17.0
	 */
	inlayHintProvider?: boolean | [InlayHintOptions](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHintOptions)
		 | [InlayHintRegistrationOptions](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHintRegistrationOptions);

And there will probably be other questions that would raise up. The best is to implement it and compare the two approaches.

[rchl]

Also i guess it would be tricky to implement it in a generic way and know which functionality to trigger on toggling. For example on toggling inlay hints we want to call specific methods to hide/trigger inlay hints

[jwortmann]

Just writing some thoughts here. Maybe I'll give it a try to implement, but I won't probably do it very soon as well, because I guess I don't desperately need this feature personally, and I have something different for LSP to work on in mind first. So I don't really want to block this PR, and perhaps it turns out that there would be other drawbacks of a more generic approach when implementing it.

---

I would imagine that we use another capability dict either per session, or maybe better globally, which stores whether a particular capability is currently temporarily disabled or not via the lsp_toggle_capability command. But it is probably only useful to store the top-level capabilities here and then they can be boolean values. Then we would also look into that dict when calling the function https://github.com/sublimelsp/LSP/blob/ee8fc6e16c19fc5478e79d83e281cd6dc18924aa/plugin/core/types.py#L833-L844

I think the registrer/unregister capabilities from the servers isn't really relevant if we use another dict like this to store the temporary overrides. So we don't want to actually mutate the options directly which are given by the server.

One aspect to think about is that the disabled_capabilities config is set per server, so if you run two servers for a given file at the same time, and let's say both support inlay hints, one server could have "inlayHintProvider" enabled and the other disabled by the user. If we implement the toggle per session, then it would be pretty confusion because it just switches the inlay hints between the two servers. So I guess only a global storage for the temporarily disabled capablities makes sense. The initial value for a certain capability should probably be True if it is activated for any session. Hm, but that could be tricky with dynamic registration, so maybe needs some more thoughts.

The need of additional actions for certain features when toggling happens could be tricky indeed in a generic way, but should be doable with some "redraw" functions for the affected features which would be called after toggling or so. I think these are basically the features which add regions/phantoms/annotations to a view (inlay hints, code lens, document link, diagnostics, ...).

For the UI I could then imagine to use a ListInputHandler for the command palette, if the lsp_toggle_capability was invoked without the capability argument (very easy to implement via input method in the command). The list items would be the capability names, so it can be easily used even without knowledge of the LSP specs and the particular names which exist. Or perhaps even use another mapping with a display name for each capability, then a list item could look like

[icon] Inlay Hints (inlayHintProvider)

And we could use a green icon for the capabilities which are currently enabled and a red icon for those which are disabled, which I imagine would look pretty cool :)

For the inlay hints, which are probably the feature where toggling is used the most, it should make sense to have a dedicated command "LSP: Toggle Inlay Hints" in the command palette (like currently in this PR).

[predragnikolic]

Made some adjustments to this branch to make this PR mergeable And also made it easy to switch to #2033 with ead75b7

If you think that it is better to wait for #2033, I will close this branch :)

@jwortmann @rchl @rwols

[jwortmann]

I'm not sure which one of the alternatives it the best and should be used. The implementation in PR #2033 seems to be useful only for very few of the capabilities. I also started an attempt to split the settings files at https://github.com/jwortmann/LSP/tree/split-settings, which is intended to not restart the servers anymore if one of the LSP settings changes. But it doesn't work yet and I haven't investigated what exactly needs to be changed for the restarting logic - if anyone wants to make a new alternative PR, this could be used as a starting point though. Optionally we could then add a lsp_toggle_setting command too, which could be used for the menu or for key bindings.

---

For this PR you could add

diff --git a/Main.sublime-menu b/Main.sublime-menu
index cddbe32..ee5ce11 100644
--- a/Main.sublime-menu
+++ b/Main.sublime-menu
@@ -33,6 +33,19 @@
       }
     ]
   },
+  {
+    "id": "view",
+    "children": [
+      {
+        "id": "lsp",
+        "caption": "-"
+      },
+      {
+        "caption": "LSP: Show Inlay Hints",
+        "command": "lsp_toggle_inlay_hints"
+      }
+    ]
+  },
   {
     "id": "goto",
     "children": [
diff --git a/plugin/inlay_hint.py b/plugin/inlay_hint.py
index 2b25b2c..eb300c5 100644
--- a/plugin/inlay_hint.py
+++ b/plugin/inlay_hint.py
@@ -27,6 +27,9 @@ class LspToggleInlayHintsCommand(LspWindowCommand):
             for sv in session.session_views_async():
                 sv.session_buffer.do_inlay_hints_async(sv.view)
 
+    def is_checked(self) -> bool:
+        return LspToggleInlayHintsCommand.are_enabled(self.window)
+
     @classmethod
     def are_enabled(cls, w: Optional[sublime.Window]) -> bool:
         if not w:

and maybe then we don't need the key binding for it, until a better behavior via https://github.com/sublimehq/sublime_text/issues/5462 is possible?

[mikesun]

Curious about the status of this?

[rchl]

On third or fourth thought, I think something like this is fine. As long as the command is generic (lsp_toggle_capability), it's fine if the implementation is not as that can also be switched out for something more generic later.

I will review the changes again when I have some time since I no longer remember any of it.

[predragnikolic]

Rafal if you want to remove the toggle capability command

Also update the key binding file, commands and docs. See this commit ead75b7

[rchl]

Also update the key binding file, commands and docs. See this commit ead75b7

Right, forgot to change those.

I think it makes sense to remove since lsp_toggle_capability is a bit of a lie since it doesn't toggle capabilities. Not in the sense the disabledCapabilities does at least.

I guess we can still think about whether there should be a generic "toggle_functionality" command but since we already have many prefs for toggling specific features, it could add to confusion and more inconsistency.

[rchl]

Apparently my mind is going back and forth with that. Previously I was saying that the command should be generic and now I'm saying that it shouldn't be. :)

One thing I'm sure of is that it we should have a generic command, it should not have "capability" in its name. But otherwise it probably makes sense to have a generic command and potentially hook up more stuff to it later...

[predragnikolic]

No rush.

Both options are perfectly fine with me. And what ever we choose, both options are good. (and we have a way to avoid breaking changes later, see below)

Having a specific command like LspToggleInlayHintCommand pros: the code is a bit smaller

Having a generic command like LspToggleCommand pros: the code is a bit larger but it will be able to handle multiple things

Even if we go to have a specific command now, like LspToggleInlayHintCommand

We can later always add the LspToggleCommand, it would look like this:

class LspToggleCommand:
 def run(..., command, args):
 	if command === 'inlay_hint':
 		v.run('lsp_toggle_inlay_hint') # note we just call the command the we already have

and if the user already assigned the lsp_toggle_inlay_hint to a keybinding, it will still work, because we will not remove that command, so there will be no breaking change.

if we decide to add a generic command right now, I am also fine.

[rchl]

I've also added support for enable argument for the lsp_toggle_inlay_hints command as I figured someone might want to have separate binding for enabling and disabling instead of a "toggle" behavior.