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

Partial/full activation mode and multi root. Disable automatic configures. #1295

Merged
merged 8 commits into from
Jun 17, 2020
6 changes: 6 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,12 @@
"description": "%cmake-tools.configuration.cmake.configureOnOpen.description%",
"scope": "resource"
},
"cmake.configureOnEdit": {
"type": "boolean",
"default": true,
"description": "%cmake-tools.configuration.cmake.configureOnEdit.description%",
"scope": "resource"
},
"cmake.skipConfigureIfCachePresent": {
"type": "boolean",
"default": null,
Expand Down
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
"cmake-tools.configuration.cmake.emscriptenSearchDirs.description": "Directories where Emscripten may be installed.",
"cmake-tools.configuration.cmake.copyCompileCommands.description": "Copy compile_commands.json to this location after a successful configure.",
"cmake-tools.configuration.cmake.configureOnOpen.description": "Automatically configure CMake project directories when they are opened.",
"cmake-tools.configuration.cmake.configureOnEdit.description": "Automatically configure CMake project directories when cmake.sourceDirectory or CMakeLists.txt content are saved.",
"cmake-tools.configuration.cmake.skipConfigureIfCachePresent.description": "Skip over the configure process if cache is present.",
"cmake-tools.configuration.cmake.cmakeCommunicationMode": "The protocol used to communicate between the extension and CMake",
"cmake-tools.configuration.cmake.ignoreKitEnv.description": "Do not use the kit environment variables when running CMake commands.",
Expand Down
55 changes: 39 additions & 16 deletions src/cmake-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import diagCollections from '@cmt/diagnostics/collections';
import * as shlex from '@cmt/shlex';
import {StateManager} from '@cmt/state';
import {Strand} from '@cmt/strand';
import {ProgressHandle, versionToString} from '@cmt/util';
import {ProgressHandle, versionToString, lightNormalizePath} from '@cmt/util';
import {DirectoryContext} from '@cmt/workspace';
import * as path from 'path';
import * as vscode from 'vscode';
Expand Down Expand Up @@ -271,8 +271,21 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
const cmakeListsFile = await vscode.window.showOpenDialog(openOpts);
if (cmakeListsFile) {
const fullPathDir: string = path.parse(cmakeListsFile[0].fsPath).dir;
const relPathDir: string = path.relative(this.folder.uri.fsPath, fullPathDir);
vscode.workspace.getConfiguration('cmake').update("sourceDirectory", path.join("${workspaceFolder}", relPathDir));
const relPathDir: string = lightNormalizePath(path.relative(this.folder.uri.fsPath, fullPathDir));
bobbrow marked this conversation as resolved.
Show resolved Hide resolved
const joinedPath = "${workspaceFolder}/".concat(relPathDir);
vscode.workspace.getConfiguration('cmake', this.folder.uri).update("sourceDirectory", joinedPath);
const drv = await this.getCMakeDriverInstance();
if (drv) {
drv.config.updatePartial({sourceDirectory: joinedPath});
} else {
const reloadWindowButton = localize('reload.window', 'Reload Window');
const reload = await vscode.window.showErrorMessage(localize('setting.sourceDirectory.failed.needs.reload.window',
'Something went wrong while updating the cmake.sourceDirectory setting. Please run the "Reload window" command for the change to take effect.'),
reloadWindowButton);
if (reload === reloadWindowButton) {
vscode.commands.executeCommand('workbench.action.reloadWindow');
}
}
} else {
fullFeatureSet = false;
}
Expand All @@ -289,14 +302,14 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
// Previously, the user decided to ignore the missing CMakeFiles.txt.
// Since we are here in cmakePreConditionProblemHandler, for the case of CMakePreconditionProblems.MissingCMakeListsFile,
// it means that there weren't yet any reasons to switch to full functionality,
// so keep enableFullFeatureSet as true.
// so keep enableFullFeatureSet as false.
fullFeatureSet = false;
}

break;
}

await enableFullFeatureSet(fullFeatureSet);
await enableFullFeatureSet(fullFeatureSet, this.folder);
}

/**
Expand Down Expand Up @@ -445,14 +458,27 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {

this.extensionContext.subscriptions.push(vscode.workspace.onDidSaveTextDocument(async td => {
const str = td.uri.fsPath;
if (str.endsWith("CMakeLists.txt")) {
// Reset to a full activation mode.
// If required, the extension will switch back to partial activation mode
// after more analysis triggered by the below configure.
await enableFullFeatureSet(true);

log.debug(localize('cmakelists.save.trigger.reconfigure', "We are saving a CMakeLists.txt file, attempting automatic reconfigure..."));
await this.configure([], ConfigureType.Normal);
const sourceDirectory = await this.sourceDir;
if (str === path.join(sourceDirectory, "CMakeLists.txt")) {
// The configure process can determine correctly whether the features set activation
// should be full or partial, so there is no need to proactively enable full here,
// unless the automatic configure is disabled.
// If there is a configure or a build in progress, we should avoid setting full activation here,
// even if cmake.configureOnEdit is true, because this may overwrite a different decision
// that was done earlier by that ongoing configure process.
const drv = await this.getCMakeDriverInstance();
if (drv && !drv.configOrBuildInProgress()) {
if (drv.config.configureOnEdit) {
log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected saving of CMakeLists.txt, attempting automatic reconfigure..."));
await this.configure([], ConfigureType.Normal);
bobbrow marked this conversation as resolved.
Show resolved Hide resolved
} else {
await enableFullFeatureSet(true, this.folder);
}
} else {
log.warning(localize('cmakelists.save.could.not.reconfigure',
'Detected saving of CMakeLists.txt but could not configure because there is no valid driver or the project is already building/configuring.'));
andreeis marked this conversation as resolved.
Show resolved Hide resolved
log.debug(localize('needs.reconfigure', 'The project needs to be reconfigured so that the changes saved in CMakeLists.txt have effect.'));
}
andreeis marked this conversation as resolved.
Show resolved Hide resolved
}
}));
}
Expand Down Expand Up @@ -678,9 +704,6 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
break;
}
if (retc === 0) {
// Switch to full CMake Tools extension functionality:
// visible status bar and commands in the pallette.
await enableFullFeatureSet(true);
await this._refreshCompileDatabase(drv.expansionOptions);
}
await this._ctestController.reloadTests(drv);
Expand Down
4 changes: 4 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export interface ExtensionConfigurationSettings {
emscriptenSearchDirs: string[];
copyCompileCommands: string|null;
configureOnOpen: boolean|null;
configureOnEdit: boolean;
skipConfigureIfCachePresent: boolean|null;
useCMakeServer: boolean;
cmakeCommunicationMode: CMakeCommunicationMode;
Expand Down Expand Up @@ -241,6 +242,8 @@ export class ConfigurationReader implements vscode.Disposable {

get configureOnOpen() { return this.configData.configureOnOpen; }

get configureOnEdit() { return this.configData.configureOnEdit; }

get skipConfigureIfCachePresent() { return this.configData.skipConfigureIfCachePresent; }

get useCMakeServer(): boolean { return this.configData.useCMakeServer; }
Expand Down Expand Up @@ -323,6 +326,7 @@ export class ConfigurationReader implements vscode.Disposable {
emscriptenSearchDirs: new vscode.EventEmitter<string[]>(),
copyCompileCommands: new vscode.EventEmitter<string|null>(),
configureOnOpen: new vscode.EventEmitter<boolean|null>(),
configureOnEdit: new vscode.EventEmitter<boolean>(),
skipConfigureIfCachePresent: new vscode.EventEmitter<boolean|null>(),
useCMakeServer: new vscode.EventEmitter<boolean>(),
cmakeCommunicationMode: new vscode.EventEmitter<CMakeCommunicationMode>(),
Expand Down
25 changes: 25 additions & 0 deletions src/drivers/cmfileapi-driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,14 @@ import rollbar from '@cmt/rollbar';
import * as util from '@cmt/util';
import * as path from 'path';
import * as vscode from 'vscode';
import * as ext from '@cmt/extension';

import {NoGeneratorError} from './cms-driver';

import * as nls from 'vscode-nls';
nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })();
const localize: nls.LocalizeFunc = nls.loadMessageBundle();

const log = logging.createLogger('cmakefileapi-driver');
/**
* The CMake driver with FileApi of CMake >= 3.15.0
Expand Down Expand Up @@ -97,6 +102,26 @@ export class CMakeFileApiDriver extends codemodel.CodeModelDriver {
log.debug(`Reload CMake cache: ${this.cachePath} changed`);
rollbar.invokeAsync('Reloading CMake Cache', () => this.updateCodeModel());
});

this.config.onChange('sourceDirectory', async () => {
andreeis marked this conversation as resolved.
Show resolved Hide resolved
// The configure process can determine correctly whether the features set activation
// should be full or partial, so there is no need to proactively enable full here,
// unless the automatic configure is disabled.
// If there is a configure or a build in progress, we should avoid setting full activation here,
// even if cmake.configureOnEdit is true, because this may overwrite a different decision
// that was done earlier by that ongoing configure process.
if (!this.configOrBuildInProgress()) {
if (this.config.configureOnEdit) {
log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure..."));
await this.configure([]);
} else if (this.workspaceFolder) {
const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder));
if (folder) {
await ext.enableFullFeatureSet(true, folder);
}
}
}
});
}

doConfigureSettingsChange() { this._needsReconfigure = true; }
Expand Down
26 changes: 25 additions & 1 deletion src/drivers/cms-driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import * as proc from '@cmt/proc';
import rollbar from '@cmt/rollbar';
import { ConfigurationReader } from '@cmt/config';
import * as nls from 'vscode-nls';
import * as ext from '@cmt/extension';

nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })();
const localize: nls.LocalizeFunc = nls.loadMessageBundle();
Expand Down Expand Up @@ -319,7 +320,30 @@ export class CMakeServerClientDriver extends codemodel.CodeModelDriver {
}
}

protected async doInit(): Promise<void> { await this._restartClient(); }
protected async doInit(): Promise<void> {
await this._restartClient();


this.config.onChange('sourceDirectory', async () => {
// The configure process can determine correctly whether the features set activation
// should be full or partial, so there is no need to proactively enable full here,
// unless the automatic configure is disabled.
// If there is a configure or a build in progress, we should avoid setting full activation here,
// even if cmake.configureOnEdit is true, because this may overwrite a different decision
// that was done earlier by that ongoing configure process.
if (!this.configOrBuildInProgress()) {
if (this.config.configureOnEdit) {
log.debug(localize('cmakelists.save.trigger.reconfigure', "Detected 'cmake.sourceDirectory' setting update, attempting automatic reconfigure..."));
await this.configure([]);
} else if (this.workspaceFolder) {
const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder));
if (folder) {
await ext.enableFullFeatureSet(true, folder);
}
}
}
});
}

static async create(cmake: CMakeExecutable, config: ConfigurationReader, kit: Kit|null, workspaceFolder: string | null, preconditionHandler: CMakePreconditionProblemSolver, preferredGenerators: CMakeGenerator[]): Promise<CMakeServerClientDriver> {
return this.createDerived(new CMakeServerClientDriver(cmake, config, workspaceFolder, preconditionHandler), kit, preferredGenerators);
Expand Down
12 changes: 12 additions & 0 deletions src/drivers/driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import * as shlex from '@cmt/shlex';
import * as telemetry from '@cmt/telemetry';
import * as util from '@cmt/util';
import {ConfigureArguments, VariantOption} from '@cmt/variant';
import {enableFullFeatureSet} from '@cmt/extension';
import * as nls from 'vscode-nls';

nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })();
Expand Down Expand Up @@ -593,6 +594,10 @@ export abstract class CMakeDriver implements vscode.Disposable {

private buildRunning: boolean = false;

public configOrBuildInProgress() : boolean {
return this.configRunning || this.buildRunning;
}

/**
* Perform a clean configure. Deletes cached files before running the config
* @param consumer The output consumer
Expand Down Expand Up @@ -863,6 +868,13 @@ export abstract class CMakeDriver implements vscode.Disposable {
return false;
}

// Returning success here should require a full activation mode,
// even if the configure process will fail later.
const folder = vscode.workspace.getWorkspaceFolder(vscode.Uri.file(this.workspaceFolder as string));
if (folder) {
await enableFullFeatureSet(true, folder);
}

return true;
}

Expand Down
79 changes: 33 additions & 46 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ class ExtensionManager implements vscode.Disposable {
this._setupSubscriptions();
}
await util.setContextValue(MULTI_ROOT_MODE_KEY, this._folders.isMultiRoot);

// Removing a workspace should trigger a re-evaluation of the partial/full activation mode
// of the extension, because the visibility depends on having at least one folder
// with valid CMakeLists.txt. If that one happens to be this, we need an opportunity
// to hide the commands and status bar.
this._enableFullFeatureSetOnWorkspace = false;
this.enableWorkspaceFoldersFullFeatureSet();
}

this._onDidChangeActiveTextEditorSub.dispose();
Expand All @@ -127,21 +134,6 @@ class ExtensionManager implements vscode.Disposable {
}
this._statusBar.setAutoSelectActiveFolder(v);
});

this._workspaceConfig.onChange('sourceDirectory', async v => {
await this._onDidChangeActiveTextEditorSub.dispose();
if (v) {
this._onDidChangeActiveTextEditorSub = vscode.window.onDidChangeActiveTextEditor(e => this._onDidChangeActiveTextEditor(e), this);
} else {
this._onDidChangeActiveTextEditorSub = new DummyDisposable();
}

// Reset to a full activation mode.
// If required, the extension will switch back to partial activation mode
// after more analysis triggered by the below configure.
await enableFullFeatureSet(true);
await vscode.commands.executeCommand("cmake.configure");
});
}

private _onDidChangeActiveTextEditorSub: vscode.Disposable = new DummyDisposable();
Expand Down Expand Up @@ -182,30 +174,24 @@ class ExtensionManager implements vscode.Disposable {
telemetry.logEvent('open', telemetryProperties);
}

public getActiveFolderContext() : StateManager | undefined {
const activeFolder = this.getActiveFolder();
if (activeFolder) {
const extensionState = new StateManager(this.extensionContext, activeFolder);
return extensionState;
}

return undefined;
public getFolderContext(folder: vscode.WorkspaceFolder): StateManager {
return new StateManager(this.extensionContext, folder);
}

// Partial activation means that the CMake Tools commands are hidden
// from the commands pallette and the status bar is not visible.
// The context variable "cmake:enableFullFeatureSet" is always equal
// to the state setting ignoreCMakeListsMissing.
// To have them both always in sync, cmake:enableFullFeatureSet is set
// by the getter and setter of ignoreCMakeListsMissing.
public enableFullFeatureSet(fullFeatureSet: boolean) {
const context = this.getActiveFolderContext();
if (context) {
context.ignoreCMakeListsMissing = !fullFeatureSet;
this._statusBar.setVisible(fullFeatureSet);
} else {
log.trace(localize('enableFullFeatureSet.no.active.folder', 'enableFullFeatureSet({0}) called but not active folder found.'), fullFeatureSet);
}
// The context variable "cmake:enableFullFeatureSet" (which controls
// all the cmake commands and UI elements) is set to true,
// if there is at least one folder with full features set enabled.
// We need to add this private _enableFullFeaturesSetOnWorkspace here
// because currently there is no way of reading a context variable
// like cmake:enableFullFeatureSet, to apply the OR operation on it.
private _enableFullFeatureSetOnWorkspace = false;
public enableFullFeatureSet(fullFeatureSet: boolean, folder: vscode.WorkspaceFolder) {
this.getFolderContext(folder).ignoreCMakeListsMissing = !fullFeatureSet;
this._enableFullFeatureSetOnWorkspace = this._enableFullFeatureSetOnWorkspace || fullFeatureSet;
Copy link
Member

Choose a reason for hiding this comment

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

I find this pattern confusing. You need to set _enableFullFeatureSetOnWorkspace to false outside of this method because fullFeatureSet can't change the true to false. Is there a reason you can't just take the value of fullFeatureSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand but let me explain more and you can follow up with rephrasing the above if needed.
Each folder of the multi root workspace may have different values for fullFeatureSet that need to stay persisted in the stage of each of them. Once I apply any OR to them, we'll persist the wrong value. I was thinking that we persist the exact state for each folder: full or partial activation, while partial activation means the "ignore" button has been pressed already and full means the ignore wasn't pressed. And calculate based on them a final decision for the whole multi root workspace. And yes, once true it can't get back to false if we don't have the extra variable.

Copy link
Member

Choose a reason for hiding this comment

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

Do we ever need to turn support off? I'm wondering if we should just leave the full feature set enabled once it's been enabled.

Copy link
Member

Choose a reason for hiding this comment

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

If you remove the state that would make CMake Tools enable the full feature set, then you would need to reload the window at that point to disable it. I would be fine with that behavior.

util.setContextValue("cmake:enableFullFeatureSet", this._enableFullFeatureSetOnWorkspace);
this._statusBar.setVisible(this._enableFullFeatureSetOnWorkspace);
}

/**
Expand All @@ -222,13 +208,6 @@ class ExtensionManager implements vscode.Disposable {
* The folder controller manages multiple instances. One per folder.
*/
private readonly _folders = new CMakeToolsFolderController(this.extensionContext);
public getActiveFolder(): vscode.WorkspaceFolder | undefined {
if (!vscode.workspace.workspaceFolders || vscode.workspace.workspaceFolders.length === 0) {
return undefined;
}

return this._folders?.activeFolder?.cmakeTools.folder || vscode.workspace.workspaceFolders![0];
}

/**
* The status bar controller
Expand Down Expand Up @@ -938,6 +917,14 @@ class ExtensionManager implements vscode.Disposable {
this._statusBar.hideDebugButton(shouldHide);
await util.setContextValue(HIDE_DEBUG_COMMAND_KEY, shouldHide);
}

// Helper that loops through all the workspace folders to enable full or partial feature set
// depending on their 'ignoreCMakeListsMissing' state variable.
enableWorkspaceFoldersFullFeatureSet() {
for (const cmtFolder of this._folders) {
this.enableFullFeatureSet(!this.getFolderContext(cmtFolder.folder)?.ignoreCMakeListsMissing, cmtFolder.folder);
}
}
}

/**
Expand All @@ -952,8 +939,8 @@ async function setup(context: vscode.ExtensionContext, progress: ProgressHandle)
// Load a new extension manager
const ext = _EXT_MANAGER = await ExtensionManager.create(context);

// Enable full or partial feature set, depending on state variable.
ext.enableFullFeatureSet(!ext.getActiveFolderContext()?.ignoreCMakeListsMissing);
// Enable full or partial feature set for each workspace folder, depending on their state variable.
ext.enableWorkspaceFoldersFullFeatureSet();

// A register function that helps us bind the commands to the extension
function register<K extends keyof ExtensionManager>(name: K) {
Expand Down Expand Up @@ -1127,8 +1114,8 @@ export async function activate(context: vscode.ExtensionContext) {
// context.subscriptions.push(vscode.commands.registerCommand('cmake._extensionInstance', () => cmt));
}

export async function enableFullFeatureSet(fullFeatureSet: boolean) {
_EXT_MANAGER?.enableFullFeatureSet(fullFeatureSet);
export async function enableFullFeatureSet(fullFeatureSet: boolean, folder: vscode.WorkspaceFolder) {
_EXT_MANAGER?.enableFullFeatureSet(fullFeatureSet, folder);
}

// this method is called when your extension is deactivated
Expand Down
Loading