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

Fix preview command sometimes not executing on Windows #81

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

RickLuiken
Copy link
Contributor

This PR attempts to fix the preview command on Windows. As seen in this video, sometimes checkpoint_paste() is not executed in the IPython shell, but it triggers the multiline input in IPython. In this PR, I detect the multiline input ("...:") and send "\x7F" to clear the extra line and send another newline to actually execute the command. This seems to fix the issue on my side. Let me know if you have any issues on other platforms!

Fixes #18

@Splines
Copy link
Member

Splines commented Nov 4, 2024

Nice, thanks! I will check if this still works for me. Generally, I'd favor a solution where we don't have to additionally detect the ...: string as that is of course one more thing where it could break in the future. So I assume the commands proposed in my comment here didn't work for you?

src/manimShell.ts Outdated Show resolved Hide resolved
@RickLuiken
Copy link
Contributor Author

I would also want to find a better solution than this. However, others have run into the same issue on the IPython repo and the vscode-python repo, so it does not seem to have an easy fix.

This solution should work out of the box for new users. Adding a delay between writing the command and the newline requires users to tweak the delay value.

Copy link
Member

@Splines Splines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me, just a few more minor comments.

src/manimShell.ts Outdated Show resolved Hide resolved
src/manimShell.ts Show resolved Hide resolved
src/manimShell.ts Show resolved Hide resolved
src/previewCode.ts Outdated Show resolved Hide resolved
@Splines
Copy link
Member

Splines commented Nov 4, 2024

This solution should work out of the box for new users. Adding a delay between writing the command and the newline requires users to tweak the delay value.

Yeah, this solution is definitely better than adding a delay. I just hope there will be a solution that gets to the root of this problem by means of a fix in the IPython repo or somewhere else where something like this would be tackled. But for now, if this fixes your problem that's already great and luckily this is also a fix that might be easily revertible once a solution is found in the future.

@RickLuiken
Copy link
Contributor Author

Sadly, I tried again with shell integration and this time it broke again... After some debugging I found that it thinks that there is a command running after sending the checkpoint_paste(). So, when sending the extra newline, it will first send a CTRL+C to stop the first command, deleting the checkpoint_paste() line. I haven't been able to break the sendText method, so let's stick to that for now.

@Splines
Copy link
Member

Splines commented Nov 5, 2024

Great, thanks for addressing all the points. The code looks good to me now. I just want to test this out a bit on my machine and ensure it doesn't break anything for me (we definitely need unit and integration tests in the very near future🙈, see #47). I will probably only be able to report on Friday as I'm a bit busy beforehand.

Copy link
Member

@Splines Splines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've tested it on my machine and it works fine. I've also checked that users can still enter the multi-line mode, e.g. when they manually type in a for-loop in the IPython shell. It works since you check for this.isExecutingCommand 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

executeCommand() doesn't execute the checkpoint_paste() command
3 participants