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

Ask user before killing an active scene #42

Merged
merged 43 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
534de31
Init new ManimShell wrapper
Splines Oct 25, 2024
7c86072
Include ManimShell as additional layer in other commands
Splines Oct 25, 2024
7436cb0
Wait until command in IPython Terminal is finished
Splines Oct 25, 2024
34046ab
Detect shell exit
Splines Oct 25, 2024
6411241
Open terminal upon error
Splines Oct 25, 2024
5511f5d
Improve docstrings & implement executeCommandEnsureActiveSession
Splines Oct 25, 2024
1a06808
Add docstrings for `exec` method
Splines Oct 25, 2024
1ed9546
Add docstrings to `retrieveOrInitActiveShell`
Splines Oct 25, 2024
70aabd5
Add docstrings for `initiateTerminalDataReading`
Splines Oct 25, 2024
ab096bd
Add docstrings for ANSI filtering
Splines Oct 25, 2024
8f8d254
Add docstrings to regular expressions
Splines Oct 25, 2024
d426004
Remove log line
Splines Oct 25, 2024
576d1a4
Fix wrong variable usage
Splines Oct 25, 2024
d1f30d8
Simplify overly complicated Promises/callbacks
Splines Oct 25, 2024
50181e9
Explain only divergence of notation
Splines Oct 25, 2024
75c519f
Don't carry optional parameter startLine around
Splines Oct 25, 2024
7c083c4
Simplify execute command waiting
Splines Oct 25, 2024
d75681a
Move resetActiveShell method down
Splines Oct 25, 2024
6c15e6e
Extract method hasActiveShell()
Splines Oct 25, 2024
e5d1576
Exit scene if start scene is invoked while scene is running
Splines Oct 25, 2024
6edb693
Make method private
Splines Oct 25, 2024
947dc7a
Import Terminal directly
Splines Oct 25, 2024
93f7560
Clarify what ManimShell is
Splines Oct 25, 2024
cc81d62
Add docstring for detectShellExecutionEnd
Splines Oct 25, 2024
5321eaa
Fix typos in docstrings
Splines Oct 25, 2024
d035d69
Lock new command execution during startup
Splines Oct 25, 2024
a5f86e9
Don't be stingy with empty lines
Splines Oct 25, 2024
835e8b6
Avoid orphan VSCode terminals upon scene start
Splines Oct 25, 2024
896cc5e
Merge branch 'main' into feature/manim-shell
Splines Oct 25, 2024
f6045be
Remove unnecessary statements
Splines Oct 25, 2024
bd40282
Listen to terminal data right from the beginning
Splines Oct 25, 2024
4d41027
Fix clipboard buffering
Splines Oct 25, 2024
6cad6ec
Prevent multiple commands from running on MacOS
Splines Oct 26, 2024
072fa74
Remove console log
Splines Oct 26, 2024
715e9fd
Update MacOS error message
bhoov Oct 27, 2024
65ed0c1
(fix MacOS typo)
bhoov Oct 27, 2024
cc725ba
Merge branch 'main' into fix/overlapping-commands
Splines Oct 27, 2024
6cec111
Wrap callback in interface (for future PR)
Splines Oct 27, 2024
1ca9fbc
Refactor execute command methods (merge into one)
Splines Oct 27, 2024
d20d947
Throw error if no active session found (but required)
Splines Oct 27, 2024
0116318
Remove unnecessary constructor
Splines Oct 27, 2024
4166dbd
Ask user before killing an active scene
Splines Oct 27, 2024
62d2f7b
Allow user to dismiss & re-enable confirmation
Splines Oct 27, 2024
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
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@
"type": "number",
"default": 650,
"markdownDescription": "Configures the number of milliseconds (ms) to wait before your clipboard is restored. (Your clipboard is used to temporarily copy the selected code to be accessible by Manim)."
},
"manim-notebook.confirmKillingActiveSceneToStartNewOne": {
"type": "boolean",
"default": true,
"markdownDescription": "If enabled, you will be prompted to confirm killing an old session when you want to start a new scene at the cursor while an active scene is running."
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,10 @@ function previewSelection() {
* the scene.
*/
async function clearScene() {
const success = await ManimShell.instance.executeCommandEnsureActiveSession("clear()");
if (!success) {
window.showErrorMessage('No active ManimGL scene found to remove objects from.');
try {
await ManimShell.instance.executeCommandErrorOnNoActiveSession("clear()");
} catch (NoActiveSessionError) {
window.showErrorMessage('No active Manim session found to remove objects from.');
}
}

Expand Down
191 changes: 157 additions & 34 deletions src/manimShell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,24 @@ enum ManimShellEvent {
IPYTHON_CELL_FINISHED = 'ipythonCellFinished',
}

/**
* Event handler for command execution events.
*/
export interface CommandExecutionEventHandler {
/**
* Callback that is invoked when the command is issued, i.e. sent to the
* terminal. At this point, the command is probably not yet finished
* executing.
*/
onCommandIssued?: () => void;
}

/**
* Error thrown when no active shell is found, but an active shell is required
* for the command execution.
*/
export class NoActiveShellError extends Error { }

/**
* Wrapper around the IPython terminal that ManimGL uses. Ensures that commands
* are executed at the right place and spans a new Manim session if necessary.
Expand All @@ -61,6 +79,15 @@ export class ManimShell {
*/
private lockDuringStartup = false;

/**
* Whether to lock the execution of a new command while another command is
* currently running. On MacOS, we do lock since the IPython terminal *exits*
* when sending Ctrl+C instead of just interrupting the current command.
* See issue #16: https://github.com/bhoov/manim-notebook/issues/16
*/
private shouldLockDuringCommandExecution = false;
private isExecutingCommand = false;

/**
* Whether to detect the end of a shell execution.
*
Expand All @@ -78,6 +105,11 @@ export class ManimShell {

private constructor() {
this.initiateTerminalDataReading();

// on MacOS
if (process.platform === "darwin") {
this.shouldLockDuringCommandExecution = true;
}
}

public static get instance(): ManimShell {
Expand All @@ -88,60 +120,119 @@ export class ManimShell {
}

/**
* Executes the given command in a VSCode terminal. If no active terminal
* running Manim is found, a new terminal is spawned, and a new Manim
* session is started in it before executing the given command.
* Executes the given command. If no active terminal running Manim is found,
* a new terminal is spawned, and a new Manim session is started in it
* before executing the given command.
*
* This command is locked during startup to prevent multiple new scenes from
* being started at the same time, see `lockDuringStartup`.
*
* For params explanations, see the docs for `execCommand()`.
*/
public async executeCommand(
command: string, startLine: number, waitUntilFinished = false,
handler?: CommandExecutionEventHandler
) {
await this.execCommand(
command, waitUntilFinished, false, false, startLine, handler);
}

/**
* Executes the given command, but only if an active ManimGL shell exists.
* Otherwise throws a `NoActiveShellError`.
*
* For params explanations, see the docs for `execCommand()`.
* @throws NoActiveShellError If no active shell is found.
*/
public async executeCommandErrorOnNoActiveSession(
command: string, waitUntilFinished = false, forceExecute = false
) {
await this.execCommand(
command, waitUntilFinished, forceExecute, true, undefined, undefined);
}

/**
* Executes a given command and bundles many different behaviors and options.
*
* This method is internal and only exposed via other public methods that
* select a specific behavior.
*
* @param command The command to execute in the VSCode terminal.
* @param waitUntilFinished Whether to wait until the actual command has
* finished executing, e.g. when the whole animation has been previewed.
* If set to false (default), we only wait until the command has been issued,
* i.e. sent to the terminal.
* @param forceExecute Whether to force the execution of the command even if
* another command is currently running. This is only taken into account
* when the `shouldLockDuringCommandExecution` is set to true.
* @param errorOnNoActiveShell Whether to execute the command only if an
* active shell exists. If no active shell is found, an error is thrown.
* @param startLine The line number in the active editor where the Manim
* session should start in case a new terminal is spawned.
* Also see `startScene()`.
* @param [waitUntilFinished=false] Whether to wait until the actual command
* has finished executing, e.g. when the whole animation has been previewed.
* Also see `startScene(). You MUST set a startLine if `errorOnNoActiveShell`
* is set to false, since the method might invoke a new shell in this case
* and needs to know at which line to start it.
* @param handler Event handler for command execution events. See the
* interface `CommandExecutionEventHandler`.
*
* @throws NoActiveShellError If no active shell is found, but an active
* shell is required for the command execution (when `errorOnNoActiveShell`
* is set to true).
*/
public async executeCommand(command: string, startLine: number, waitUntilFinished = false) {
if (this.lockDuringStartup) {
return vscode.window.showWarningMessage("Manim is currently starting. Please wait a moment.");
private async execCommand(
command: string,
waitUntilFinished: boolean,
forceExecute: boolean,
errorOnNoActiveShell: boolean,
startLine?: number,
handler?: CommandExecutionEventHandler
) {
if (!errorOnNoActiveShell && startLine === undefined) {
// should never happen if method is called correctly
window.showErrorMessage("Start line not set. Internal extension error.");
return;
}

this.lockDuringStartup = true;
const shell = await this.retrieveOrInitActiveShell(startLine);
this.lockDuringStartup = false;
if (this.lockDuringStartup) {
window.showWarningMessage("Manim is currently starting. Please wait a moment.");
return;
}

this.exec(shell, command);
if (errorOnNoActiveShell && !this.hasActiveShell()) {
throw new NoActiveShellError();
}

if (waitUntilFinished) {
await new Promise(resolve => {
this.eventEmitter.once(ManimShellEvent.IPYTHON_CELL_FINISHED, resolve);
});
if (this.shouldLockDuringCommandExecution && !forceExecute && this.isExecutingCommand) {
window.showWarningMessage(
`Simultaneous Manim commands are not currently supported on MacOS. `
+ `Please wait for the current operations to finish before initiating `
+ `a new command.`);
return;
}
}

/**
* Executes the given command, but only if an active ManimGL shell exists.
*
* @param command The command to execute in the VSCode terminal.
* @param [waitUntilFinished=false] Whether to wait until the actual command
* has finished executing, e.g. when the whole animation has been previewed.
* @returns A boolean indicating whether an active shell was found or not.
* If no active shell was found, the command was also not executed.
*/
public async executeCommandEnsureActiveSession(
command: string, waitUntilFinished = false): Promise<boolean> {
if (!this.hasActiveShell()) {
return Promise.resolve(false);
this.isExecutingCommand = true;

let shell: Terminal;
if (errorOnNoActiveShell) {
shell = this.activeShell as Terminal;
} else {
this.lockDuringStartup = true;
shell = await this.retrieveOrInitActiveShell(startLine!);
this.lockDuringStartup = false;
}

this.exec(this.activeShell as Terminal, command);
this.exec(shell, command);
handler?.onCommandIssued?.();

if (waitUntilFinished) {
await new Promise(resolve => {
this.eventEmitter.once(ManimShellEvent.IPYTHON_CELL_FINISHED, resolve);
});
}
return Promise.resolve(true);

this.eventEmitter.once(ManimShellEvent.IPYTHON_CELL_FINISHED, () => {
this.isExecutingCommand = false;
});
}

/**
Expand All @@ -164,8 +255,14 @@ export class ManimShell {
public async executeStartCommand(command: string, isRequestedForAnotherCommand: boolean) {
if (!isRequestedForAnotherCommand) {
if (this.hasActiveShell()) {
const shouldAsk = await vscode.workspace.getConfiguration("manim-notebook")
.get("confirmKillingActiveSceneToStartNewOne");
if (shouldAsk) {
if (!await this.doesUserWantToKillActiveScene()) {
return;
}
}
exitScene();
await new Promise(resolve => setTimeout(resolve, 2000));
}
this.activeShell = window.createTerminal();
}
Expand All @@ -185,6 +282,32 @@ export class ManimShell {
this.activeShell = null;
}

/**
* 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.
*
* @returns true if the user wants to kill the active scene, false otherwise.
*/
private async doesUserWantToKillActiveScene(): Promise<boolean> {
const CANCEL_OPTION = "Cancel";
const KILL_IT_ALWAYS_OPTION = "Kill it (don't ask the next time)";

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 || selection === CANCEL_OPTION) {
return false;
}

if (selection === KILL_IT_ALWAYS_OPTION) {
await vscode.workspace.getConfiguration("manim-notebook")
.update("confirmKillingActiveSceneToStartNewOne", false);
window.showInformationMessage(
"You can re-enable the confirmation in the settings.");
}
return true;
}

/**
* Returns whether an active shell exists, i.e. a terminal that has an
* active ManimGL IPython session running.
Expand Down
44 changes: 14 additions & 30 deletions src/previewCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,6 @@ const PREVIEW_COMMAND = `\x0C checkpoint_paste()\x1b`;
// \x0C: is Ctrl + L
// \x1b: https://github.com/bhoov/manim-notebook/issues/18#issuecomment-2431146809

/**
* Whether the extension is currently executing a Manim command.
*
* Note that this is not capturing whether the `checkpoint_paste()` command is
* still running. Instead, it only captures whether reading/writing to clipboard
* is currently happening to prevent unpredictable behavior.
*
* We don't need to capture the state of `checkpoint_paste()` because users
* might actually want to preview a cell (or another one) again from the start
* even though the animation is still running. With the new VSCode terminal
* shell integration, it will automatically send a `Ctrl + C` to the terminal
* when a new command is sent, so the previous command will be interrupted.
*/
let isExecuting = false;

/**
* Interactively previews the given Manim code by means of the
* `checkpoint_paste()` method from Manim.
Expand All @@ -37,26 +22,25 @@ let isExecuting = false;
* @param code The code to preview (e.g. from a Manim cell or from a custom selection).
*/
export async function previewCode(code: string, startLine: number): Promise<void> {
if (isExecuting) {
vscode.window.showInformationMessage('Please wait a few seconds, then try again.');
return;
}
isExecuting = true;

try {
const clipboardBuffer = await vscode.env.clipboard.readText();
await vscode.env.clipboard.writeText(code);

await ManimShell.instance.executeCommand(PREVIEW_COMMAND, startLine);

// Restore original clipboard content
const timeout = vscode.workspace.getConfiguration("manim-notebook").clipboardTimeout;
setTimeout(async () => {
await vscode.env.clipboard.writeText(clipboardBuffer);
}, timeout);
await ManimShell.instance.executeCommand(
PREVIEW_COMMAND, startLine, true, {
onCommandIssued: () => {
restoreClipboard(clipboardBuffer);
}
}
);
} catch (error) {
vscode.window.showErrorMessage(`Error: ${error}`);
} finally {
isExecuting = false;
}
}

function restoreClipboard(clipboardBuffer: string) {
const timeout = vscode.workspace.getConfiguration("manim-notebook").clipboardTimeout;
setTimeout(async () => {
await vscode.env.clipboard.writeText(clipboardBuffer);
}, timeout);
}
9 changes: 4 additions & 5 deletions src/startStopScene.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,9 @@ export async function startScene(lineStart?: number) {
* and the IPython terminal.
*/
export async function exitScene() {
const success = await ManimShell.instance.executeCommandEnsureActiveSession("exit()");
if (success) {
ManimShell.instance.resetActiveShell();
} else {
window.showErrorMessage('No active ManimGL scene found to exit.');
try {
await ManimShell.instance.executeCommandErrorOnNoActiveSession("exit()", false, true);
} catch(NoActiveSessionError) {
window.showErrorMessage('No active Manim session found to exit.');
}
}