Ensure lsp_save actually runs when context is active for keybinding

[jwortmann]

Fixes #1791.

Not sure if it is a proper fix, but the logic to determine whether the keybinding is active or not, is already present in documents.py/on_query_context(). Otherwise, the is_enabled evaluating to False from LspTextCommand resulted in preventing Ctrl+S from doing anything, because that binding was active, but not actually run.

I would assume that the code actions on save still won't work in the case described in #1791, but I haven't tested it.

[rwols]

I don’t understand how this solves the linked issue. Can you add some comments in code to explain why this is needed?

[rchl]

Looks like after detaching a document, the on_query_context in:

LSP/plugin/documents.py

Line 408 in 0153663

if any(self.sessions_async(capability)):

returns True but LspTextCommand.is_enabled:

LSP/plugin/core/registry.py

Line 68 in 0153663

return any(self.sessions())

returns False.

So it looks like those places check for sessions in a different way and one way is affected by the detach bug and the other isn't.

And it appears that LspTextCommand.is_enabled returning False doesn't cause ST to ignore the keybinding.

[jwortmann]

DocumentSyncListener.on_query_context returns True if a view is dragged to a new window, while LspTextCommand.is_enabled returns False. I don't know why this divergence happens, but there must be something fundamentally wrong with how applicable views are managed in a session or so. Probably due to the mentioned ST core issue that on_post_move_async isn't triggered. Anyway, on_query_context will cause the keybinding to be active, while is_enabled prevents the run method of lsp_save to be executed. This turns Ctrl+S into "noop". By enforcing is_enabled to be True is it at least ensured that the command is run, and the file can be saved. The bug affected users even if they don't use any code actions on save at all.

It should be pretty fine to have is_enabled always be True, because it is only used for that keybinding and not in code via run_command("lsp_save"). In general, it should not make much sense to do the same check twice whether the keybinding is applicable or not, both in on_query_context and in is_enabled. Eventually a proper fix would ensure that both will return the same value for lsp_save, so that the code actions on save also work as expected (not sure if they already work or not - I just wanted to fix the linked issue here).

I don't really know if the actual bug is in DocumentSyncListener.on_query_context or in LspTextCommand.is_enabled or somewhere else.

[jwortmann]

By the way, this bug affects other things too. For example DocumentHighlights still seem to work fine, while most other things like hover won't work anymore with such a view.

[rchl]

I like the fix but also agree that a comment (in the code) clarifying why it's needed would be good.