-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Automatically restart clangd language server after it is installed #749
Automatically restart clangd language server after it is installed #749
Conversation
bb1d370
to
9f2f7ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. The general approach looks reasonable to me. A few comments on the specifics:
src/install.ts
Outdated
@@ -129,6 +132,11 @@ class UI { | |||
return p; | |||
} | |||
set clangdPath(p: string) { | |||
config.update('path', p, vscode.ConfigurationTarget.Global); | |||
this._pathUpdated = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a more conventional way to write this is:
this._pathUpdated = new Promise(resolve => {
config.update('path', ...).then(resolve);
});
or alternatively:
this._pathUpdated = new Promise(async resolve => {
await config.update('path', ...);
resolve();
});
This avoids the extra variable _resolvePathUpdated
, and also ensures that if the setter is called multiple times, awaiting _pathUpdated
will correctly wait until the most recent promise is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with first option as config.update
is not awaitable.
src/install.ts
Outdated
async promptReload(message: string) { | ||
if (await vscode.window.showInformationMessage(message, 'Reload window')) | ||
vscode.commands.executeCommand('workbench.action.reloadWindow'); | ||
await this._pathUpdated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should null out _pathUpdated
after having awaited it, to avoid any objects being unnecessarily kept alive
@@ -81,9 +81,12 @@ class UI { | |||
} | |||
} | |||
|
|||
private _pathUpdated: Promise<void>|undefined; | |||
private _resolvePathUpdated: (() => void)|undefined; | |||
|
|||
async promptReload(message: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should ignore message
entirely. Can we still show it in an info dialog, without waiting for the user to interact with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this:
async promptReload(message: string) {
vscode.window.showInformationMessage(message)
await this._pathUpdated;
this._pathUpdated = null;
vscode.commands.executeCommand('clangd.restart');
}
Without await
, this won't wait for user prompt and it would serve as a status message 'clangd ${version} is installed'
. Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly
e377555
to
ff43647
Compare
ff43647
to
add1a70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.