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

Support for captioned and ordered images in galleries #3017 #3018

Merged
merged 10 commits into from Apr 19, 2018

Conversation

@jmcp
Copy link
Contributor

@jmcp jmcp commented Apr 11, 2018

For galleries: this change provides support for per-image captions
and for specifying an order to display images in.

Pull Request Checklist

  • [ X ] I’ve read the guidelines for contributing.
  • [ X ] I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • [ X ] I tested my changes.

Description

Implements issue #3017.

Example:
header.jpg This is the header image for my blog
Copy link
Member

@Kwpolska Kwpolska Apr 11, 2018

This format won’t cut it. Filenames could contain spaces. Instead of a custom parser, use something like JSON or YAML.

@getnikola getnikola deleted a comment Apr 12, 2018
@getnikola getnikola deleted a comment Apr 12, 2018
@getnikola getnikola deleted a comment Apr 12, 2018
For galleries: this change provides support for per-image captions
and for specifying an order to display images in.

We look for a file called metadata.yml in the gallery directory. If this
file exists, we read 'name', 'caption' and 'order' from it and apply that
metadata when generating the gallery. An entry must have at least 'name'
to be considered valid. We cannot guarantee that yaml.safe_load_all() will
return the elements in the same order that they are listed in the file.
@jmcp jmcp force-pushed the captioned-images branch from a653aff to 232a81b Apr 12, 2018
@getnikola getnikola deleted a comment Apr 12, 2018
For galleries: this change provides support for per-image captions
and for specifying an order to display images in.

We look for a file called metadata.yml in the gallery directory. If this
file exists, we read 'name', 'caption' and 'order' from it and apply that
metadata when generating the gallery. An entry must have at least 'name'
to be considered valid. We cannot guarantee that yaml.safe_load_all() will
return the elements in the same order that they are listed in the file.

The user is warned about incorrectly formed YAML entries in metadata.yml
(no 'name' found), and images found in the gallery directory which have
no corresponding entry in metadata.yml. These images are appended to the
end of the image list with the real filename as the caption.
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 12, 2018

It looks like your commit was misattributed. Please add the e-mail address you used for this commit (jmcp your domain) to your GitHub profile so your commits will appear as yours.
You may also want to add yourself to AUTHORS.txt.

Copy link
Member

@Kwpolska Kwpolska left a comment

A few more things to fix.

docs/manual.rst Outdated
Images in galleries may be provided with captions and given a specific
ordering, by creating a file in the gallery directory called metadata.yml.
This YAML file should contain a 'name' field for each image in the gallery,
and (if desired) either or both of 'caption' and 'order'.
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

This needs a specific example for multiple files and cases.

Copy link
Contributor Author

@jmcp jmcp Apr 16, 2018

Done.

# named metadata.yml, is YAML and must contain at least a 'name' field. Other
# recognised field names are 'order' and 'caption'.
# Defaults to True
# GALLERY_SORT_BY_META_FILE = True
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

This doesn’t need to be a setting. If user provides a metadata.yml file and it contains order fields, try to use them. Otherwise, don’t. I can’t think of a legitimate use case that would have a metadata file with order fields and not want to use it.

Copy link
Contributor Author

@jmcp jmcp Apr 16, 2018

Agreed

img_titles.append(self.captions[fn])
else:
img_titles.append(fn)
self.logger.warn(
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

No need for a warning IMO.

Copy link
Contributor Author

@jmcp jmcp Apr 16, 2018

Made this a self.logger.debug() call instead.

# final element, so skip it
if not img:
continue
if 'name' in img.keys():
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

If that is a dict, you can use if 'name' in img: — and please do that in other imgkeys cases below

Copy link
Contributor Author

@jmcp jmcp Apr 17, 2018

Done.

@@ -568,7 +635,7 @@ def url_from_path(p):
else:
img_list, thumbs, img_titles = [], [], []

photo_array = []
pre_photo = {}
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

photo_info perhaps?

Copy link
Contributor Author

@jmcp jmcp Apr 16, 2018

Seems reasonable; done.

photo_array.append(pre_photo.pop(entry))
# Do we have any orphan entries from metadata.yml?
if len(pre_photo) > 0:
for entry in pre_photo:
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

I would sort entries here and below (in else) so the ordering does not change due to dict randomness (on Python ≤3.5)

Copy link
Contributor Author

@jmcp jmcp Apr 17, 2018

I'm uncertain about this suggestion.

We cannot sort photo_info since it's a dict, and unless we've got an order (implicitly or explicitly) specified from metadata.yml, then having unsorted output matches the existing behaviour. I'd prefer to leave this as-is.

@@ -229,7 +236,18 @@ def gen_tasks(self):

image_name_list = [os.path.basename(p) for p in image_list]

if self.kw['use_filename_as_title']:
if self.captions:
Copy link
Member

@Kwpolska Kwpolska Apr 12, 2018

Why is this a global variable? This should be handled in a gallery-specific way. Nikola can be run with threading, in which case breakage will occur.

Instead, return captions, order from find_metadata() and use that.

Also: how does this work for multilingual sites?

Copy link
Contributor Author

@jmcp jmcp Apr 16, 2018

Now using order, captions = self.find_metadata(gallery) as suggested, and adding both as elements of context.

I haven't got a multilingual site to with, unfortunately. Looking through the code, though, I believe that the only place where non-en might get caught is in the call to json.dumps(). If we specify ensure_ascii=False that should do the trick.

Copy link
Contributor Author

@jmcp jmcp left a comment

I believe that I've addressed all the changes requested.
This is my first github PR so I'm unsure if I have got everything figured out correctly in order for this PR to progress to integration - I would really appreciate feedback to let me know what I've missed, or what process steps I have mucked up. Thankyou!

.. code:: yaml
---
name: ready-for-the-acid-wash.jpg
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

Is this necessary? I would prefer something where users can specify only the files they want changed from the default.

Copy link
Contributor Author

@jmcp jmcp Apr 19, 2018

I've updated the docs to make it plain that you only need to list images for which a specific caption and/or order is required.

Copy link
Member

@Kwpolska Kwpolska left a comment

This looks pretty good! Localization support is very simple to add (I added instructions in comments). Other than that, there are a few minor style improvements to make.

PS. I still haven’t bothered to run and test this.

PPS. No, we really don’t require a lot of special effort from contributors. Authors/changelog and making sure it works is basically it.

else:
img_titles.append(fn)
self.logger.debug(
"Image {0} found in gallery but not listed "
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

(Feel free to go past 79 characters per line, especially if adhering to the limit makes code look ugly. Keep it below 100 if you can, 120 max.)

@@ -393,6 +411,51 @@ def create_galleries(self):
'uptodate': [utils.config_changed(self.kw.copy(), 'nikola.plugins.task.galleries:mkdir')],
}

def find_metadata(self, gallery):
"""Search for a gallery metadata file. Returns list, dict if found."""
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

Please merge the docstring and the comment below it into one large, multiline docstring.

else:
order.append(img['name'])
else:
self.logger.warn("no 'name:' for ({0}) in {1}".format(
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

I would argue this is error-and-exception worthy.

@@ -220,6 +225,8 @@ def gen_tasks(self):
self.kw[k] = self.site.GLOBAL_CONTEXT[k](lang)

context = {}
context['order'] = order
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

Let’s make this translatable, it won’t be hard.

  1. create order, captions inside this loop, so you can
  2. pass lang to find_metadata, which
  3. will use it to get the correct language path (I’ll comment on it below)

# use an empty string.
# Returns a list (ordering) and dict (captions).

meta_path = os.path.join(gallery, "metadata.yml")
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

base_meta_path = os.path.join(gallery, "metadata.yml")
localized_meta_path = utils.get_translation_candidate(self.site.config, base_meta_path, lang)

And then (pseudocode):

if localized path exists:
    used_path = localized
else if base path exists:
    used_path = base
else:
    return [], {}
# all the other logic here, with `used_path`

if os.path.isfile(meta_path):
self.logger.debug("Using {0} for gallery {1}".format(
meta_path, gallery))
self.meta_path = meta_path
Copy link
Member

@Kwpolska Kwpolska Apr 17, 2018

One more global reference to remove here.

@Kwpolska Kwpolska mentioned this pull request Apr 17, 2018
Kwpolska and others added 6 commits Apr 18, 2018
It is now only necessary to specify an image filename in metadata.yml
if you really want to have a specific caption or ordering for an image.
Updated manual to make this clear.
Update galleries.py to make it an error to provide a YAML entry which
does not have a name: field.
photo_array.append(photo_info.pop(entry))
# Do we have any orphan entries from metadata.yml, or
# are the files from the gallery not listed in metadata.yml?
if len(photo_info) > 0:
Copy link
Member

@Kwpolska Kwpolska Apr 19, 2018

if photo_info: would be better spelling of that (I fixed it)

@Kwpolska Kwpolska merged commit 7db87dd into getnikola:master Apr 19, 2018
0 of 3 checks passed
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Apr 19, 2018

Thanks for contributing!

You may still want to add the e-mail address you used for the first two commits to your GitHub profile so your commits will appear as yours.

@Kwpolska Kwpolska mentioned this pull request Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants