search

Manim-Notebook / manim-notebook

Simple commands to replicate the manim dev workflow in VSCode
MIT License
5 stars 0 forks source link

Prevent overlapping commands (MacOS) & refactor `execCommand()` #32

Closed Splines closed 5 days ago

Splines commented 6 days ago

Closes #16.

What we'd love to have is this behavior that works on Windows

https://github.com/user-attachments/assets/1328d465-fd7f-4832-b5ca-4bfb5239f7f5

Unfortunately, this does not seem to work on MacOS, see #16. When requesting a new command, Ctrl+C is sent to the IPython shell and then the new command is executed. On MacOS, the Ctrl+C actually closes the whole shell with an error message (see #16). This is not an issue with this extension but rather with some underlaying layers like Manim and/or IPython.

Additionally, this PR also refactors the execCommand() method, sorry that this is intermingled with the core change of this PR. But otherwise the two exec methods would quickly become a mess, which became apparent in the next PR (#33). This is because the existing exec methods share most of their behavior and only differ in some aspects. So I've introduced an internal execCommand() method that is very flexible and is exposed to the outside by calling it with the desired set of parameters.

We also wrap the onCommandIssued() callback inside a CommandExecutionEventHandler, which is going to be extended by a onData() callback in #33. This is just to reduce the burden of having to specify each callback as single method parameter (they already suffer a bit from many parameters).

bhoov commented 6 days ago

@Splines this PR is still specified as a "draft", but it seems like you have completed all tasks -- may I begin a review?

bhoov commented 6 days ago

OMG. The quality of life improves even further now that the computer stops me from accidentally killing a scene I am rendering. Thank you @splines.

I'm making a small change to the error message for MacOS since the "above the fold" (what users see) before expanding is uninformative. This is what I see:

image

Changing to:

image
bhoov commented 6 days ago

Overall, things look great to me. I'm ready to approve this PR when all conflicts with main are resolved

Splines commented 5 days ago

this PR is still specified as a "draft", but it seems like you have completed all tasks

Yes it is still in draft as I wanted to do another round of refactoring after #30 is merged. Now that this is the case, I will fix conflicts with main, improve the code and then open it for review.

Splines commented 5 days ago

When squashing and merging, please make sure to only include the commits starting with 6cad6ec (Prevent multiple commands from running on MacOS) in the commit message. This is again an annoying GitHub limitation.

And note that I've appended some text to the PR comment about refactoring.