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

Add command to toggle server capability #2033

Closed
wants to merge 2 commits into from

Conversation

jwortmann
Copy link
Member

I went ahead and created a draft what I would imagine as alternative to #2023.

Probably needs some thorough testing, but seems to work pretty well so far. I like the list input in the command palette :)

And I realized that diagnostics can't be toggled, because there is no server capability for that.

@rchl
Copy link
Member

rchl commented Aug 29, 2022

The list of capabilities seems cool (especially the presentation) but I wonder about usefulness of it...

If one wants to quickly and often toggle a capability then it doesn't seem that convenient and keyboard shortcuts would be much better option.

I wonder if there is really a good use case for toggling a capability rarely and also have it only persist temporarily.

What I envision such list being useful for is with connection with the per-server disabled_capabilities setting. It's not always easy to figure out the mapping between capability and the key to use so a list like that is quite useful here but then I wonder if this list input should maybe be more tailored for that use case.

Comment on lines +231 to +237
elif capability == "codeLensProvider":
for session in self.sessions():
for sv in session.session_views_async():
if new_state == "on":
sv.start_code_lenses_async()
else:
sv.clear_all_code_lenses()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be cleaner to expose override_capability (or whatever is the appropriate name) on the Session instead since it already has get_capability/has_capability. Feels like it would have better parity there.

Copy link
Member

Choose a reason for hiding this comment

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

Which then could also be useful for LSP-* plugins to use (see one potential use case in sublimelsp/LSP-css#43).

While the plugin could trigger the command directly instead, it's not ideal as it's "fire and forget" without a return value so there is no feedback that would be good to have in a plugin.

Comment on lines +225 to +230
for session in self.sessions():
for sv in session.session_views_async():
if new_state == "on":
sv.view.settings().set("show_definitions", False)
else:
sv.view.settings().set("show_definitions", user_setting)
Copy link
Member

Choose a reason for hiding this comment

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

Haven't investigated but after disabling hoverProvider for LSP-typescript on a document that runs LSP-eslint and LSP-typescript I'm getting weird flash of content that immediately disappears when showing the popup.

I could investigate if we're gonna proceed with this.

@jwortmann
Copy link
Member Author

Yeah I'm also not convinced if the toggling is useful for most of the features. Besides the inlay hints, I could imagine usecases for the color boxes (which are kind of inlay hints too), and maybe the code lenses. But it doesn't seem very useful to toggle for example the "Goto ..." functionality, since we have the "fallback_command" now anyway.

But the main motivation was rather to make the command for inlay hint toggling more generic, so that we don't end up with more of such commands like lsp_toggle_color_boxes, etc. in the future. The input handler for the command palette was just a small bonus :)

Another problem I see is that some of the features still have a check whether a setting value is enabled (e.g. if userprefs().document_highlight_style, so even if the server has the capability and it is currently shown as activated in the list input handler, then the feature still might not work if the user has the setting disabled. I guess it is possible to fix by removing the if-checks in the code and initializing the stored toggle state dependent on the user-setting value when the plugin is loaded. But I think I'll leave it for now, unless we want to proceed with the approach from this PR.

But perhaps a better alternative would even be to not restart the servers if any of the setting values is changed. Actually a server restart should only be necessary when things like the starting "command", "enabled", or "initializationOptions" setting is changed. Otherwise it should be sufficient to redraw a particular feature in the view if the corresponding option was changed. But I guess to implement that, LSP would need to store all the previous user settings and compare them to the new values to find out which exact setting has changed.

@rchl
Copy link
Member

rchl commented Aug 30, 2022

But perhaps a better alternative would even be to not restart the servers if any of the setting values is changed. Actually a server restart should only be necessary when things like the starting "command", "enabled", or "initializationOptions" setting is changed. Otherwise it should be sufficient to redraw a particular feature in the view if the corresponding option was changed. But I guess to implement that, LSP would need to store all the previous user settings and compare them to the new values to find out which exact setting has changed.

That would be better but it would really be nice for ST to provide such functionality. I imagine it can get pretty complicated to diff preferences changes with more complex configuration objects.

But if we want to attempt to tackle this then it should be a change on its own.

@rchl
Copy link
Member

rchl commented Aug 30, 2022

I suppose at this point my stance is that the generic lsp_toggle_capability is nice but we probably should remove the list input and related code.

@jwortmann
Copy link
Member Author

jwortmann commented Aug 31, 2022

That would be better but it would really be nice for ST to provide such functionality. I imagine it can get pretty complicated to diff preferences changes with more complex configuration objects.

Perhaps there is a simpler solution to it. What if we split the settings file into the regular LSP.sublime-settings containing all the general LSP settings, and another file server-config.sublime-settings (or similar), which contains the configurations for manually installed servers (which is currently the "clients" object in the settings file). Then we know that servers should be restarted if server-config.sublime-settings was edited, but not for the LSP.sublime-settings file. In the latter case we just need to send requests for the visible file(s) and relevant features and redraw those in the view(s). With that it shouldn't even be necessary to figure out what exact setting was changed.

@rwols
Copy link
Member

rwols commented Aug 31, 2022

I think this is a good idea @jwortmann. I had the same idea about having the server startup info, server initialization options and server settings in separate .sublime-settings files so that we only have to listen for changes in the server startup info and initialization options file to restart a server. Hmm, maybe we only need two files then:

For a language server called "foobar":

  • LSP-foobar.sublime-settings: defines process startup info and initialization options. When this file changes, restart the server
  • LSP-foobar-settings.sublime-settings: defines server settings for foobar. When this file changes, only send workspace/didChangeConfiguration.

As an added bonus, it's then "natural" to override LSP-foobar-settings.sublime-settings in the User/ folder without having to deal with DottedDict.

@rchl
Copy link
Member

rchl commented Aug 31, 2022

Changing it for LSP-* packages would be quite disrupting but separating clients from the main LSP settings seems pretty doable (either in a breaking or non breaking way).

But in any case, this is not a strict requirement for this PR.

predragnikolic pushed a commit that referenced this pull request Sep 26, 2022
…an easily switch to that logic, without breaking changes to the user
@jwortmann
Copy link
Member Author

I guess I can close this draft PR, because it doesn't seem to be very useful in general.

I think the proper way to handle the underlying issue would be to split the settings files and never restart servers when LSP's UI settings changes (#2033 (comment)). This would mean to remove "clients" from the settings file, and the individual server configs would then go as top-level entries into a separate .sublime-settings file (this also allows to entirely remove "default_clients" and overrides for the individual configs would still work automatically due to Sublime's built-in settings override mechanism).

I remember that I quickly attempted to start an implementation for that, but I gave up because it wasn't trivial to do it in a backward compatible way. And I think I don't want to invest time for this at the moment.

@jwortmann jwortmann closed this Dec 21, 2022
@rchl
Copy link
Member

rchl commented Dec 21, 2022

I would kinda want to have a way to be able to toggle inline hints quickly. But I'm not sure of what's the correct solution for that.

As far as not restarting on setting changes I'm a little overwhelmed even thinking about it. It would be quite a massive change for LSP packages and existing users. (Where would we expose settings other then the ones from the settings object? How would we handle potential migration of existing user overrides?).

@jwortmann
Copy link
Member Author

It would be quite a massive change for LSP packages and existing users. (Where would we expose settings other then the ones from the settings object? How would we handle potential migration of existing user overrides?).

I'm talking about the settings from the LSP.sublime-settings file, i.e. settings which mainly affect UI and LSP behavior, but not server specific behavior. LSP-* packages would still use their LSP-foobar.sublime-settings file unchanged, and users only need to copy their manual server configs from "clients" to the new settings file. If there happens a change in the LSP-foobar.sublime-settings file from a plugin, we can still restart servers, so that server-specific settings and initialization options will take effect. But if we detect a change/save of LSP.sublime-settings, we only redraw all features in the view, like diagnostics, inlay hints, etc. (if necessary just make new requests to the server as after on_text_change).

To also avoid restarting servers when server-specific settings changes, and use didChangeConfiguration for that instead (#2033 (comment)) would be another possible step, but this could be implemented independently at a later time. This would then have implications for LSP-* packages and users to migrate their individual configuration files though.

@rchl
Copy link
Member

rchl commented Dec 21, 2022

OK, if we ignore LSP packages for now then it should be doable.

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.

3 participants