Added media_embed config option to control whether media in Jupyter notebooks is embedded

[mforbes]

Overview: What does this pull request change?

Adds a --embed option to the %%manim magic that will embed the video data.

Resolves #2441, #2416.

Motivation and Explanation: Why and how do your changes improve the library?

Currently video data is embedded only if running on Google's CoLab platform, but there are other use-cases (relocatable notebooks) and this should be customizable.

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[mforbes]

I think the code is complete, but need to figure out how to write tests. Also, I have slightly abused config allowing users to set config.embed_video = True (or False to override CoLab). Should add an official config option or just leave this as a mildly documented feature?

[mforbes]

Any suggestions on how to test this change would be appreciated. Only the _generate_file_name() function seems to be currently tested in tests/. Am I missing some sort of testing that spins up a notebook and executes the magic?

[mforbes]

In principle Images can be embedded too, so I changed the config option to simply embed and apply it to images as well. The CoLab override only applies to videos, but the default for Image is generally True unless it is a URL.

[behackl]

In general this looks good to me, thank you for your contribution!

I am somewhat on the fence regarding the Jupyter-only implementation of --embed, and I think that given the fact that stuff like media_width also is a proper configuration option it would be more consistent to do so here as well. What do you (or other devs) think about that?

[behackl]

I have thought about it some more, and I think introducing jupyter-only flags is a bad idea overall. This should be implemented as a proper config option, just like media_width. Perhaps also renamed to media_embed.

In case there is no reaction here, I'll look into it.

[mforbes]

Okay, I took a stab at making media_embed follow the same pattern as media_width. I don't see any tests for media_width and am not familiar enough with the code to know how to add new tests for these. Can one do manim --media_width? It does not seem to be supported. (Or is it only a config-file configurable option?)

[behackl]

Thank you very much for your efforts! There are two problems that should be addressed:

• In default.cfg, media_embed should not be assigned a value (otherwise it will be false by default); just put media_embed = .
• The value from the default config currently get read and parsed as strings, you can check that python -c "from manim import config; config.media_embed" yields the string "False". The logic in utils.py needs to be adapted: after changing the value in default.cfg the default case will be that parser["jupyter"].get("media_embed") will yield the empty string "".

What do you think?

[behackl]

I went ahead and fixed the issues raised in my code review via 1ec2017 -- from my point of view, this is good to go now.

Thanks again for your efforts!

[Darylgolden]

LGTM, thank you!

[ad-chaos]

This seems to have broken preview in jupyter notebooks.