Fix exit scene behavior (when exit is anticipated) (#48)

Splines · web-flow · commit c9d35c68097e · 2024-10-31T19:34:41.000+01:00
* Detect errors on Manim startup * Wait for exit * Revert "Wait for exit" This reverts commit d6e3916. * Only react to data in active shell and reset on exit * Fix stale progress notification & improve exit mechanism * Remove leftover error class * Add comment explaining early return * Replace return by continue 🙄 * Remove left-over line from merge conflict * Add debug statement back in * Add two more debug statements * Add warn statement when selection is undefined * Propagate async nature of notification messages to caller * Add missing return * Add debug statement to hasActiveShell * Fix try-catch error handling * Rework exit mechanism (force-quit) * Add comment explaining supplementary reset * Send additional keyboard interrupt before quitting scene * Store temp reference to last active shell
diff --git a/src/extension.ts b/src/extension.ts
@@ -1,6 +1,6 @@
 import * as vscode from 'vscode';
 import { window } from 'vscode';
-import { ManimShell } from './manimShell';
+import { ManimShell, NoActiveShellError } from './manimShell';
 import { ManimCell } from './manimCell';
 import { ManimCellRanges } from './manimCellRanges';
 import { previewCode } from './previewCode';
@@ -152,8 +152,13 @@ async function previewSelection() {
 async function clearScene() {
 	try {
 		await ManimShell.instance.executeCommandErrorOnNoActiveSession("clear()");
-	} catch (NoActiveSessionError) {
-		Window.showErrorMessage('No active Manim session found to remove objects from.');
+	} catch (error) {
+		if (error instanceof NoActiveShellError) {
+			Window.showErrorMessage('No active Manim session found to remove objects from.');
+		} else {
+			Logger.error(`💥 Error while trying to remove objects from scene: ${error}`);
+			throw error;
+		}
 	}
 }
 
diff --git a/src/logger.ts b/src/logger.ts
@@ -161,19 +161,19 @@ export class Logger {
  */
 export class Window {
 
-    public static showInformationMessage(message: string) {
+    public static async showInformationMessage(message: string) {
         Logger.info(`💡 ${message}`);
-        window.showInformationMessage(message);
+        return await window.showInformationMessage(message);
     }
 
-    public static showWarningMessage(message: string, ...items: string[]) {
+    public static async showWarningMessage(message: string, ...items: string[]) {
         Logger.warn(`💡 ${message}`);
-        window.showWarningMessage(message, ...items);
+        return await window.showWarningMessage(message, ...items);
     }
 
-    public static showErrorMessage(message: string) {
+    public static async showErrorMessage(message: string) {
         Logger.error(`💡 ${message}`);
-        window.showErrorMessage(message);
+        return await window.showErrorMessage(message);
     }
 }
 
diff --git a/src/manimShell.ts b/src/manimShell.ts
@@ -59,7 +59,12 @@ enum ManimShellEvent {
      * execution has ended before we have detected the start of the ManimGL
      * session.
      */
-    MANIM_NOT_STARTED = 'manimglNotStarted'
+    MANIM_NOT_STARTED = 'manimglNotStarted',
+
+    /**
+     * Event emitted when the active shell is reset.
+     */
+    RESET = 'reset'
 }
 
 /**
@@ -81,6 +86,12 @@ export interface CommandExecutionEventHandler {
      * of ANSI control codes.
      */
     onData?: (data: string) => void;
+
+    /**
+     * Callback that is invoked when the manim shell is reset. This indicates
+     * that the event handler should clean up any resources.
+     */
+    onReset?: () => void;
 }
 
 /**
@@ -263,6 +274,8 @@ export class ManimShell {
 
         this.isExecutingCommand = true;
         Logger.debug("🔒 Command execution locked");
+        const resetListener = () => { handler?.onReset?.(); };
+        this.eventEmitter.on(ManimShellEvent.RESET, resetListener);
 
         let shell: Terminal;
         if (errorOnNoActiveShell) {
@@ -285,6 +298,7 @@ export class ManimShell {
             Logger.debug("🔓 Command execution unlocked");
             this.isExecutingCommand = false;
             this.eventEmitter.off(ManimShellEvent.DATA, dataListener);
+            this.eventEmitter.off(ManimShellEvent.RESET, resetListener);
         });
         if (waitUntilFinished) {
             Logger.debug(`🕒 Waiting until command has finished: ${command}`);
@@ -317,12 +331,14 @@ export class ManimShell {
                 const shouldAsk = await vscode.workspace.getConfiguration("manim-notebook")
                     .get("confirmKillingActiveSceneToStartNewOne");
                 if (shouldAsk) {
+                    Logger.debug("🔆 Active shell found, asking user to kill active scene");
                     if (!await this.doesUserWantToKillActiveScene()) {
+                        Logger.debug("🔆 User didn't want to kill active scene");
                         return;
                     }
                 }
                 Logger.debug("🔆 User confirmed to kill active scene");
-                exitScene();
+                await this.forceQuitActiveShell();
             }
             this.activeShell = window.createTerminal();
         } else {
@@ -361,12 +377,39 @@ export class ManimShell {
         this.iPythonCellCount = 0;
         this.activeShell = null;
         this.shellWeTryToSpawnIn = null;
+        this.eventEmitter.emit(ManimShellEvent.RESET);
         this.eventEmitter.removeAllListeners();
     }
 
+    /**
+     * Forces the terminal to quit and thus the ManimGL session to end.
+     * 
+     * Beforehand, this was implemented more gracefully by sending a keyboard
+     * interrupt (`Ctrl+C`), followed by the `exit()` command in the IPython
+     * session. However, on MacOS, the keyboard interrupt itself already exits
+     * from the entire IPython session and does not just interrupt the current
+     * running command (inside IPython) as would be expected.
+     * See https://github.com/3b1b/manim/discussions/2236
+     */
+    public async forceQuitActiveShell() {
+        if (this.activeShell) {
+            Logger.debug("🔚 Force-quitting active shell");
+            let lastActiveShell = this.activeShell;
+            await this.sendKeyboardInterrupt();
+            lastActiveShell?.dispose();
+            // This is also taken care of when we detect that the shell has ended
+            // in the `onDidEndTerminalShellExecution` event handler. However,
+            // it doesn't harm to reset the active shell here as well just to
+            // be sure.
+            this.resetActiveShell();
+        } else {
+            Logger.debug("🔚 No active shell found to force quit");
+        }
+    }
+
     /**
      * Ask the user if they want to kill the active scene. Might modify the
-     * setting that control if the user should be asked in the future.
+     * setting that controls if the user should be asked in the future.
      * 
      * @returns true if the user wants to kill the active scene, false otherwise.
      */
@@ -377,6 +420,9 @@ export class ManimShell {
         const selection = await Window.showWarningMessage(
             "We need to kill your Manim session to spawn a new one.",
             "Kill it", KILL_IT_ALWAYS_OPTION, CANCEL_OPTION);
+        if (selection === undefined) {
+            Logger.warn("❌ User selection undefined, but shouldn't be");
+        }
         if (selection === undefined || selection === CANCEL_OPTION) {
             return false;
         }
@@ -398,7 +444,10 @@ export class ManimShell {
      * Manim session (IPython environment), is considered inactive.
      */
     private hasActiveShell(): boolean {
-        return this.activeShell !== null && this.activeShell.exitStatus === undefined;
+        const hasActiveShell =
+            this.activeShell !== null && this.activeShell.exitStatus === undefined;
+        Logger.debug(`👩‍💻 Has active shell?: ${hasActiveShell}`);
+        return hasActiveShell;
     }
 
     /**
@@ -492,7 +541,8 @@ export class ManimShell {
 
     private async sendKeyboardInterrupt() {
         Logger.debug("💨 Sending keyboard interrupt to terminal");
-        this.activeShell?.sendText('\x03'); // send `Ctrl+C`
+        await this.activeShell?.sendText('\x03'); // send `Ctrl+C`
+        await new Promise(resolve => setTimeout(resolve, 250));
     }
 
     /**
@@ -514,17 +564,26 @@ export class ManimShell {
                 for await (const data of withoutAnsiCodes(stream)) {
                     Logger.trace(`🧾 Terminal data:\n${data}`);
 
-                    this.eventEmitter.emit(ManimShellEvent.DATA, data);
-
                     if (data.match(MANIM_WELCOME_REGEX)) {
+                        // Manim detected in new terminal
                         if (this.activeShell && this.activeShell !== event.terminal) {
                             Logger.debug("👋 Manim detected in new terminal, exiting old scene");
-                            exitScene();
+                            await this.forceQuitActiveShell();
                         }
                         Logger.debug("👋 Manim welcome string detected");
                         this.activeShell = event.terminal;
                     }
 
+                    // Subsequent data handling should only occur for the
+                    // currently active shell. This is important during
+                    // overlapping commands, e.g. when one shell is exited
+                    // and another one started.
+                    if (this.activeShell !== event.terminal) {
+                        continue;
+                    }
+
+                    this.eventEmitter.emit(ManimShellEvent.DATA, data);
+
                     if (data.match(KEYBOARD_INTERRUPT_REGEX)) {
                         Logger.debug("🛑 Keyboard interrupt detected");
                         this.eventEmitter.emit(ManimShellEvent.KEYBOARD_INTERRUPT);
diff --git a/src/previewCode.ts b/src/previewCode.ts
@@ -41,6 +41,9 @@ export async function previewCode(code: string, startLine: number): Promise<void
             },
             onData: (data) => {
                 progress?.reportOnData(data);
+            },
+            onReset: () => {
+                progress?.finish();
             }
         });
     } finally {
diff --git a/src/startStopScene.ts b/src/startStopScene.ts
@@ -1,5 +1,5 @@
 import * as vscode from 'vscode';
-import { ManimShell } from './manimShell';
+import { ManimShell, NoActiveShellError } from './manimShell';
 import { window } from 'vscode';
 import { Logger, Window } from './logger';
 
@@ -105,13 +105,11 @@ export async function startScene(lineStart?: number) {
 }
 
 /**
- * Runs the `exit()` command in the terminal to close the animation window
- * and the IPython terminal.
+ * Force-quits the active Manim session by disposing the respective VSCode
+ * terminal that is currently hosting the session.
+ * 
+ * See `forceQuitActiveShell()` for more details.
  */
 export async function exitScene() {
-    try {
-        await ManimShell.instance.executeCommandErrorOnNoActiveSession("exit()", false, true);
-    } catch (NoActiveSessionError) {
-        Window.showErrorMessage('No active Manim session found to exit.');
-    }
+    await ManimShell.instance.forceQuitActiveShell();
 }

Original file line number	Diff line number	Diff line change
`@@ -161,19 +161,19 @@ export class Logger {`
`161`	`161`	`*/`
`162`	`162`	`export class Window {`
`163`	`163`
`164`		`- public static showInformationMessage(message: string) {`
	`164`	`+ public static async showInformationMessage(message: string) {`
`165`	`165`	Logger.info(`💡 ${message}`);
`166`		`- window.showInformationMessage(message);`
	`166`	`+ return await window.showInformationMessage(message);`
`167`	`167`	`}`
`168`	`168`
`169`		`- public static showWarningMessage(message: string, ...items: string[]) {`
	`169`	`+ public static async showWarningMessage(message: string, ...items: string[]) {`
`170`	`170`	Logger.warn(`💡 ${message}`);
`171`		`- window.showWarningMessage(message, ...items);`
	`171`	`+ return await window.showWarningMessage(message, ...items);`
`172`	`172`	`}`
`173`	`173`
`174`		`- public static showErrorMessage(message: string) {`
	`174`	`+ public static async showErrorMessage(message: string) {`
`175`	`175`	Logger.error(`💡 ${message}`);
`176`		`- window.showErrorMessage(message);`
	`176`	`+ return await window.showErrorMessage(message);`
`177`	`177`	`}`
`178`	`178`	`}`
`179`	`179`