Skip to content

Fix race condition with displaying PowerShell name on icon #4696

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

Merged
merged 1 commit into from
Aug 14, 2023
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
67 changes: 24 additions & 43 deletions src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export class SessionManager implements Middleware {

// The `exeNameOverride` is used by `restartSession` to override ANY other setting.
// We've made this function idempotent, so it can used to ensure the session has started.
public async start(exeNameOverride?: string): Promise<void> {
public async start(): Promise<void> {
switch (this.sessionStatus) {
case SessionStatus.NotStarted:
// Go ahead and start.
Expand Down Expand Up @@ -177,20 +177,16 @@ export class SessionManager implements Middleware {
break;
}

// This status needs to be set immediately so the above check works
this.setSessionStatus("Starting...", SessionStatus.Starting);

this.startCancellationTokenSource = new vscode.CancellationTokenSource();
const cancellationToken = this.startCancellationTokenSource.token;

if (exeNameOverride != undefined) {
this.logger.writeVerbose(`Starting with executable overriden to: ${exeNameOverride}`);
this.sessionSettings.powerShellDefaultVersion = exeNameOverride;
}

// Create a folder for the session files.
await vscode.workspace.fs.createDirectory(this.sessionsFolder);

// Migrate things.
await this.promptPowerShellExeSettingsCleanup();
await this.migrateWhitespaceAroundPipeSetting();

// Find the PowerShell executable to use for the server.
Expand All @@ -205,6 +201,8 @@ export class SessionManager implements Middleware {
return;
}

// Refresh the status with the found executable details.
this.refreshSessionStatus();
this.logger.write(`Starting '${this.PowerShellExeDetails.displayName}' at: ${this.PowerShellExeDetails.exePath}`);

// Start the server.
Expand Down Expand Up @@ -274,7 +272,6 @@ export class SessionManager implements Middleware {
this.startCancellationTokenSource?.dispose();
this.startCancellationTokenSource = undefined;
this.sessionDetails = undefined;
this.versionDetails = undefined;

this.setSessionStatus("Not Started", SessionStatus.NotStarted);
}
Expand All @@ -286,7 +283,16 @@ export class SessionManager implements Middleware {
// Re-load the settings.
this.sessionSettings = getSettings();

await this.start(exeNameOverride);
if (exeNameOverride) {
// Reset the version and PowerShell details since we're launching a
// new executable.
this.logger.writeVerbose(`Starting with executable overriden to: ${exeNameOverride}`);
this.sessionSettings.powerShellDefaultVersion = exeNameOverride;
this.versionDetails = undefined;
this.PowerShellExeDetails = undefined;
}

await this.start();
}

public getSessionDetails(): IEditorServicesSessionDetails | undefined {
Expand Down Expand Up @@ -432,38 +438,6 @@ export class SessionManager implements Middleware {
}
}

// TODO: Remove this migration code.
private async promptPowerShellExeSettingsCleanup(): Promise<void> {
if (this.sessionSettings.powerShellExePath === "") {
return;
}

this.logger.writeWarning("Deprecated setting: powerShellExePath");
let warningMessage = "The 'powerShell.powerShellExePath' setting is no longer used. ";
warningMessage += this.sessionSettings.powerShellDefaultVersion
? "We can automatically remove it for you."
: "We can remove it from your settings and prompt you for which PowerShell you want to use.";

const choice = await vscode.window.showWarningMessage(warningMessage, "Let's do it!");

if (choice === undefined) {
// They hit the 'x' to close the dialog.
return;
}

this.suppressRestartPrompt = true;
try {
await changeSetting("powerShellExePath", undefined, true, this.logger);
} finally {
this.suppressRestartPrompt = false;
}

// Show the session menu at the end if they don't have a PowerShellDefaultVersion.
if (this.sessionSettings.powerShellDefaultVersion === "") {
await vscode.commands.executeCommand(this.ShowSessionMenuCommandName);
}
}

private async onConfigurationUpdated(): Promise<void> {
const settings = getSettings();
this.logger.updateLogLevel(settings.developer.editorServicesLogLevel);
Expand Down Expand Up @@ -524,7 +498,6 @@ export class SessionManager implements Middleware {
break;
}
}

}
foundPowerShell = defaultPowerShell ?? await powershellExeFinder.getFirstAvailablePowerShellInstallation();
if (wantedName !== "" && defaultPowerShell === undefined && foundPowerShell !== undefined) {
Expand Down Expand Up @@ -846,9 +819,12 @@ Type 'help' to get help.
const semver = new SemVer(this.versionDetails.version);
this.languageStatusItem.text += ` ${semver.major}.${semver.minor}`;
this.languageStatusItem.detail += ` ${this.versionDetails.commit} (${this.versionDetails.architecture.toLowerCase()})`;
} else if (this.PowerShellExeDetails?.displayName) { // In case it didn't start.
} else if (this.PowerShellExeDetails?.displayName) { // When it hasn't started yet.
this.languageStatusItem.text += ` ${this.PowerShellExeDetails.displayName}`;
this.languageStatusItem.detail += ` at '${this.PowerShellExeDetails.exePath}'`;
} else if (this.sessionSettings.powerShellDefaultVersion) { // When it hasn't been found yet.
this.languageStatusItem.text += ` ${this.sessionSettings.powerShellDefaultVersion}`;
this.languageStatusItem.detail = `Looking for '${this.sessionSettings.powerShellDefaultVersion}'...`;
}

if (detail) {
Expand Down Expand Up @@ -877,6 +853,11 @@ Type 'help' to get help.
}
}

// Refreshes the Language Status Item details with ehe same status.
private refreshSessionStatus(): void {
this.setSessionStatus("", this.sessionStatus);
}

private setSessionRunningStatus(): void {
this.setSessionStatus("", SessionStatus.Running);
}
Expand Down
2 changes: 0 additions & 2 deletions src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class PartialSettings { }
export class Settings extends PartialSettings {
powerShellAdditionalExePaths: PowerShellAdditionalExePathSettings = {};
powerShellDefaultVersion = "";
// This setting is no longer used but is here to assist in cleaning up the users settings.
powerShellExePath = "";
promptToUpdatePowerShell = true;
suppressAdditionalExeNotFoundWarning = false;
startAsLoginShell = new StartAsLoginShellSettings();
Expand Down