Skip to content

Conversation

behackl
Copy link
Member

@behackl behackl commented Jan 18, 2025

See title.

Due to a version upgrade of pygments, support for short notation color hex codes was required -- which also required a patch to Code. This part of this PR could be factored out (e.g., to allow us to finalize the release faster).

The Code mobject was in a somewhat bad state, code quality wise. This re-implementation is clean(er) and easier to maintain / extend (especially considering the interest in terms of adding specific transform and highlight animations in recent PRs and issues).

Note: I did not keep the interface of the old mobject -- in fact, there are several changes to the names of the keyword arguments etc., and I've also not ported all of the functionality. I'm open to porting over further features (like configureable tab width etc.) though.


Summary of breaking changes for the Code class, by keyword arguments. Old arguments:

  • file_name -- renamed to code_file
  • code -- renamed to code_string
  • tab_width -- kept as is
  • line_spacing -- kept, but has to be passed inside of paragraph_config dict now
  • font_size -- kept, but has to be passed inside of paragraph_config dict now
  • font -- kept, but has to be passed inside of paragraph_config dict now
  • stroke_width -- kept, but has to be passed inside of paragraph_config dict now
  • margin -- renamed to buff and has to be passed inside of background_config dict now
  • indentation_chars -- gone
  • background -- kept as is
  • background_stroke_width -- renamed to stroke_width and has to be passed inside of background_config dict now
  • background_stroke_color -- renamed to stroke_color and has to be passed inside of background_config dict now
  • corner_radius -- kept, but has to be passed inside of background_config dict now
  • insert_line_no -- renamed to add_line_numbers
  • line_no_from -- renamed to line_numbers_from
  • line_no_buff -- gone
  • style -- renamed to formatter_style
  • language -- kept as is
  • generate_html_file -- gone
  • warn_missing_font -- gone
  • **kwargs -- gone

New keyword arguments:

  • background_config -- extra arguments passed to background mobject (currently: SurroundingRectangle), default arguments are stored in class attribute Code.default_background_config
  • paragraph_config -- extra arguments passed to Paragraph (for code and line numbers), default arguments are stored in class attribute Code.default_paragraph_config.

Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Haven't tested this yet, but just some stuff I saw from looking through.

On another note, since the Code mobject has been fully rewritten, it might be a good idea to enable mypy on it (see mypy.ini in the root directory).

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for the very needed rewriting of Code and the support of shorter hex code colors!

I left some comments regarding Code.

EDIT: I just noticed that, while I was writing my review and before publishing it, Jason published his own. A few of his comments are similar or even the same as mine.

@behackl behackl linked an issue Jan 19, 2025 that may be closed by this pull request
@behackl
Copy link
Member Author

behackl commented Jan 19, 2025

Thanks for your reviews! I've addressed all suggestions, the interface of the rewritten mobject now looks quite different than in the first version -- but I think it was improved significantly and brought in line to how other mobjects composed of different sub-mobjects like Axes are constructed.

I've updated the overview in the PR description, the main takeaway is that there are now two dict[str, Any] arguments paragraph_config and background_config to which further arguments for Paragraph and the SurroundingRectangle can be passed. Semantically, the implementation could be improved somewhat by renaming these paragraph_config_extra and background_config_extra, but I also don't think this is bad as it is currently.

Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

LGTM!
I noticed we don't have any graphical tests for Code - maybe in this PR or a followup, it might be a good idea to add one?

@behackl
Copy link
Member Author

behackl commented Jan 19, 2025

LGTM! I noticed we don't have any graphical tests for Code - maybe in this PR or a followup, it might be a good idea to add one?

IIRC the issue with graphical tests was that Text and Paragraph do not render pixel-consistently across systems, but it might be worth to look into this again -- if needs be, perhaps with OS-dependent control data.

@behackl behackl added refactor Refactor or redesign of existing code breaking changes This PR introduces breaking changes labels Jan 19, 2025
@behackl behackl added this to the v0.19.0 milestone Jan 19, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chopan050 chopan050 merged commit 1efe869 into ManimCommunity:main Jan 19, 2025
21 checks passed
@behackl behackl deleted the rewrite-code-fix-color-shortcodes branch January 19, 2025 18:03
@chopan050 chopan050 mentioned this pull request Jan 19, 2025
3 tasks
@behackl behackl mentioned this pull request Jan 21, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changes This PR introduces breaking changes refactor Refactor or redesign of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Code Mobject incorrect color string parsing

3 participants