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

Make thumbnail naming configurable with IMAGE_THUMBNAIL_FORMAT. #2565

Merged

Conversation

@blubberdiblub
Copy link
Contributor

blubberdiblub commented Nov 23, 2016

This will make it possible to deviate from the hard-coded thumbnail naming scheme of original_image_name.thumbnail.extension.

Coincidentally, it also allows to force all thumbnails to a particular image format (say JPEG) by putting that extension into the template literally instead of using the {ext} placeholder.

The default template '{stem}.thumbnail{ext}' corresponds to the current hard-coded naming scheme, so it's backwards compatible.

My personal use case is getting .jpg thumbnails out of source .png images, as my source material mainly consists of paletted 4 and 16 color images, which compress extremely well as PNG images, but when scaled down as thumbnails, they gain all kinds of extra shades of colors due to interpolation (which I want to keep, this is really fine and makes the thumbnails very nice to look at despite their smaller size), so the thumbnail files get significantly larger than the actual images. Considering that the primary blog article (before following the links to the original images) should be lean and mean, this is somewhat disappointing. So generating JPEG thumbnails is my way of counteracting the file size increase somewhat:

 41818 images/tfmx-tutorial-1/17-tfmx-load-song.png
 21586 output/images/tfmx-tutorial-1/17-tfmx-load-song.png
148790 output/images/tfmx-tutorial-1/17-tfmx-load-song.thumbnail.png
 86125 output/images/tfmx-tutorial-1/17-tfmx-load-song.thumbnail.jpg

Also, I don't particularly like the .thumbnail string, as it's a bit too long for my taste. Making the naming configurable enables me to change that in my blog's conf.py later to whatever I desire.

Copy link
Member

ralsina left a comment

LGTM, and it's useful.

@@ -45,6 +45,7 @@ def set_site(self, site):

def process_tree(self, src, dst):
"""Process all images in a src tree and put the (possibly) rescaled images in the dst folder."""
thumb_fmt = self.kw['image_thumbnail_format']
ignore = set(['.svn'])

This comment has been minimized.

Copy link
@ralsina

ralsina Nov 23, 2016

Member

Not really related to this PR but WTF. Also, Thumbs.db special casing a few lines below makes little sense.

This comment has been minimized.

Copy link
@blubberdiblub

blubberdiblub Nov 23, 2016

Author Contributor

Does the WTF relate to the plugin's usage of a dictionary for its configuration items (instead of just object attributes) or to my placement of retrieving the thumbnail format?

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Nov 23, 2016

Member

.svn is the real WTF. And Thumbs.db/.DS_Store special handling below. Pre-existing WTF, that is.

(You don’t need that variable, you could just take it out of self.kw on line 65; that would actually look better IMO

This comment has been minimized.

Copy link
@blubberdiblub

blubberdiblub Nov 23, 2016

Author Contributor

Yes, I considered that, but I don't like retrieving the same item from a dictionary over and over again during each iteration, that's why I opted for retrieving it once before entering the loop.

This comment has been minimized.

Copy link
@ralsina

ralsina Nov 23, 2016

Member

@blubberdiblub sorry, the WTF is not in your code, it's that preexisting stuff about .svn :-)

Copy link
Member

Kwpolska left a comment

Perhaps {name} would look better than {stem} though?

@blubberdiblub
Copy link
Contributor Author

blubberdiblub commented Nov 23, 2016

Sure, can be argued. I preferred {stem} as it's unambiguous (whereas {name} could also refer to the whole name including the extension), but I don't have a strong opinion concerning that and am ready to change it, if desired.

@Kwpolska
Copy link
Member

Kwpolska commented Nov 23, 2016

You decide.

@blubberdiblub
Copy link
Contributor Author

blubberdiblub commented Nov 23, 2016

Alright, there's no need to contribute to the proliferation of even more terms for the same thing (there are already like 4 or so ;) ) and there's no danger of someone getting it wrong, as far as I can tell, so I'll change it to {name}.

@Kwpolska Kwpolska merged commit 0957b3d into getnikola:master Nov 23, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Kwpolska
Copy link
Member

Kwpolska commented Nov 23, 2016

Alright, thanks for contributing to Nikola! 🎉

@Kwpolska
Copy link
Member

Kwpolska commented Nov 23, 2016

(I added your name to AUTHORS.txt, by the way)

@ralsina
Copy link
Member

ralsina commented Nov 23, 2016

Thanks for the neat feature @blubberdiblub !!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.