Ensure view gets registered when tab is moved to create new window #1927
Conversation
| 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) |
There was a problem hiding this comment.
We usually just skip the timeout argument. Should be equivalent.
There was a problem hiding this comment.
Should I remove or leave it? I figured it would be nice to see at first glance what this parameter "1" stands for.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
With this 1ms delay we basically craft our own on_post_move_async, but without the bug from the same ST API function :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Lines 205 to 206 in 19a01aa
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.
This is a workaround for the following ST bug: sublimehq/sublime_text#4630
With this change, LspTextCommands still work after a tab is dragged out of a window to create a new window. For example lsp_hover will work again in such a case :)
The 1ms delay appears to be sufficient, so that this check still works:
LSP/plugin/documents.py
Line 205 in 19a01aa