-
Notifications
You must be signed in to change notification settings - Fork 16
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: Download image to cache_path #23
base: master
Are you sure you want to change the base?
Conversation
c933d80
to
4d65651
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I noticed this too and possibly started doing something with it, but I either broke too much or got distracted with something else. I belive this change makes sense, so thanks!
P.S. you have a typo for "image" in your PR title
@@ -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 |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
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']) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Thank you for contributing @SilverRainZ and thank you for reviewing @terencehonles. I'll have a look this weekend or next Wednesday at the latest. |
Please also see my co-maintainer request at #20, thank you! |
@SilverRainZ I tested your PR with sphinx 4.0.2 and sphinxcontrib-images from this PR (commit id 4d65651 [1]), a fresh sphinx project [2] and the following content: conf.py:
index.html
I get a directory I however do not get the downloaded image copied to The file is copied if you add
Why remote images need to be added with Could you verify these findings and if you find the same as I do, update your PR. [1] git clone (remember [2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also #23 (comment) as already mentioned.
img['remote_uri'] = self.arguments[0] | ||
env.remote_images[img['remote_uri']] = img['uri'] | ||
env.images.add_file('', img['uri']) |
There was a problem hiding this comment.
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)
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']) |
There was a problem hiding this comment.
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
@@ -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 |
There was a problem hiding this comment.
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
Sorry for delay, I have add the missing |
No description provided.