fix lsp_check_applicable starting extra server instance

[rchl]

Fix issue noticed in sublimelsp/LSP-copilot#279

The LspCheckApplicableCommand was triggering wm.start_async while there was session being started already, causing it to be started twice. It requires certain timing that for me only seemed to trigger on linux.

Use the existing session-starting machinery instead of short-cutting some of its internal logic that historically caused us issues.

[jwortmann]

Use the existing session-starting machinery instead of short-cutting some of its internal logic that historically caused us issues.

To be honest, this is exactly what I intended to do when I implemented this command. So start_async seemed the most straightforward way to do that.

It's not clear from looking at the code why wm.register_listener_async works here (in fact just from looking at the diff I have no clue what happens in that method, so to properly review I'll have to open that code myself), and why we would register the view listener again. Maybe we want to add a comment like # start a new session for this config?

I think the best would be to refactor start_async so that it automatically skips the given config if it is already starting.
(And ideally it should actually do what its name says - currently when called it leaves the session in a broken state, and that was why the additional lines below that were also needed.)

Or maybe we could even move the logic from here into a new higher level function handle_view (or some different name) in WindowManager that takes the config and the view, and then automatically either attaches/detaches the view (or shutdown session) if a session is running, or starts a new session if not already running.

[rchl]

I'm not sure I really want to do big refactorings here but I've now checked and that fix doesn't really work (at least for re-enabling previously disabled view) so need to do some change for sure. I guess it's missing explicit on_session_initialized.

[rchl]

I've moved the logic to WindowManager (mainly to avoid accessing private properties) and added a check to not do anything if session is already starting (fix for this specific issue):

        if listener == self._new_listener:
            debug(f'Already starting relevant sessions for view {view}.')
            return

Aside from that I've chosen to ignore the session_name argument and I'm processing all configs/session for the given view instead. I think that makes sense but could change it back if someone disagrees.

[jwortmann]

After seeing how this command is used in LSP-copilot (unconditionally in on_activated_async 😕), I'd say it wouldn't be a good idea to trigger new checks for all configs. Imagine there is a LSP-claude or so in the future which uses the command in a similar way. Then we would run the checks for all configs even multiple times on each tab focus change. Also it feels a bit strange that some plugin could enable/disable the session of a different plugin.

Maybe we could add a session_name: str | None = None default value as a "hidden" feature, and then only check all configs if session_name is not provided as a command argument.

[rchl]

I kinda thought that maybe it would be OK to just run this code on view activation. It shouldn't be that expensive and in most cases it would do nothing.

[jwortmann]

I kinda thought that maybe it would be OK to just run this code on view activation. It shouldn't be that expensive and in most cases it would do nothing.

Then we could better move that functionality into LSP directly, instead of providing the command, to ensure that the checks are only triggered one time per view activation. But I'd rather keep it as a command and only do the check on demand. My intention when implementing the command was that LSP-copilot would use a file watcher for .copilotignore files (or alternatively use on_save from a ST ViewEventListener for that filename), then compare its content to the previously ignored files, and then trigger the command only for the affected views. Maybe that thought was a bit too idealistic.