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.

Sign up for GitHub

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: Download image to cache_path #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix: Download image to cache_path #23

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f705bd
Fix: Download iamge to cache_path
SilverRainZ Jun 16, 2021
f9084e0
URI of image node should be prefixed with '/'
SilverRainZ Jun 17, 2021
4d65651
No need to add file to env by self
SilverRainZ Jun 17, 2021
747bf48
Manually copy downloaded image to env
SilverRainZ Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions sphinxcontrib/images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,17 +157,16 @@ def run(self):
if self.is_remote(self.arguments[0]):
img['remote'] = True
if download:
img['uri'] = os.path.join('_images', hashlib.sha1(self.arguments[0].encode()).hexdigest())
reluri = os.path.join(conf['cache_path'], hashlib.sha1(self.arguments[0].encode()).hexdigest())
img['uri'] = os.path.join('/', reluri) # relative to source dir
Copy link

Choose a reason for hiding this comment

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

I'm a little confused about this, do you need to os.path.join the relative path to /? Is this actually the URL that Sphinx will use to reference the image in the HTML? If it's actually that it might make sense to use urllib.parse.urljoin in order to make it more obvious what you're doing.

# *nix
>>> os.path.join('/', 'test/key')
'/test/key'
>>> urllib.parse.urljoin('/', 'test/key')
'/test/key'

Copy link
Author

Choose a reason for hiding this comment

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

I think I wrote a confusing comment :'(.

The prefixed / means the uri is "relative to source dir"(rootof sphinx project) but not "relative to current document's dir". Without it, sphinx will try load image from srcdir/foo/bar/_images, while our cache_path is srcdir/_images/.

And the URI is actualy a filesystem path here, so I think we need os.path.join.

Copy link
Collaborator

@jonascj jonascj Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My testing shows me thumbnail directives with :download: true were copied correctly to _build/html/_images/ and the URL set correctly before this PR (e.g. in 0.9.3).

Thumbnail directives (local or remote) in index.html turn into _images/<imagefile>-URLs and thumbnail directives in subdir/subfile.rst turn into ../_images/<imagefile>-URLs (because the html-file resides in a subdirectory compared to index.html ../_images are used).

Edit: clarity

img['remote_uri'] = self.arguments[0]
env.remote_images[img['remote_uri']] = img['uri']
env.images.add_file('', img['uri'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find that remote images with :download: true are downloaded to the custom cache directory but not copied to _build/html/_images/ upon building. Hence they are not shown in the built HTML-docs. Adding env.images.add_file() back fixes this.

See also my comment here #23 (comment)

env.remote_images[img['remote_uri']] = reluri
else:
img['uri'] = self.arguments[0]
img['remote_uri'] = self.arguments[0]
else:
img['uri'] = self.arguments[0]
img['remote'] = False
env.images.add_file('', img['uri'])
Copy link

Choose a reason for hiding this comment

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

Is this change needed? I'm not completely up to speed about env.images since I've not looked at this code in quite awhile.

Copy link
Author

@SilverRainZ SilverRainZ Jun 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to add file by self -- because we return a image node, it will be processed by sphinx's env collector.

Copy link
Collaborator

@jonascj jonascj Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In contrast to line 163 which seems necessary (see https://github.com/sphinx-contrib/images/pull/23/files/4d656516ca9453f46d158ca29a20fe8e22ff5674#r660153218) line 170 does not appear to be necessary, local images are copied to the _build/html/_images/ without this line. See also my comment here #23 (comment)

Edit: clarity


img['content'] = description.astext()

Expand Down