Fix "disable server globally" command for LSP packages

[predragnikolic]

This should fix LSP: Enable / Disable Language Server Globally for LSP-* plugins.

Previously we suggeted users to disable LSP-* with Package Control: Disable package.
The downside of that is that we clean up the Package Storage folder.
And when we run Package Control: Enable package, we have to download the servers bins from the internet again.

Which is ok, unless the user do not have access to the internet at the moment he ran Package Control: Enable package, then we wont be able to download what is necessary.

---

But if we use LSP: Enable / Disable Language Server Globally,
we will just switch the enabled bool setting,
which wont delete anything inside the Package Storage folder.

---

The only thing is that my fix doesn't work, because of this import error:

Traceback (most recent call last):
  File "/Applications/Sublime Text.app/Contents/MacOS/Lib/python33/sublime_plugin.py", line 308, in reload_plugin
    m = importlib.import_module(modulename)
  File "./python3.3/importlib/__init__.py", line 90, in import_module
  File "<frozen importlib._bootstrap>", line 1584, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1565, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1532, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 584, in _check_name_wrapper
  File "<frozen importlib._bootstrap>", line 1022, in load_module
  File "<frozen importlib._bootstrap>", line 1003, in load_module
  File "<frozen importlib._bootstrap>", line 560, in module_for_loader_wrapper
  File "<frozen importlib._bootstrap>", line 868, in _load_module
  File "<frozen importlib._bootstrap>", line 313, in _call_with_frames_removed
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP-json/plugin.py", line 2, in <module>
    from LSP.plugin import DottedDict
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP/plugin/__init__.py", line 12, in <module>
    from .core.sessions import AbstractPlugin
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/sessions.py", line 2, in <module>
    from .diagnostics_manager import DiagnosticsManager
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/diagnostics_manager.py", line 4, in <module>
    from .views import diagnostic_severity
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/views.py", line 20, in <module>
    from .settings import userprefs
  File "/Users/codetribe/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/settings.py", line 3, in <module>
    from .sessions import get_plugin
ImportError: cannot import name get_plugin

related issue -> #1880

[rchl]

The downside of that is that we clean up the Package Storage folder.

While I agree with the fix, this should no longer be true. The storage should only be removed on uninstalling the package now.

The only thing is that my fix doesn't work, because of this import error:

Looks like a recursive import loop.
Not that trivial to fix. Might require some refactoring.

[predragnikolic]

I fixed the import loop,
I expect only "enabled": false to be inserted in LSP-volar.sublime-settings

But currently all settings keys are inserted. Which is not what I want.

Kapture.2021-11-28.at.17.08.12.mp4

[predragnikolic]

When I call sublime.save_settings(settings_basename),

            plugin_settings, plugin_settings_name = plugin.configuration()
            settings_basename = os.path.basename(plugin_settings_name)
            plugin_settings.set("enabled", is_enabled)
            sublime.save_settings(settings_basename)

ST will save create a key in the setting file for each set field. (example plugin_settings.set("enabled", is_enabled))

It turns out that
lsp_utils/generic_client_handler.py set some settings,
but never really saves the loaded_settings with sublime.save_settings(loaded_settings_basename) file so we never see it.

https://github.com/sublimelsp/lsp_utils/blob/master/st3/lsp_utils/generic_client_handler.py#L124

[rchl]

Your need to manually load the user overrides file (and create if missing).

[predragnikolic]

I took a different approach.

I overwrote the:
/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/_client_handler/abstract_plugin.py

class ClientHandler(AbstractPlugin, ClientHandlerInterface):
    """
    The base class for creating an LSP plugin.
    """

    # --- AbstractPlugin handlers -------------------------------------------------------------------------------------

    @classmethod
    def name(cls) -> str:
        return cls.get_displayed_name()

    @classmethod
    def configuration(cls) -> Tuple[sublime.Settings, str]:
+        name = cls.name()
+        basename = "{}.sublime-settings".format(name)
+        filepath = "Packages/{}/{}".format(name, basename)
+        return sublime.load_settings(basename), filepath
-     @classmethod
-     def configuration(cls) -> Tuple[sublime.Settings, str]:
-         return cls.read_settings()

and that fixed the issue.

Kapture.2021-11-28.at.17.36.46.mp4

[predragnikolic]

Your need to manually load the user overrides file (and create if missing).

Can you tell me how to read the settings and override it?

I tried with:

settings = sublime.load_settings(basename)`
settings.set('enabled', is_enabled)
sublime.save_settings(basename)

but that approach doesn't work as it will give the same result as in the video above.

So I am not sure on to manually load the user overrides file.

---

Are you suggesting sublime.load_resource(name)?

[rchl]

Can you tell me how to read the settings and override it?

Manually read the file like in

LSP/plugin/core/types.py

Line 668 in c608b43

base = sublime.decode_value(sublime.load_resource(file))

for example.

But since the issue is more or less caused by lsp_utils, maybe it should be fixed there (though can't immediately tell how).

[rchl]

Searching github for all uses of on_client_configuration_ready seems to indicate that nothing else is using that API so it should be possible to just change it in lsp_utils without breaking anything.

[predragnikolic]

Because you know lsp_utils more than I do,

I still will try to give my assumptions to this: (but keep in mind that I might be very wrong).

To me the main cause to me seems to be read_settings,
because it mutates the settings object here, without saving:

    @classmethod
    def read_settings(cls) -> Tuple[sublime.Settings, str]:
        ...
        for key in cls.get_default_settings_schema().keys():
            loaded_settings.set(key, settings_copy[key]) # mutates the settings object here, without saving
        filepath = "Packages/{}/{}".format(cls.package_name, filename)
        return (loaded_settings, filepath)

read_settings is used in 4 places:

Searching 26 files for "read_settings"

/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/generic_client_handler.py:
  106  
  107      @classmethod
  108:     def read_settings(cls) -> Tuple[sublime.Settings, str]:
  109          filename = "{}.sublime-settings".format(cls.package_name)
  110          loaded_settings = sublime.load_settings(filename)

/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/_client_handler/abstract_plugin.py:
   96      @classmethod
   97      def configuration(cls) -> Tuple[sublime.Settings, str]:
   98:         return cls.read_settings()
   99  
  100      @classmethod

/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/_client_handler/interface.py:
   76  
   77      @classmethod
   78:     def read_settings(cls) -> Tuple[sublime.Settings, str]:
   79          ...
   80  

/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/_client_handler/language_handler.py:
   79      @property
   80      def config(self) -> ClientConfig:
   81:         settings, filepath = self.read_settings()
   82          settings_dict = {}
   83          for key, default in self.get_default_settings_schema().items():

4 matches across 4 files

While I think that we can fix it in abstract_plugin.py with:

class ClientHandler(AbstractPlugin, ClientHandlerInterface):
    @classmethod
    def configuration(cls) -> Tuple[sublime.Settings, str]:
+        name = cls.name()
+        basename = "{}.sublime-settings".format(name)
+        filepath = "Packages/{}/{}".format(name, basename)
+        return sublime.load_settings(basename), filepath
-     @classmethod
-     def configuration(cls) -> Tuple[sublime.Settings, str]:
-         return cls.read_settings()

I am not sure how to fix it for language_handler.py:

   79      @property
   80      def config(self) -> ClientConfig:
   81:         settings, filepath = self.read_settings()
   82          settings_dict = {}
   83          for key, default in self.get_default_settings_schema().items():

As I think it is used for ST 3 only. Which would mean that this wont be fixed for ST3 users.

[rchl]

To me the main cause to me seems to be read_settings,
because it mutates the settings object here, without saving:

As far as I can see, the problem is not so much with it not saving the settings (since that would trigger the same problem that you have, just earlier), but the fact that it mutates the settings. We can't just skip that part, as you did with your change, as we need to extend env in one place.

I think that a proper solution would require some new API to be able to modify the config without actually modifying the Settings object. So basically modifying the ClientConfig rather than Settings. We'd need to make sure then that the changes are also applied when original settings are reloaded.

I guess I will try to come up with some solution for that.

[predragnikolic]

The PR probably fixed on the lsp_utils side,
but it seems that there is a settings.set('languages', value) call left somewhere.

If I have a LSP-json.sublime-settings file:

{
	"enabled": true,
}

when I run the disable language server command and select LSP-json.

I expect the LSP-json.sublime-settings to be:

{
	"enabled": true,
}

but this gets "languages" key gets inserted as well. (and note that the languageId is null)

{
	"enabled": false,
	"languages":
	[
		{
			"document_selector": "source.json",
			"languageId": null
		}
	],
}

---

EDIT: found the responsible code :)
/Users/codetribe/Library/Application Support/Sublime Text/Packages/lsp_utils/st3/lsp_utils/_client_handler/abstract_plugin.py

    @classmethod
    def on_settings_read_internal(cls, settings: sublime.Settings) -> None:
        languages = settings.get('languages', None)  # type: Optional[List[LanguagesDict]]
        if languages:
            settings.set('languages', cls._upgrade_languages_list(languages)) # here it is :)

[predragnikolic]

I believe that PC still has the issue where it doesn't update dependencies regularly.

Even when the fix is merged to lsp_utils,
should we merge this in the LSP main immediately?

Or wait a month before we merge this, just to make sure that users do not run into this issue

[rchl]

Even when the fix is merged to lsp_utils,
should we merge this in the LSP main immediately?

I think that the time makes no difference here.

I haven't verified but I believe the dependencies are only updated when the package that uses given dependency gets updated.

[jwortmann]

Anything stopping this PR from being merged?

I just ran into this issue and saw that here is already a PR to fix it. The solution looks good to me and it worked without any problems on my side. The release for lsp_utils with the corresponding fix was already two months ago.

[rchl]

Looks good to me.
There was a failing test but that was just a fluke.