Skip to content

Commit

Permalink
Show notification reaffirming Python extension still handles activati…
Browse files Browse the repository at this point in the history
…on when in `pythonTerminalEnvVarActivation` experiment (microsoft#21802)

Closes microsoft#21793

Only show notification when terminal prompt does not already indicate
that env is activated.
  • Loading branch information
Kartik Raj authored Aug 15, 2023
1 parent b447bf1 commit 9c740b9
Show file tree
Hide file tree
Showing 15 changed files with 545 additions and 22 deletions.
4 changes: 2 additions & 2 deletions src/client/activation/extensionSurvey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ export class ExtensionSurveyPrompt implements IExtensionSingleActivationService
@traceDecoratorError('Failed to display prompt for extension survey')
public async showSurvey() {
const prompts = [ExtensionSurveyBanner.bannerLabelYes, ExtensionSurveyBanner.maybeLater, Common.doNotShowAgain];
const telemetrySelections: ['Yes', 'Maybe later', 'Do not show again'] = [
const telemetrySelections: ['Yes', 'Maybe later', "Don't show again"] = [
'Yes',
'Maybe later',
'Do not show again',
"Don't show again",
];
const selection = await this.appShell.showInformationMessage(ExtensionSurveyBanner.bannerMessage, ...prompts);
sendTelemetryEvent(EventName.EXTENSION_SURVEY_PROMPT, undefined, {
Expand Down
6 changes: 6 additions & 0 deletions src/client/common/experiments/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@

'use strict';

import { env, workspace } from 'vscode';
import { IExperimentService } from '../types';
import { TerminalEnvVarActivation } from './groups';
import { isTestExecution } from '../constants';

export function inTerminalEnvVarExperiment(experimentService: IExperimentService): boolean {
if (!isTestExecution() && workspace.workspaceFile && env.remoteName) {
// TODO: Remove this if statement once https://github.com/microsoft/vscode/issues/180486 is fixed.
return false;
}
if (!experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)) {
return false;
}
Expand Down
5 changes: 4 additions & 1 deletion src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export namespace Common {
export const openOutputPanel = l10n.t('Show output');
export const noIWillDoItLater = l10n.t('No, I will do it later');
export const notNow = l10n.t('Not now');
export const doNotShowAgain = l10n.t('Do not show again');
export const doNotShowAgain = l10n.t("Don't show again");
export const reload = l10n.t('Reload');
export const moreInfo = l10n.t('More Info');
export const learnMore = l10n.t('Learn more');
Expand Down Expand Up @@ -198,6 +198,9 @@ export namespace Interpreters {
);
export const activatingTerminals = l10n.t('Reactivating terminals...');
export const activateTerminalDescription = l10n.t('Activated environment for');
export const terminalEnvVarCollectionPrompt = l10n.t(
'The Python extension automatically activates all terminals using the selected environment. You can hover over the terminal tab to see more information about the activation. [Learn more](https://aka.ms/vscodePythonTerminalActivation).',
);
export const activatedCondaEnvLaunch = l10n.t(
'We noticed VS Code was launched from an activated conda environment, would you like to select it?',
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { Uri } from 'vscode';
import { IActiveResourceService, IApplicationShell, ITerminalManager } from '../../common/application/types';
import {
IConfigurationService,
IDisposableRegistry,
IExperimentService,
IPersistentStateFactory,
} from '../../common/types';
import { Common, Interpreters } from '../../common/utils/localize';
import { IExtensionSingleActivationService } from '../../activation/types';
import { ITerminalEnvVarCollectionService } from './types';
import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers';

export const terminalEnvCollectionPromptKey = 'TERMINAL_ENV_COLLECTION_PROMPT_KEY';

@injectable()
export class TerminalEnvVarCollectionPrompt implements IExtensionSingleActivationService {
public readonly supportedWorkspaceTypes = { untrustedWorkspace: false, virtualWorkspace: false };

constructor(
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IPersistentStateFactory) private readonly persistentStateFactory: IPersistentStateFactory,
@inject(ITerminalManager) private readonly terminalManager: ITerminalManager,
@inject(IDisposableRegistry) private readonly disposableRegistry: IDisposableRegistry,
@inject(IActiveResourceService) private readonly activeResourceService: IActiveResourceService,
@inject(ITerminalEnvVarCollectionService)
private readonly terminalEnvVarCollectionService: ITerminalEnvVarCollectionService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IExperimentService) private readonly experimentService: IExperimentService,
) {}

public async activate(): Promise<void> {
if (!inTerminalEnvVarExperiment(this.experimentService)) {
return;
}
this.disposableRegistry.push(
this.terminalManager.onDidOpenTerminal(async (terminal) => {
const cwd =
'cwd' in terminal.creationOptions && terminal.creationOptions.cwd
? terminal.creationOptions.cwd
: this.activeResourceService.getActiveResource();
const resource = typeof cwd === 'string' ? Uri.file(cwd) : cwd;
const settings = this.configurationService.getSettings(resource);
if (!settings.terminal.activateEnvironment) {
return;
}
if (this.terminalEnvVarCollectionService.isTerminalPromptSetCorrectly(resource)) {
// No need to show notification if terminal prompt already indicates when env is activated.
return;
}
await this.notifyUsers();
}),
);
}

private async notifyUsers(): Promise<void> {
const notificationPromptEnabled = this.persistentStateFactory.createGlobalPersistentState(
terminalEnvCollectionPromptKey,
true,
);
if (!notificationPromptEnabled.value) {
return;
}
const prompts = [Common.doNotShowAgain];
const selection = await this.appShell.showInformationMessage(
Interpreters.terminalEnvVarCollectionPrompt,
...prompts,
);
if (!selection) {
return;
}
if (selection === prompts[0]) {
await notificationPromptEnabled.updateValue(false);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ import { Interpreters } from '../../common/utils/localize';
import { traceDecoratorVerbose, traceVerbose } from '../../logging';
import { IInterpreterService } from '../contracts';
import { defaultShells } from './service';
import { IEnvironmentActivationService } from './types';
import { IEnvironmentActivationService, ITerminalEnvVarCollectionService } from './types';
import { EnvironmentType } from '../../pythonEnvironments/info';
import { getSearchPathEnvVarNames } from '../../common/utils/exec';
import { EnvironmentVariables } from '../../common/variables/types';
import { TerminalShellType } from '../../common/terminal/types';
import { OSType } from '../../common/utils/platform';

@injectable()
export class TerminalEnvVarCollectionService implements IExtensionActivationService {
export class TerminalEnvVarCollectionService implements IExtensionActivationService, ITerminalEnvVarCollectionService {
public readonly supportedWorkspaceTypes = {
untrustedWorkspace: false,
virtualWorkspace: false,
Expand Down Expand Up @@ -127,6 +129,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
await this._applyCollection(resource, defaultShell?.shell);
return;
}
await this.trackTerminalPrompt(shell, resource, env);
this.processEnvVars = undefined;
return;
}
Expand All @@ -146,6 +149,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
if (prevValue !== value) {
if (value !== undefined) {
if (key === 'PS1') {
// We cannot have the full PS1 without executing in terminal, which we do not. Hence prepend it.
traceVerbose(`Prepending environment variable ${key} in collection with ${value}`);
envVarCollection.prepend(key, value, {
applyAtShellIntegration: true,
Expand All @@ -165,6 +169,61 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
const displayPath = this.pathUtils.getDisplayName(settings.pythonPath, workspaceFolder?.uri.fsPath);
const description = new MarkdownString(`${Interpreters.activateTerminalDescription} \`${displayPath}\``);
envVarCollection.description = description;

await this.trackTerminalPrompt(shell, resource, env);
}

private isPromptSet = new Map<number | undefined, boolean>();

// eslint-disable-next-line class-methods-use-this
public isTerminalPromptSetCorrectly(resource?: Resource): boolean {
const workspaceFolder = this.getWorkspaceFolder(resource);
return !!this.isPromptSet.get(workspaceFolder?.index);
}

/**
* Call this once we know terminal prompt is set correctly for terminal owned by this resource.
*/
private terminalPromptIsCorrect(resource: Resource) {
const key = this.getWorkspaceFolder(resource)?.index;
this.isPromptSet.set(key, true);
}

private terminalPromptIsUnknown(resource: Resource) {
const key = this.getWorkspaceFolder(resource)?.index;
this.isPromptSet.delete(key);
}

/**
* Tracks whether prompt for terminal was correctly set.
*/
private async trackTerminalPrompt(shell: string, resource: Resource, env: EnvironmentVariables | undefined) {
this.terminalPromptIsUnknown(resource);
if (!env) {
this.terminalPromptIsCorrect(resource);
return;
}
// Prompts for these shells cannot be set reliably using variables
const exceptionShells = [
TerminalShellType.powershell,
TerminalShellType.powershellCore,
TerminalShellType.fish,
TerminalShellType.zsh, // TODO: Remove this once https://github.com/microsoft/vscode/issues/188875 is fixed
];
const customShellType = identifyShellFromShellPath(shell);
if (exceptionShells.includes(customShellType)) {
return;
}
if (this.platform.osType !== OSType.Windows) {
// These shells are expected to set PS1 variable for terminal prompt for virtual/conda environments.
const interpreter = await this.interpreterService.getActiveInterpreter(resource);
const shouldPS1BeSet = interpreter?.type !== undefined;
if (shouldPS1BeSet && !env.PS1) {
// PS1 should be set but no PS1 was set.
return;
}
}
this.terminalPromptIsCorrect(resource);
}

private async handleMicroVenv(resource: Resource) {
Expand All @@ -178,7 +237,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
envVarCollection.replace(
'PATH',
`${path.dirname(interpreter.path)}${path.delimiter}${process.env[pathVarName]}`,
{ applyAtShellIntegration: true },
{ applyAtShellIntegration: true, applyAtProcessCreation: true },
);
return;
}
Expand Down
8 changes: 8 additions & 0 deletions src/client/interpreter/activation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,11 @@ export interface IEnvironmentActivationService {
interpreter?: PythonEnvironment,
): Promise<string[] | undefined>;
}

export const ITerminalEnvVarCollectionService = Symbol('ITerminalEnvVarCollectionService');
export interface ITerminalEnvVarCollectionService {
/**
* Returns true if we know with high certainity the terminal prompt is set correctly for a particular resource.
*/
isTerminalPromptSetCorrectly(resource?: Resource): boolean;
}
12 changes: 9 additions & 3 deletions src/client/interpreter/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types';
import { IServiceManager } from '../ioc/types';
import { EnvironmentActivationService } from './activation/service';
import { TerminalEnvVarCollectionPrompt } from './activation/terminalEnvVarCollectionPrompt';
import { TerminalEnvVarCollectionService } from './activation/terminalEnvVarCollectionService';
import { IEnvironmentActivationService } from './activation/types';
import { IEnvironmentActivationService, ITerminalEnvVarCollectionService } from './activation/types';
import { InterpreterAutoSelectionService } from './autoSelection/index';
import { InterpreterAutoSelectionProxyService } from './autoSelection/proxy';
import { IInterpreterAutoSelectionService, IInterpreterAutoSelectionProxyService } from './autoSelection/types';
Expand Down Expand Up @@ -109,8 +110,13 @@ export function registerTypes(serviceManager: IServiceManager): void {
IEnvironmentActivationService,
EnvironmentActivationService,
);
serviceManager.addSingleton<IExtensionActivationService>(
IExtensionActivationService,
serviceManager.addSingleton<ITerminalEnvVarCollectionService>(
ITerminalEnvVarCollectionService,
TerminalEnvVarCollectionService,
);
serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService);
serviceManager.addSingleton<IExtensionSingleActivationService>(
IExtensionSingleActivationService,
TerminalEnvVarCollectionPrompt,
);
}
3 changes: 2 additions & 1 deletion src/client/pythonEnvironments/info/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
'use strict';

import { Architecture } from '../../common/utils/platform';
import { PythonEnvType } from '../base/info';
import { PythonVersion } from './pythonVersion';

/**
Expand Down Expand Up @@ -85,7 +86,7 @@ export type PythonEnvironment = InterpreterInformation & {
envName?: string;
envPath?: string;
cachedEntry?: boolean;
type?: string;
type?: PythonEnvType;
};

/**
Expand Down
6 changes: 3 additions & 3 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,7 @@ export interface IEventNamePropertyMapping {
tool?: LinterId;
/**
* `select` When 'Select linter' option is selected
* `disablePrompt` When 'Do not show again' option is selected
* `disablePrompt` When "Don't show again" option is selected
* `install` When 'Install' option is selected
*
* @type {('select' | 'disablePrompt' | 'install')}
Expand Down Expand Up @@ -1374,7 +1374,7 @@ export interface IEventNamePropertyMapping {
/**
* `Yes` When 'Yes' option is selected
* `No` When 'No' option is selected
* `Ignore` When 'Do not show again' option is clicked
* `Ignore` When "Don't show again" option is clicked
*
* @type {('Yes' | 'No' | 'Ignore' | undefined)}
*/
Expand Down Expand Up @@ -1571,7 +1571,7 @@ export interface IEventNamePropertyMapping {
/**
* Carries the selection of user when they are asked to take the extension survey
*/
selection: 'Yes' | 'Maybe later' | 'Do not show again' | undefined;
selection: 'Yes' | 'Maybe later' | "Don't show again" | undefined;
};
/**
* Telemetry event sent when starting REPL
Expand Down
2 changes: 1 addition & 1 deletion src/test/activation/extensionSurvey.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ suite('Extension survey prompt - showSurvey()', () => {
platformService.verifyAll();
});

test("Disable prompt if 'Do not show again' option is clicked", async () => {
test('Disable prompt if "Don\'t show again" option is clicked', async () => {
const prompts = [ExtensionSurveyBanner.bannerLabelYes, ExtensionSurveyBanner.maybeLater, Common.doNotShowAgain];
platformService.setup((p) => p.osType).verifiable(TypeMoq.Times.never());
appShell
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => {
expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set');
expect(messagePrompt!.commandPrompts).to.be.deep.equal([
{ prompt: 'Select Python Interpreter', command: cmd },
{ prompt: 'Do not show again', command: cmdIgnore },
{ prompt: "Don't show again", command: cmdIgnore },
]);
});
test('Should not display a message if No Interpreters diagnostic has been ignored', async () => {
Expand Down
12 changes: 6 additions & 6 deletions src/test/common/installer/installer.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,10 @@ suite('Module Installer only', () => {
TypeMoq.It.isAnyString(),
TypeMoq.It.isValue('Install'),
TypeMoq.It.isValue('Select Linter'),
TypeMoq.It.isValue('Do not show again'),
TypeMoq.It.isValue("Don't show again"),
),
)
.returns(async () => 'Do not show again')
.returns(async () => "Don't show again")
.verifiable(TypeMoq.Times.once());
const persistVal = TypeMoq.Mock.ofType<IPersistentState<boolean>>();
let mockPersistVal = false;
Expand Down Expand Up @@ -367,7 +367,7 @@ suite('Module Installer only', () => {
TypeMoq.It.isAnyString(),
TypeMoq.It.isValue('Install'),
TypeMoq.It.isValue('Select Linter'),
TypeMoq.It.isValue('Do not show again'),
TypeMoq.It.isValue("Don't show again"),
),
)
.returns(async () => undefined)
Expand Down Expand Up @@ -864,7 +864,7 @@ suite('Module Installer only', () => {

test('Ensure 3 options for pylint', async () => {
const product = Product.pylint;
const options = ['Select Linter', 'Do not show again'];
const options = ['Select Linter', "Don't show again"];
const productName = ProductNames.get(product)!;

await installer.promptToInstallImplementation(product, resource);
Expand All @@ -875,7 +875,7 @@ suite('Module Installer only', () => {
});
test('Ensure select linter command is invoked', async () => {
const product = Product.pylint;
const options = ['Select Linter', 'Do not show again'];
const options = ['Select Linter', "Don't show again"];
const productName = ProductNames.get(product)!;
when(
appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]),
Expand All @@ -892,7 +892,7 @@ suite('Module Installer only', () => {
});
test('If install button is selected, install linter and return response', async () => {
const product = Product.pylint;
const options = ['Select Linter', 'Do not show again'];
const options = ['Select Linter', "Don't show again"];
const productName = ProductNames.get(product)!;
when(
appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]),
Expand Down
Loading

0 comments on commit 9c740b9

Please sign in to comment.