Regression: restore extraction of data-URI images from source for builders whose output formats do not support them natively. #12344
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n the builder's file format does not support remotely-hosted image references.
…copy_images test cases.
jayaddison
changed the title
jayaddison
changed the title
…` and ``supported_data_uri_images`` builder attributes.
AA-Turner
reviewed
May 3, 2024
| @@ -72,6 +72,7 @@ def test_copy_images(app, status, warning): | |||
| images_dir = Path(app.outdir) / '_images' | |||
| images = {image.name for image in images_dir.rglob('*')} | |||
| assert images == { | |||
| # 'ba30773957c3fe046897111afd65a80b81cad089.png', # html: image from data:image/png URI in source | |||
There was a problem hiding this comment.
Yep; it's intended as a hint that the same image exists as in the other test_copy_images unit tests, but that it isn't written to file because HTML (and similarly ePub) can natively include img src="data:..." elements.
Probably not required, but if some future change affects the logic, it's possible that all of the relevant test cases would need to be updated, and in that case the cross-reference provided by the comment might be useful.
|
I'll remove the shoutiness from the pull request description; that was written at a moment where I'd emerged from confusion about what the |
jayaddison
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature or Bugfix
Purpose
data:URIs even when the builder has thesupported_remote_imagesflag set toFalse(the default).Ambiguity
I think the change could seem counter-intuitive. If I saw this changeset I might ask: if remote images are disabled, then why should we parse and extract an image from a URI on an image node?
The way I've resolved that question while developing it is:
These data-URI images can be considered 'local' to the source -- they're found in the input
reStructuredTextcontent, so we don't need to check the builder'ssupported_remote_imagesattribute at all during theDataURIExtractor.matchmethod; these are not remote images.Detail
Expected behaviour description
DataURIExtractor:data:URIs natively, leave the URI reference on the image unmodified, and allow the builder to process it normally.data:, and the contents are declared as having a supported MIME type, then decode the contents and write them to a hash-based filename with the corresponding filetype extension suffixed.Relates
data:URIs #12331.