Re-read config when restarting server #78
Conversation
src/extension.ts
Outdated
| vscode.commands.registerCommand('clangd.activate', async () => {})); | ||
| context.subscriptions.push( | ||
| clientContext.subscriptions.push( | ||
| vscode.commands.registerCommand('clangd.restart', async () => { |
There was a problem hiding this comment.
I think the restart command registration needs to move to activate -- if any flags passing to clangd are invalid, this code will be not executed, then restart command would not work.
There was a problem hiding this comment.
True, thanks; this is addressed by the rewritten version, see below.
src/extension.ts
Outdated
| await client.stop(); | ||
| client.activate(); | ||
| await clientContext.dispose(); | ||
| clientContext = new LocalContext; |
There was a problem hiding this comment.
looks like we don't need this initialization as the startClient already does this (Line 56) for us.
src/local-context.ts
Outdated
| * A class that lets us keep track of disposables, independently from the | ||
| * extension's global context. | ||
| */ | ||
| export class LocalContext implements vscode.Disposable { |
There was a problem hiding this comment.
Thinking more about this, I feel like we should probably move clangdClient into the Context,
What do you think the proposed API below?
export class ClangdContext implements vscode.Disposable {
subscriptions: vscode.Disposable[] = [];
client: ClangdLanguageClients
globalStoragePath: string
activate() {
// do whatever `startClient` does, get client & server options, init client, call client.start etc.
}
dispose() {
...
}
}
This seems to simplify the codebase:
- the
clientparameter inactivateis not needed anymore, it can be fetched viaclangdContextobject (the same for theglobalStoragePath); - the
startDisposableinclangdLanuageClientcan be removed; - the restart command implement logic seems more natural;
But it would require more refactoring changes, I think it is fine.
There was a problem hiding this comment.
Thanks, that's much nicer.
I pushed a new version of the branch (it's totally rewritten, this couldn't be done with fixup commits in any useful way, sorry for that). I decided to leave globalStoragePath the way it is, since it's only used in constructors, so I thought it's not useful to store it in the class.
The first commit is a bit big and the diff is hard to read because of the large amount of moved code. Let me know if you have any suggestions for improving that.
stefanhaller
force-pushed
the
re-read-config-when-restarting-server
branch
from
885e3fc to
3ec282f
Compare
We introduce a new class ClangdContext that holds the language client and tracks any resources that were allocated for it; this allows us to tear down the client and instanciate a whole new one for restarting the server, instead of just calling stop() and activate() on one and the same instance. This makes it possible to change config options (e.g. the clangd path, or clangd arguments), and have that change take effect when restarting the server.
It's now a member of the context, so let everybody get it from there.
Now that we have a context with the same lifetime as the client, we can use it in the normal way for this, too. This reverts part of 6f2cef1.
stefanhaller
force-pushed
the
re-read-config-when-restarting-server
branch
from
3ec282f to
591a929
Compare
src/extension.ts
Outdated
| })); | ||
|
|
||
| startClient(context.globalStoragePath, outputChannel); |
There was a problem hiding this comment.
I think we can get rid of startClient, stopClient.
const ClangdContext = new ....
context.subscriptions.push(
vscode.commands.registerCommand('clangd.restart', async () => {
await clangdContext.dispose();
clangdContext.activate(...);
}));
clangdContext.activate(...);
There was a problem hiding this comment.
Oh yes! I didn't realize that we can reuse the existing clangdContext instance. This also allows us to get rid of the ugly global variable and the deactivate() function. See fixup commit 405c9de.
When restarting the clangd server, don't just call stop and activate on the same client, but instead tear down the client and instantiate a whole new one. This way we get to see any changed settings (e.g.
clangd.pathorclangd.arguments).I'm new to VS Code development and to Typescript, so please be gentle if I'm doing something stupid here!
Addresses #77.