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 view gets registered when tab is moved to create new window #1927

Merged
merged 2 commits into from
Jan 15, 2022
Merged

Ensure view gets registered when tab is moved to create new window #1927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95ac074
Ensure view gets registered when tab is moved to create new window
jwortmann Jan 6, 2022
16ffb84
Remove unnecessary imports
jwortmann Jan 6, 2022
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
9 changes: 7 additions & 2 deletions boot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,18 @@ def on_new_window_async(self, w: sublime.Window) -> None:
def on_pre_close_window(self, w: sublime.Window) -> None:
windows.discard(w)

def on_post_move_async(self, view: sublime.View) -> None:
# Note: EventListener.on_post_move_async does not fire when a tab is moved out of the current window in such a way
# that a new window is created: https://github.com/sublimehq/sublime_text/issues/4630
# Hence, as a workaround we use on_pre_move, which still works in that case.
def on_pre_move(self, view: sublime.View) -> None:
listeners = sublime_plugin.view_event_listeners.get(view.id())
if not isinstance(listeners, list):
return
for listener in listeners:
if isinstance(listener, DocumentSyncListener):
return listener.on_post_move_window_async()
# we need a small delay here, so that the DocumentSyncListener will recognize a possible new window
sublime.set_timeout_async(listener.on_post_move_window_async, timeout_ms=1)
Copy link
Member

Choose a reason for hiding this comment

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

We usually just skip the timeout argument. Should be equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove or leave it? I figured it would be nice to see at first glance what this parameter "1" stands for.

Copy link
Member

Choose a reason for hiding this comment

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

You could skip the timeout value completely, including the number. I feel that that would be more meaningful since it would mean "as soon as possible after current message loop iteration finishes" instead of "after some arbitrary time".

In practice it would of course work the same pretty much.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that is not possible, see my comment in the line above. If there is no delay, the view.window() will still point to the old window, which will result the logic in DocumentSyncListener.on_post_move_window_async not working.
ST needs a little time (>0, but <1ms) after on_pre_move was fired, to create the new window (or to attach the tab to an existing window). At least it didn't work when I tested it without the 1ms delay.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this 1ms delay we basically craft our own on_post_move_async, but without the bug from the same ST API function :)

Copy link
Member

Choose a reason for hiding this comment

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

If those two behave differently then it doesn't bode well for the reliability of this solution. It means it relies on some arbitrary time period that might not be guaranteed in all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what do you suggest then? Increase the delay to for example 5ms? Or keep the current behavior of LSP being mostly broken whenever a tab is dragged out of the window to create a new one?

Copy link
Member

@rchl rchl Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Tried this in safe mode on Mac and "no timeout" works the same as 1ms timeout in this simple case.
(Though I can imagine that different platforms might behave differently)

import sublime
import sublime_plugin

class Listener(sublime_plugin.EventListener):
    def on_pre_move(self, view):
        print('on_pre_move before', view.window())
        sublime.set_timeout_async(lambda: print('on_pre_move after timeout (0ms)', view.window()))
        sublime.set_timeout_async(lambda: print('on_pre_move after timeout (1ms)', view.window()), 1)

on_pre_move before Window(3)
on_pre_move after timeout (0ms) Window(6)
on_pre_move after timeout (1ms) Window(6)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm your results in safe mode on Windows, and surprisingly it seems to work for me in LSP without the delay as well now. But I can swear that when I tested this yesterday, that it didn't work for me without the delay. While I was debugging this, I even added a print statement in

LSP/plugin/documents.py

Lines 205 to 206 in 19a01aa

if new_window.id() == old_window.id():
return
and it would print everytime if, and only if I had not specified the delay.

When I use Tools > Developer > Profile plugins, it shows no other package under on_pre_move, which could possibly block the operation.

Not sure how we should proceed now. If we remove the delay, one option could be to keep the is_enabled: return True in the subclasses of LspTextCommand, for safety in case it doesn't work sometimes or for some users without the delay.

return

def on_load(self, view: sublime.View) -> None:
file_name = view.file_name()
Expand Down
11 changes: 1 addition & 10 deletions 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, Optional, Type
from .core.typing import Callable, List, Type
from abc import ABCMeta, abstractmethod
import sublime
import sublime_plugin
Expand Down Expand Up @@ -90,15 +90,6 @@ 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

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
5 changes: 1 addition & 4 deletions plugin/semantic_highlighting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from .core.registry import LspTextCommand
from .core.typing import List, Optional
from .core.typing import List
import sublime


Expand All @@ -21,9 +21,6 @@ class LspShowScopeNameCommand(LspTextCommand):

capability = 'semanticTokensProvider'

def is_enabled(self, event: Optional[dict] = None, point: Optional[int] = None) -> bool:
return True

def run(self, edit: sublime.Edit) -> None:
point = self.view.sel()[-1].b

Expand Down