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 & test Windows bracketed pasting issue #117

Merged
merged 47 commits into from
Jan 30, 2025

Conversation

Splines
Copy link
Member

@Splines Splines commented Jan 19, 2025

Closes #18.

This fixes an annoying Windows bug. See the Wiki for more details. What this PR does is apply the Python patch I presented here. It will create the respective patch files under your site-packages of your Python installation (it also works in virtual environments). Two new files are added, see the Wiki.

TODO:

  • Remove print statements in patch

@Splines
Copy link
Member Author

Splines commented Jan 29, 2025

@mitkonikov At the end, I was just fed up with encoding errors, so I've decided to base64-encode the file and then decode it in Python itself. And finally, it works 💫

Could you please remove the patch files manually in your Python installation that you use when running the extension, as well as inside /tmp/manimVenv/<subfolder>, then:

  • Run the tests
  • Run the extension and test manually via the laggy scene

Hopefully, this will be the last time ;) In the latest commit, I've removed the temporary console.log() statements and ran all tests. And the pipeline is green now ✅

@Splines Splines requested a review from mitkonikov January 29, 2025 22:46
Copy link
Contributor

@mitkonikov mitkonikov left a comment

Choose a reason for hiding this comment

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

LGTM! The Windows tests work locally too! So, to me this is already mergable! And we can move on to testing the Manim --version thing, because this bugs me out and I had to bypass it locally to run the tests properly.

@Splines
Copy link
Member Author

Splines commented Jan 30, 2025

The Windows tests work locally too!

Nice, great to hear that it finally works 🎉. I hope I will never have to patch anything again in my life, this was such an awful dev experience to get to this point with a working patch. Fingers crossed that this closes #18 once and for all and that no quirky workarounds like manually appending escape sequences or adding manual timeouts etc. are needed anymore.

Thanks a lot for helping out @mitkonikov in identifying the causes and providing useful guidance for fixing this.

@Splines Splines merged commit ad76016 into main Jan 30, 2025
4 checks passed
@Splines Splines deleted the fix/prompt-toolkit-recognize-paste branch January 30, 2025 13:41
@mitkonikov
Copy link
Contributor

@Splines You are welcomed, it was a pleasure! Ideally, we should contact Grant and maybe try to one day fix it inside Manim itself (maybe by adding a flag or something, is this possible from inside Manim?). Because this is still a very invasive patch. But, for now, it's all good. ✨

@Splines
Copy link
Member Author

Splines commented Jan 30, 2025

@mitkonikov

we should contact Grant and maybe try to one day fix it inside Manim itself

Your motivation to do so is obviously stronger since you're a Windows users ;) Feel free to open a new issue on the Manim repo.

is this possible from inside Manim?

If we can fix it here in TypeScript, it can also be fixed directly in Manim, yes ;) But in the best case, the underlying issue is fixed instead of deploying patches everywhere. But who knows when this will be the case...

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
2 participants