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

Fix referencing video files from any path #36

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Feb 15, 2024

This makes it no longer necessary to place video files in _static, but instead in any location.

I spent a while going through Shinx's implementation for images, and this copies what it does:

  1. File paths are normalized with Environment.relfn2path, and also a dependency is added for the document in the Environment. This allows paths relative to any document.
  2. A post transform adds the files from the Environment to a tracker on the Builder. This causes the HTML5 builder to copy the file to the _images directory (and a unique name is guaranteed by the tracker on the Environment).
  3. The URI is re-written to be relative to the builder image path (only when using HTML output.)

Fixes #33
Because Sphinx then tries to copy the file, it raises a warning if not found, so I think this also fixes (partially) #14. However, if you expected the file to be in the _static directory, it might not warn. I did not check that.

@severin-lemaignan
Copy link

I've tested the patch and it works brilliantly for me (also with 2 video sources). Thanks!

Copy link
Member

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I really like this update, aligning on how image directive is working is a very good thing.
Could you update the documentation to reflect this change there as well ?

I also see that mypy is unhappy with typing checks can you have a look to the latest build ?

sphinxcontrib/video.py Show resolved Hide resolved
sphinxcontrib/video.py Outdated Show resolved Hide resolved
This makes it no longer necessary to place video files in `_static`, but
instead in any location. This copies Sphinx's implementation for images:

1. File paths are normalized with `Environment.relfn2path`, and also a
   dependency is added for the document in the `Environment`. This
   allows paths relative to any document.
2. A post transform adds the files from the `Environment` to a tracker
   on the `Builder`. This causes the HTML5 builder to copy the file to
   the `_images` directory (and a unique name is guaranteed by the
   tracker on the `Environment`).
3. The URI is re-written to be relative to the builder image path (only
   when using HTML output.)
sphinxcontrib/video.py Show resolved Hide resolved
@12rambau
Copy link
Member

12rambau commented Mar 6, 2024

I'll merge it with master once the tests are happy and create a release candidate to let people play with it. As it's changing how images are referenced it's important to give people a heads up.

@QuLogic can you have a look to the mypy issues ?

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 6, 2024

The mypy issue seems unrelated; I can reproduce it on master. I see previous workflows passed, so maybe something in Sphinx changed recently?

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 7, 2024

See #37.

@12rambau 12rambau merged commit 92bb356 into sphinx-contrib:master Mar 8, 2024
5 checks passed
@QuLogic QuLogic deleted the local-video branch March 8, 2024 09:05
@Calinou
Copy link

Calinou commented Mar 12, 2024

I see the version was bumped to 0.2.1rc0 in 098b467. Could this be published on PyPI (perhaps as 0.2.1) so that it can be referenced in Sphinx repositories? This would unblock me for integrating videos on godot-docs. Thanks in advance 🙂

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 12, 2024

It is on PyPI: https://pypi.org/project/sphinxcontrib-video/0.2.1rc0/ (you'd need to pass --pre to pip install to see it, or explicitly ask for that version.)

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.

Local videos not getting copied as images
4 participants