Delegate color handling in Code to pygments and add instructions for creating custom styles - #4816
Conversation
Using a custom `formatter_style` (such as a light theme) was previously overridden by the hardcoded dark `ManimCommunity#222` default `fill_color`. This change dynamically extracts the native `background_color` attribute from the specified Pygments style to ensure the container matches the theme. Additionally, updated the line number color logic: it now respects the color defined by the active Pygments style, falling back to `GRAY` if unspecified, preventing invisible white text on light backgrounds. Fixes ManimCommunity#4811
Previously, user-defined background `fill_color` was accidentally overridden by the default fallback color. This fix ensures that the custom value is preserved, passing the related test assertions.
behackl
left a comment
behackl
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I agree that this should be fixed, I've just found a few small details that were not quite right with your proposed solution:
base_paragraph_config.update({"color": BLACK})overrides a user-provided paragraph color and makes unstyled text unreadable with dark themes.- the
Texttokens in pygments also sometimes define a foreground color, if one is available that should be used (and fall back to BLACK only if that is unset) - While we are at it, the line number styling should be decoupled a bit more from the code paragraph config
- And finally, a few more tests would be nice.
I'll push a reviewer commit addressing these changes in a second -- could you please take a look and let me know whether you are happy and whether your original usecase is still covered as intended? I'm happy to get this merged afterwards.
| background_config_base.update(background_config) | ||
| background_config_base.update(background_config or {}) | ||
| if background_config_base["fill_color"] is None: | ||
| background_config_base["fill_color"] = selected_style.background_color |
There was a problem hiding this comment.
Would it perhaps make sense to use the existing setdefault method here? I.e., replace lines 235-236 with
background_config_base.setdefault("fill_color", selected_style.background_color)This also removes the need for defining "fill_color": None, in default_background_config.
There was a problem hiding this comment.
Hi, thanks for the feedback! I think we should keep fill_color for now, mainly for backward compatibility. Also, removing it right away would require providing an alternative approach for custom styling, which still needs further discussion. So keeping it feels like a good temporary workaround.
There was a problem hiding this comment.
I'm maybe misunderstanding you here! Are you saying that this replacement would break backward compatibility?
AFAICT, behavior is entirely unchanged by removing "fill_color": None from default_background_config, since background_config_base.setdefault will populate the value with either background_config["fill_color"] or selected_style.background_color depending on whether fill_color is already a key in the background_config_base dict. That is, if the user provides a fill color, we use that, otherwise we use the BG color from selected_style.
Am I misunderstanding my own code, is this actually a completely different thing? 😄
There was a problem hiding this comment.
Haha, my bad! Looks like I misunderstood that part. Thanks for clarifying!
I see what you mean now. In my opinion, it comes down to coding habits — the previous approach was a bit more traditional, whereas your solution is definitely more Pythonic.
# 1. Using if-else (Traditional / Explicit)
if background_config_base["fill_color"] is None:
background_config_base["fill_color"] = selected_style.background_color
# Or
if "fill_color" not in background_config_base:
background_config_base["fill_color"] = selected_style.background_color
# 2. Using setdefault (Pythonic / Concise)
background_config_base.setdefault("fill_color", selected_style.background_color)That said, I personally slightly prefer the former just because I find it a bit more explicit and semantic to read for Python beginners. But I'm completely fine with your version if you prefer!
Thanks!
There was a problem hiding this comment.
I'm fine either way, and this could easily be a PR of its own along with my other comment below. "Proper handling of config dictionaries" or whatever. I'm sure there are other mobject types where this could be applied.
| background_config = {} | ||
| background_config_base = self.default_background_config.copy() | ||
| background_config_base.update(background_config) | ||
| background_config_base.update(background_config or {}) |
There was a problem hiding this comment.
Maybe this breaks some convention I'm unaware of – if so, please disregard!
The current default value of background_config (and paragraph_config for that matter) is
background_config: dict[str, Any] | None = None,which is why this background_config or {} construct is needed. Could we instead set the default value to
background_config: dict[str, Any] = {},and instead do one of these for updating the config dict?
background_config_base = self.default_background_config.copy()
background_config_base.update(background_config)or
background_config_base = self.default_background_config | background_configedit: I guess one reason to keep them as None is that it explicitly communicates optionality to the user, though you could make that case about other types of default value.
There was a problem hiding this comment.
Interesting idea in principle. I think, however, it is better if library code used "conventional" means of manipulating dictionaries; if we start using the same mechanisms as users do we might inadvertently override a decision the user has made already... which we could still check for here of course.
Either way, I think you are right in that handling of configuration dicts is a separate issue to discuss (and I am pretty sure that, especially for more complex mobjects, there is a ton to improve.)
|
I've checked commit aa7a1c6. The changes look great to me; thanks for helping with this! I agree with these updates. Summary & Context for Future Reference1. The default paragraph colorRather than setting the default paragraph color, you are actually setting the color for characters that Pygments does not explicitly classify. Since we adopt Pygments, we found that it provides default colors for unrecognized plain text or punctuation. There are two situations: one is base_paragraph_config.update(paragraph_config or {})
default_text_color = selected_style.style_for_token(TextToken).get("color")
if default_text_color is not None:
default_text_color = ManimColor(f"#{default_text_color}")
base_paragraph_config.setdefault("color", default_text_color or BLACK)The code snippet above allocates the default paragraph color. If not set, the default paragraph color is white because Paragraph uses Text to render characters, where the default color is white. White text works fine with dark styles, but is problematic for light styles—especially with a white background like In practice, paragraph color settings only affect unrecognized plain text and punctuation (such as commas default_paragraph_config: dict[str, Any] = {
"font": "Monospace",
"font_size": 24,
"line_spacing": 0.5,
"disable_ligatures": True,
"color": "#03fcfc" # Specified by users
}It is worth noting that this custom color has no effect in dark styles. This is because all built-in dark styles explicitly set the color for every character, overriding any user-specified paragraph color during the syntax highlighting process. Related ScriptGetting the Default Pygments Color for Unrecognized Charactersfrom pygments.styles import STYLE_MAP
from pygments.token import Text as TextToken
from pygments.styles import get_style_by_name
for formatter_style in list(STYLE_MAP.keys())[:]:
# print(formatter_style)
selected_style = get_style_by_name(formatter_style)
# print(selected_style)
default_text_color = selected_style.style_for_token(TextToken).get("color")
print(f"{formatter_style}: {default_text_color}")Output2. The implementation of line number colorInitially, we noticed that when specifying light styles like Since we use Pygments, we found that it provides the Under 3. Since it only affects the line number color, why set a default color for the paragraph?This is because we aimed to follow the original design pattern and keep code changes to a minimum. Additionally, while we identified potential improvements in the Code class—such as supporting text attributes (e.g., bold, italic, underline, and background color) to better align with the Pygments standard—these changes require further discussion within the Manim Community Dev Team. Therefore, we prioritized backward compatibility and minimal code modification for now. Furthermore, this approach enhances robustness and allows users to define custom styles for characters that Pygments does not recognize.
Thanks for your time and review! |
|
Regarding line number coloring: |
behackl
left a comment
behackl
left a comment
There was a problem hiding this comment.
Thanks for pointing out ManimColor.contrasting()! I took another look at the intended semantics of Pygments line_number_color = "inherit":
Rather than choosing a fixed gray or always deriving black/white from the background, I think the cleanest behavior is to inherit the foreground color already resolved for the code paragraph.
- A user-configured paragraph_config["color"] continues to apply to both code and line numbers.
- Otherwise, an explicit Pygments line_number_color is used.
- If Pygments specifies "inherit", line numbers use the style’s general Text foreground color.
- If that foreground is unspecified, they use the existing black fallback.
I checked the styles bundled with Pygments 2.20.0: 52 use "inherit", and 24 of those do not define a general Text foreground. All 24 are light themes, so the black fallback is appropriate. For example, Vim now inherits #cccccc, while Xcode falls back to black.
ManimColor.contrasting() would guarantee black/white contrast, but it would discard the style’s chosen foreground color when one exists. Inheriting that color seems more faithful to the formatter style.
I also added tests covering explicit paragraph colors, class-default paragraph colors, explicit Pygments line-number colors, inherited foreground colors, and the black fallback.
GH Actions are currently down, but I'll leave this open for others to weigh in anyways -- thanks for all the comments, reviews and work invested here; much appreciated. Thank you!
|
Indeed, in terms of Pygments semantics ( Related Resources1. Line number color resolution logicgraph TD
C{Is paragraph color configured?}
C -->|Yes| D[Use configured color]
C -->|No| E{Is line number color `inherit`?}
E --> |Yes| F{Is `default_text_color` `None`?}
E --> |No| G[Use line number color specified by style]
F --> |Yes| H[Fall back to BLACK]
F --> |No| I[Use foreground color specified by style]
2. Script to inspect line number colors and foreground colors across all Pygments stylesfrom pygments.styles import get_all_styles, get_style_by_name
from pygments.token import Text as TextToken
for style_name in get_all_styles():
selected_style = get_style_by_name(style_name)
default_text_color = selected_style.style_for_token(TextToken).get("color")
line_number_color = getattr(selected_style, "line_number_color", None)
print(f"{style_name:<20}: {line_number_color:<20} {default_text_color}")Output
LGTM! Thank you, @behackl and @nikolajmunk, for reviewing! |
|
I identified an issue during testing. If users don't set a paragraph color, everything looks as expected. However, if they try to change the line number color by setting the paragraph color, it breaks the original styling for code snippets.
Test scriptTest commit d360a05from typing import Any
from manim import *
from pygments.styles import STYLE_MAP
class Test(Scene):
def construct(self):
default_background_config: dict[str, Any] = {
"buff": 0.3,
# "fill_color": ManimColor("#222"), # Here
"stroke_color": WHITE,
"corner_radius": 0.2,
"stroke_width": 1,
"fill_opacity": 1,
}
default_paragraph_config: dict[str, Any] = {
"font": "Monospace",
"font_size": 24,
"line_spacing": 0.5,
"disable_ligatures": True,
"color": "GREEN", # Here
}
# file: str = "code_snippets.py"
code_snippets: str = """from collections.abc import Iterator
# This is an example
class Math:
@staticmethod
def fib(n: int) -> Iterator[int]:
\"""Fibonacci series up to n.\"""
a, b = 0, 1
while a < n:
yield a
a, b = b, a + b
result = sum(Math.fib(42))
print(f"The answer is {result}")
"""
rendered_code = Code(
# code_file=file,
code_string=code_snippets,
language="python",
formatter_style="xcode", # Here
tab_width=4,
add_line_numbers=True,
line_numbers_from=1,
background="window",
background_config=default_background_config,
paragraph_config=default_paragraph_config,
)
self.add(rendered_code)
self.wait(2)This happens because the line number color and paragraph color are bound together—changing one inevitably changes the other. Resolving this properly is a bit complex for now. Therefore, I propose reverting to commit aa7a1c6, where the default line number color is GRAY and users cannot modify it via Another reason I suggest reverting to commit aa7a1c6 is that the core question is whether we should allow users to customize styles. I believe the answer is yes, but we can table that discussion for later. Could we focus on getting this PR merged first to support light styles for code snippets? For future reference, here are a few ideas on how we might handle custom styles down the road:
Thanks again! |
|
I'll think about this a bit more, but after some more reflection I now believe that what we should actually do is teach people that in order to customise the color scheme rendered by If pygments says I'd be willing to implement some shallow helpers to make interaction with pygments easier and save the user an import; |
|
I agree with you, @behackl. Ideally, styling should be fully delegated to Pygments. Therefore, we won't allow users to modify styles directly via configs like We can write docs guiding users on how to leverage Pygments' native capabilities (Write your own style) for custom styling, treating it as an advanced topic. As a result, commit d360a05 works great. If users still pass color via paragraph_config, we can consider it an unsupported workaround (which is actually inevitable due to the class hierarchy between |
|
Okay -- in this case, (and hopefully for the last time :-)) I've pushed some more changes that refactor the handling of colors with respect to I've also added a bunch of explicit documentation, should appear soon in the preview docs at https://manimce--4816.org.readthedocs.build/en/4816/reference/manim.mobject.text.code_mobject.Code.html#manim.mobject.text.code_mobject.Code. I'll leave this open until you've had the chance to take another look and see whether it works for your usecase -- then we can get this merged. Thanks again for the constructive feedback! |
|
IMO, config dicts for style overrides are common enough in Manim that it seems entirely natural to let people use them here as well, even for colors. As far as I know, nowhere else in Manim do we try to protect the user from passing something "stupid" in a config dict (off the top of my head, the But this can be the topic of another PR :) |
|
Hi @nikolajmunk, while enabling users to override |
|
Everything looks good! The doc preview is available via this link. Thank you so much for the feedback and guidance! Always happy to contribute to such an awesome community. |
|
Thanks, everyone! |
behackl
added
enhancement
behackl
changed the title
Code to pygments and add instructions for creating custom styles
3 participants
Overview: What does this pull request change?
This pull request fixes an issue (#4811 ) where custom
formatter_style(especially in a light theme likexcode) was previously overridden by the hardcoded darkManimColor("#222")defaultfill_color. It also updates the line number color logic to dynamically respect the active Pygments style.Fixes #4811
Motivation and Explanation: Why and how do your changes improve the library?
Previously, specifying a light
formatter_style(e.g.,xcode) resulted in an incorrect dark container background because of the hardcodedManimColor("#222")default.This pull request resolves the issue by dynamically extracting the native
background_colorfrom the active Pygments style. Additionally, line number colors are now dynamically fetched from the style context (falling back toGRAYif unspecified) to prevent invisible white text on light backgrounds.Q&A
Q: Why is the
fill_colorattribute preserved instead of deleted?A: To maintain backward compatibility. While deprecating
fill_coloraligns with the WYSIWYG principle (since Pygments styles should dictate the theme), removing it immediately would break existing user configurations. It is recommended to deprecate this attribute now and remove it during a future major refactor of the Code class.Links to added or changed documentation pages
Further Information and Comments
Verification
This fix successfully resolves the issue. Below is the demonstration of the expected behavior:
Test.mp4
Test Comparison
Test Script
Code for verifying the fix
Related Resources
Reviewer Checklist
Thanks for your time and review!