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

clipboard buffer + several improvements #5

Closed
wants to merge 5 commits into from
Closed

clipboard buffer + several improvements #5

wants to merge 5 commits into from

Conversation

VladimirFokow
Copy link
Collaborator

@VladimirFokow VladimirFokow commented Oct 16, 2024

Addresses #3, and also makes some improvements.

Added steps 1. and 3. Now the order is:

  1. Save clipboard to buffer
  2. Set clipboard to your selection, run the command
  3. After 0.5 sec – restore the original clipboard

Also added:

  • some error checking (e.g. you can't run >1 commands at the same time,
    but it's fast and dirty:
    it only waits for 0.5 sec.
    It doesn't actually detect when the animation has stopped playing! This can be an idea for future improvement)

  • if nothing is selected -> run the whole line where the cursor is

  • if something is selected (e.g. multiple lines) - extend selection to the start and end of respective lines (much more convenient - no need to accurately drag the selection to the edges)

  • after running checkpoint_paste() - the cursor now automatically goes down 1 line (for convenience, if you want to execute lines in order - now there's no need to manually move the cursor down 1 line after each run)

  • added runing cmd+L before checkpoint_paste()

to preserve the current clipboard.

issue: #3
Notes:

- While the command is running, the clipboard will change to your selection, but the original clipboard will be restored after the command finishes running

Also:

- If nothing is selected -> runs the whole line where cursor is

- also added some error checking, e.g. you can't run >1 commands at the same time
@VladimirFokow
Copy link
Collaborator Author

VladimirFokow commented Oct 16, 2024

By the way, on another branch
–– I've added the run_scene command, BUT:

I'm thinking that that change is under CC-BY-NC-SA-4.0 license (so I haven't made a PR from that branch yet),
because it's adapted from repo 3b1b/videos (license discussion)

Maybe this workflow can be re-written, to be free from that license?
Although Grant is saying he's mainly worried about others using "videos, and the assets" and "directly using content" which I'm not doing here, but still bound by that license..


Update (from that discussion) : an adaptation of the workflow itself - can be without the CC license

@bhoov
Copy link
Collaborator

bhoov commented Oct 16, 2024

Neat stuff! I'm currently traveling and I won't be able to look into this until likely early next week, but thank you for the contribution!

@Splines
Copy link
Member

Splines commented Oct 16, 2024

Wow, thanks for the quick implementation. The extension.ts file already gets quite large, maybe it'd be useful to extract the individual commands to separate files for better readability? Maybe there's also some shared behavior between commands that could also be outsourced?

@VladimirFokow
Copy link
Collaborator Author

VladimirFokow commented Oct 16, 2024

The extension.ts file already gets quite large

On the other hand, spreading things over different places will make it harder to follow each one.

extension.ts is straightforward - it just defines commands 1 by 1,
and inside each command you just read linearly from top to bottom to understand 100% what it's doing.

Right now there isn't much overlap,
there may be some shared parts in the future, similar to manim_plugins.py, e.g. send_terminus_command is used in several places:
https://github.com/3b1b/videos/blob/master/sublime_custom_commands/manim_plugins.py

But after all, I don't have strong opinions - I'd support what's fastest for everyone involved.

@Splines
Copy link
Member

Splines commented Oct 16, 2024

On the other hand, spreading things over different places will make it harder to follow each one.

I think quite the contrary is true ;) For me, it's easier to follow if the file is manageable and only contains of the things I need to look at (and no noise) — high cohesion, low coupling and separation of responsibilities. But maybe that's not too much of a problem yet in this early stage of the extension.

Of course, this is up to bhoov to decide in the end. I just think that it would make sense to watch out for this in the long-term run and to generally aim for high code quality right from the start to avoid aggregation of costs later on.

@bhoov
Copy link
Collaborator

bhoov commented Oct 17, 2024

extension.ts is straightforward - it just defines commands 1 by 1, and inside each command you just read linearly from top to bottom to understand 100% what it's doing.

I am in general in favor of this style by @VladimirFokow , provided we all use VSCode's code folding functionality to fold away each individual function that we don't work on, I would prefer to keep everything in one file. The editor ends up looking pretty clean. In your contributions for new functionality, please add a descriptive comment to each new command in a similar fashion

image

@Splines
Copy link
Member

Splines commented Oct 18, 2024

Note that it might be worth to first merge #7, then move the code that is adjusted in this PR (#5) to the respective previewCode() method. merge #8.

@bhoov bhoov mentioned this pull request Oct 21, 2024
Merged
@bhoov bhoov closed this in #10 Oct 21, 2024
@bhoov
Copy link
Collaborator

bhoov commented Oct 21, 2024

Ended up integrating this into my local branch and doing some cleanup. I changed no functionality, works great. Thanks @VladimirFokow !

@VladimirFokow
Copy link
Collaborator Author

VladimirFokow commented Oct 21, 2024

Great! I'm happy to have contributed! 👍

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.

3 participants