Conversation
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
Two observations:
-
The patch has the counter-intuitive behaviour that if the clangd extension has never been activated yet, then running the
clangd.shutdowncommand will actually start the clangd server (because vscode will callactivate()on the extension that declared the command if it hasn't been activated yet). This can probably be avoided with some more work, but it's a bit of an edge case, so I'm not sure if it's worth bothering. -
After a shutdown, running
clangd.activatedoes not have the effect of starting clangd again (butclangd.restartdoes). Should we change it such that it does?
cc @JVApen for any thoughts
|
Before diving into the details, I can say that I've missed this feature and regularly killed clangd on Windows such that I could recompile it. (A problem that doesn't exist on Linux) Regarding 1., although it looks strange to activate the extension due to the abort, I don't think we have much of an option here. I think this is such an edge case that we'll simply have to live with it. Regarding 2., this has me more worried, arguments can be made that 'activate' is about the extension, not the server, though the average user won't see it that way. Personally, I also would expect it to launch the server if not running. The difference I see between activate and restart is that restart does shutdown when a server is active while activate will do nothing. |
|
Added the functionality described by @JVApen of activating the language server via the |
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
LGTM with my comment addressed, and the CI failure fixed (a newly added method needs to be defined in a mock class as well)
src/extension.ts
Outdated
| vscode.commands.registerCommand('clangd.activate', async () => {})); | ||
| vscode.commands.registerCommand('clangd.activate', async () => { | ||
| // If clangd server is already running, do nothing. | ||
| if (clangdContext && clangdContext.clientIsRunning()) { |
There was a problem hiding this comment.
We should do this if the client is in the Starting state as well. (I know clangd.restart checks that and early-returns, but by the time it's called the client may be in the Running state and then we'd stop/start it again for no reason.)
There was a problem hiding this comment.
Added this check
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
Thanks for the update. LGTM.
src/extension.ts
Outdated
| context.subscriptions.push( | ||
| vscode.commands.registerCommand('clangd.activate', async () => {})); | ||
| vscode.commands.registerCommand('clangd.activate', async () => { | ||
| if (clangdContext && clangdContext.clientIsStarting()) { |
There was a problem hiding this comment.
nit: combine these conditions (clangdContext.clientIsStarting() || clangdContext.clientisRunning())?
3 participants
Resolves #512