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.

Sign up for GitHub

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

Ensure lsp_save actually runs when context is active for keybinding #1852

Merged
merged 3 commits into from
Sep 17, 2021
Merged

Ensure lsp_save actually runs when context is active for keybinding #1852

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dc1ed3
Ensure lsp_save actually runs when context is active for keybinding
jwortmann Sep 16, 2021
3d0c9b0
Fix method signature
jwortmann Sep 16, 2021
cd8674b
Add a comment to explain the workaround
jwortmann Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion plugin/save_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from .core.registry import LspTextCommand
from .core.settings import userprefs
from .core.typing import Callable, List, Type
from .core.typing import Callable, List, Optional, Type
from abc import ABCMeta, abstractmethod
import sublime
import sublime_plugin
Expand Down Expand Up @@ -90,6 +90,15 @@ def run(self, edit: sublime.Edit) -> None:
else:
self._trigger_native_save()

def is_enabled(self, event: Optional[dict] = None, point: Optional[int] = None) -> bool:
# Workaround to ensure that the command will run, even if a view was dragged out to a new window,
# see https://github.com/sublimelsp/LSP/issues/1791.
# The check to determine whether the keybinding for lsp_save is applicable already happens in
# DocumentSyncListener.on_query_context and should not be required here, if lsp_save is only used for the
# keybinding. A proper fix should ensure that LspTextCommand.is_enabled returns the correct value even for
# dragged out views and that LSP keeps working as expected.
return True
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@rchl rchl Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


def _trigger_on_pre_save_async(self) -> None:
# Supermassive hack that will go away later.
listeners = sublime_plugin.view_event_listeners.get(self.view.id(), [])
Expand Down