Notes about activation

Author: Tyriar

src/features/terminal/shells/bash/bashStartup.ts

@@ -70,10 +70,22 @@ async function isStartupSetup(profile: string, key: string): Promise<ShellSetupS
 }
 async function setupStartup(profile: string, key: string, name: string): Promise<boolean> {
     const shellIntegrationEnabled = await getShellIntegrationEnabledCache();
-    if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(name, profile))) && !isWsl()) {
+    // shellIntegrationEnabled should not be checked here, consider a shell setup where it's
+    // incompatible with our shell integration. `terminal.integrated.shellIntegration.enabled` sets
+    // whether we try to inject the script, not whether it will work. `.attemptToInject` would be a
+    // more accurate name for this.
+    //
+    // Doing it this way would handle these cases:
+    // - Inject failed (.shellIntegration won't be there)
+    // - Inject failed, but it was installed manually (.shelIntegration will be there)
+    if (await shellIntegrationForActiveTerminal(name, profile) && !isWsl()) {
         removeStartup(profile, key);
         return true;
     }
+
+    // We may have activated here, but not have command detection. Eg. shell integration injects,
+    // env activates, p10k disables our shell integration; we have activated, but don't know in the
+    // extension.
     const activationContent = getActivationContent(key);
     try {
         if (await fs.pathExists(profile)) {
@@ -90,6 +102,8 @@ async function setupStartup(profile: string, key: string, name: string): Promise
             traceInfo(`SHELL: Created new ${name} profile at: ${profile}\n${activationContent}`);
         }
 
+        // If this fails for the first time, do we need a notification here telling the user to restart the terminal?
+
         return true;
     } catch (err) {
         traceError(`SHELL: Failed to setup startup for profile at: ${profile}`, err);

src/features/terminal/shells/common/shellUtils.ts

@@ -108,6 +108,17 @@ export async function shellIntegrationForActiveTerminal(name: string, profile?:
     let hasShellIntegration = activeTerminalShellIntegration();
     let timeout = 0;
 
+    // Should use onDidChangeTerminalShellIntegration event instead of polling
+    // const result = Promise.race([
+    //   // Make sure event listener is disposed
+    //   new Promise(r => window.onDidChangeTerminalShellIntegration(() => r))),
+    //   // Note that you might not know when terminal started up, but this should be (fromNewTimeoutSetting - terminalOpened)
+    //   // It's important that the time the terminal is laucnhed is recorded and used, otherwise you might end up waiting 5s*2 if called twice
+    //   // terminalOpened = onDidOpenTerminal event time, or extension startup time (as it was already there)
+    //   timeout(fromNewTimeoutSetting)
+    // ]);
+    // // check result
+    // 500ms isn't long enough
     while (!hasShellIntegration && timeout < SHELL_INTEGRATION_TIMEOUT) {
         await sleep(SHELL_INTEGRATION_POLL_INTERVAL);
         timeout += SHELL_INTEGRATION_POLL_INTERVAL;
@@ -134,7 +145,11 @@ export function isWsl(): boolean {
 }
 
 export async function getShellIntegrationEnabledCache(): Promise<boolean> {
-    const persistentState = await getGlobalPersistentState();
+    // Cache doesn't do anything?
+    // const persistentState = await getGlobalPersistentState();
+    // if (persistentState.get(SHELL_INTEGRATION_STATE_KEY)) {
+    //     return
+    // }
     const shellIntegrationInspect =
         getConfiguration('terminal.integrated').inspect<boolean>('shellIntegration.enabled');
 
@@ -158,6 +173,6 @@ export async function getShellIntegrationEnabledCache(): Promise<boolean> {
         }
     }
 
-    await persistentState.set(SHELL_INTEGRATION_STATE_KEY, shellIntegrationEnabled);
+    // await persistentState.set(SHELL_INTEGRATION_STATE_KEY, shellIntegrationEnabled);
     return shellIntegrationEnabled;
 }

src/features/terminal/terminalManager.ts

@@ -175,6 +175,7 @@ export class TerminalManagerImpl implements TerminalManager {
                             )}`,
                         );
 
+                        // TODO: This needs the same treatment as in setupStartup (bashStartup.ts)
                         if (
                             (shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(p.name))) &&
                             !isWsl()