Manim-Notebook / manim-notebook

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

Init Manim Cell detection #7

Closed Splines closed 1 week ago

Splines commented 2 weeks ago

Closes #1.

In this PR, we init the cell detection mechanism, i.e. we provide code lenses above what we call a "Manim Cell" as well as folding ranges.

A Manim Cell is introduced by a Python comment starting with ## instead of just one #. If we wanted to change this, it's really just one line of code (the definition of the regex). More definitions on where Manim cells start and where they end can be found in respective doc strings.

Feature preview

For reviewers

A side question: does TypeScript IntelliSense work for you in this project? For me I get: Project Wide IntelliSense not available and this is quite annoying as IntelliSense is really limited.

bhoov commented 1 week ago

Shoot this is neat stuff, thanks @Splines ! I just checked the functionality and it is really smooth. Code folding works, the preview button works. I also like the choice of ## as a manim cell. Simple.

Next steps before I merge:

  1. Implement the logic that runs the cell
  2. Add a default hotkey for previewing the cell (suggestion: cmd+' cmd+c?)
  3. Improve help-text on cell. Perhaps "Preview Manim" -> "Preview in Interactive Manim" (since it is the only command right now, I don't think we need to worry about text length)
  4. Unify command descriptions in package.json. I am trying to make the package.json command descriptions easily searchable (even if other manim extensions popup). Right now, the existing checkpoint-paste command has the phrase vscode-manim, which I intended to include somewhere in the description of every command exposed through this extension. Would you include vscode-manim in the cmd description, or propose your own solution to this requirement?
  5. Only do cell detection in manim scene files. Right now, the ## ... folding logic and button is present in EVERY python file. We don't currently have a way to check that we are in a "manim" file. Can we implement logic to only show this when we are in a manim file?

I do not mind what you have done with splitting the logic across files. Certainly, with as much code as was required to make this functionality work, it is better split across files.

I have not had a problem with Intellisense for this project. Weird...

bhoov commented 1 week ago

(I see that you have updated the logic in #8. Ignore my request 1)

Splines commented 1 week ago

With regards to merging: I don't think it's beneficial to merge this PR via a merge commit as then every single commit that I made is on the main branch, e.g. also commits like Add more docstrings which is probably not that interesting. To have it a bit tidier, maybe it'd be better to Squash and merge this PR instead in order to only have one commit: Init Manim Cell detection (#7)?

Splines commented 1 week ago

The codelens Command also takes tooltip as argument, maybe there we could include some more text if you'd like to... See for example 3a54a2b as an idea.

bhoov commented 1 week ago

I originally used Ctrl+' Ctrl+e just because e is close to r and Ctrl+c is strongly anchored in my mind as "cancel"

This is totally fine, i didn't read #8 thoroughly. Thanks!

... preview active Manim Cell for the command and Preview Manim Cell for the codelens

I like your solution a lot, in fact. Let's call it what you suggest: "Preview Manim Cell" for code lens and "Preview active Manim Cell" for the command

In the specification there is also a category field. E.g. set it to the string Manim and all our commands will get Manim: prepended

THIS is what I was looking for. Thank you! However (and this is worth discussion) I expect others to develop Manim extensions on VSCode and I would like this extension to focus on the interactive workflow (e.g., there is already Manim Sideview whose functionality is orthogonal to that of this project). Indeed, the name of this extension likely needs to be improved from "vscode-manim" which does not capture the emphasis on interactive development. How should we brand this package on the marketplace? This will effect what we choose for the category of each cmd in package.json

To have it a bit tidier, maybe it'd be better to Squash and merge this PR instead in order to only have one commit

I can certainly click this button as you suggest. I appreciate that you provide guidance on clean commit histories across many collaborators as I have little experience in this regard :)

You don't publish the extension for every commit on main anyways, right?

Correct, I will try to publish 1x/week while things are rapidly developing

bhoov commented 1 week ago

Honestly, all these changes look amazing. Merged. Thank you!