Skip to content
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

Audit for immediate effect of settings #1500

Merged
merged 2 commits into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/cmake-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,21 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
private readonly _codeModel = new Property<codemodel_api.CodeModelContent|null>(null);
private _codeModelDriverSub: vscode.Disposable|null = null;

private readonly _communicationModeSub = this.workspaceContext.config.onChange('cmakeCommunicationMode', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know what 'sub' means here. I know it's reused from other places, but it doesn't really make sense to me. Can you open an issue to rename some of these things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.info(localize('communication.changed.restart.driver', "Restarting the CMake driver after a communication mode change."));
return this._reloadCMakeDriver();
});

private readonly _generatorSub = this.workspaceContext.config.onChange('generator', () => {
log.info(localize('generator.changed.restart.driver', "Restarting the CMake driver after a generator change."));
return this._reloadCMakeDriver();
});

private readonly _preferredGeneratorsSub = this.workspaceContext.config.onChange('preferredGenerators', () => {
log.info(localize('preferredGenerator.changed.restart.driver', "Restarting the CMake driver after a preferredGenerators change."));
return this._reloadCMakeDriver();
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected to see watchers for configureArgs/Settings. I noticed there is something in driver.ts that should be doing it, but there are still reports of those config settings not initiating a reload of the driver. Are you able to repro that at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I saw the reports and I tried but I wasn't able to reproduce. They seemed to update fine for me. I tried windows and linux, fileApi and serverApi. Can you also try to reproduce configureArgs/Settings not getting updated between a settings change and a configure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there may be a bug with deleting the whole setting not taking effect, but I'm not going to block this PR on that. We can open a bug for it. The important scenario of being able to add settings to a fresh folder appears to be working, so I'll approve.

/**
* The variant manager keeps track of build variants. Has two-phase init.
*/
Expand Down Expand Up @@ -191,6 +206,9 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
if (this._launchTerminal) {
this._launchTerminal.dispose();
}
for (const sub of [this._generatorSub, this._preferredGeneratorsSub, this._communicationModeSub]) {
sub.dispose();
}
rollbar.invokeAsync(localize('extension.dispose', 'Extension dispose'), () => this.asyncDispose());
}

Expand Down Expand Up @@ -425,14 +443,14 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
/**
* Reload/restarts the CMake Driver
*/
// private async _reloadCMakeDriver() {
// log.debug(localize('reloading.driver', 'Reloading CMake driver'));
// const drv = await this._cmakeDriver;
// log.debug(localize('disposing.old.driver', 'Diposing old CMake driver'));
// await drv.asyncDispose();
// return this._cmakeDriver = this._startNewCMakeDriver();
// }

private async _reloadCMakeDriver() {
const drv = await this._cmakeDriver;
if (drv) {
log.debug(localize('reloading.driver', 'Reloading CMake driver'));
await drv.asyncDispose();
return this._cmakeDriver = this._startNewCMakeDriver(await this.getCMakeExecutable());
}
}
/**
* Second phase of two-phase init. Called by `create`.
*/
Expand Down
1 change: 0 additions & 1 deletion src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,6 @@ export function reportProgress(progress: ProgressHandle|undefined, message: stri

export function chokidarOnAnyChange(watcher: chokidar.FSWatcher, listener: (path: string, stats?: fs.Stats | undefined) => void) {
return watcher.on('add', listener)
.on('raw', listener)
.on('change', listener)
.on('unlink', listener);
}
Expand Down