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

include camera model and shorten exif stats #280

Merged
merged 5 commits into from Jan 8, 2018

Conversation

2 participants
@anarcat
Contributor

anarcat commented Dec 31, 2017

This adds a line to show the camera model and make, if available, and
reorders and cleans up the EXIF parameters extracted so they are more
readable for photographers (who are the ones who care about this
anyways).

Note that, as a photographer, I know what "Fstop" mean, but I would
expect "exposure" or the common f/X.Y notation. The "Focal" label
means nothing to me: "Focal length" would mean something, but it's
long - and what are the units? Just using "mm" here clarifies any
possible misunderstanding.

I have found this format to be way more readable. The following:

ISO: 3200, Focal: 50, Exposure: 1/40, Fstop: 4.0
Date: dimanche, 31. décembre 2017

Becomes:

ISO 3200, 1/40s f/4.0, 50mm
FUJIFILM X-T2
dimanche, 31. décembre 2017

Now: dropping the explicit labels may be controversial, but I would
argue this is what markup is for. We could wrap those elements in
named or elements to provide more information through
mouse-over or CSS.

I use this patch in my install because I like to know the camera make:
it's important because I just got a new camera and I'd like to show
the difference between the different models. I'd be happy to modify to
suit your wishes better, in particular by adding the aforementioned
<span> elements to improve the markup.

Let me know!

include camera model and shorten exif stats
This adds a line to show the camera model and make, if available, and
reorders and cleans up the EXIF parameters extracted so they are more
readable for photographers (who are the ones who care about this
anyways).

Note that, as a photographer, I know what "Fstop" mean, but I would
expect "exposure" or the common f/X.Y notation. The "Focal" label
means nothing to me: "Focal length" would mean something, but it's
long - and what are the units? Just using "mm" here clarifies any
possible misunderstanding.

I have found this format to be way more readable. The following:

    ISO: 3200, Focal: 50, Exposure: 1/40, Fstop: 4.0
    Date: dimanche, 31. décembre 2017

Becomes:

    ISO 3200, 1/40s f/4.0, 50mm
    FUJIFILM X-T2
    dimanche, 31. décembre 2017

Now: dropping the explicit labels may be controversial, but I would
argue this is what markup is for. We could wrap those elements in
named <span> or <acronym> elements to provide more information through
mouse-over or CSS.
@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 3, 2018

Owner

The output looks nicer indeed, but it would be good to have labels, maybe with <abbr> tags ?

Owner

saimn commented Jan 3, 2018

The output looks nicer indeed, but it would be good to have labels, maybe with <abbr> tags ?

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 3, 2018

Contributor

okay, i'll rework this one. <abbr> is quite nice because it's like <span>; ie. it's an inline element with limited impact on layout but it can still be styled in different ways. should we have a per element "class"?

<abbrv class="exposure" title="exposure">f/5.3</abbr>

note, however, that browsers style the <abbr> tag differently - some add a dotted line, others in smallcaps, others with nothing at all. so we need to be careful here. see the MDN docs for more info.

Contributor

anarcat commented Jan 3, 2018

okay, i'll rework this one. <abbr> is quite nice because it's like <span>; ie. it's an inline element with limited impact on layout but it can still be styled in different ways. should we have a per element "class"?

<abbrv class="exposure" title="exposure">f/5.3</abbr>

note, however, that browsers style the <abbr> tag differently - some add a dotted line, others in smallcaps, others with nothing at all. so we need to be careful here. see the MDN docs for more info.

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 3, 2018

Owner

Not sure the class is needed, I leave it up to you. For the styling, maybe you can just avoid the small caps, as suggested in MDN ?

Owner

saimn commented Jan 3, 2018

Not sure the class is needed, I leave it up to you. For the styling, maybe you can just avoid the small caps, as suggested in MDN ?

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 3, 2018

Contributor

awesome. will do.

Contributor

anarcat commented Jan 3, 2018

awesome. will do.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jan 3, 2018

Codecov Report

Merging #280 into master will increase coverage by 4.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   83.35%   87.48%   +4.12%     
==========================================
  Files          19       19              
  Lines        1442     1358      -84     
==========================================
- Hits         1202     1188      -14     
+ Misses        240      170      -70
Impacted Files Coverage Δ
sigal/gallery.py 87.95% <0%> (-0.97%) ⬇️
sigal/log.py 89.65% <0%> (-0.67%) ⬇️
sigal/plugins/nomedia.py 90% <0%> (-0.57%) ⬇️
sigal/plugins/upload_s3.py 0% <0%> (ø) ⬆️
sigal/__init__.py 87.5% <0%> (+5.33%) ⬆️
sigal/plugins/watermark.py 93.02% <0%> (+5.75%) ⬆️
sigal/compat.py 100% <0%> (+7.14%) ⬆️
sigal/plugins/media_page.py 100% <0%> (+29.72%) ⬆️
sigal/plugins/feeds.py 100% <0%> (+81.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a9b8a0...2d5cd23. Read the comment docs.

codecov bot commented Jan 3, 2018

Codecov Report

Merging #280 into master will increase coverage by 4.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   83.35%   87.48%   +4.12%     
==========================================
  Files          19       19              
  Lines        1442     1358      -84     
==========================================
- Hits         1202     1188      -14     
+ Misses        240      170      -70
Impacted Files Coverage Δ
sigal/gallery.py 87.95% <0%> (-0.97%) ⬇️
sigal/log.py 89.65% <0%> (-0.67%) ⬇️
sigal/plugins/nomedia.py 90% <0%> (-0.57%) ⬇️
sigal/plugins/upload_s3.py 0% <0%> (ø) ⬆️
sigal/__init__.py 87.5% <0%> (+5.33%) ⬆️
sigal/plugins/watermark.py 93.02% <0%> (+5.75%) ⬆️
sigal/compat.py 100% <0%> (+7.14%) ⬆️
sigal/plugins/media_page.py 100% <0%> (+29.72%) ⬆️
sigal/plugins/feeds.py 100% <0%> (+81.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a9b8a0...2d5cd23. Read the comment docs.

use abbr to explain what the metadata fields are
Keep the normal font for browsers that switch to smallcaps, as
suggested by MDN:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/abbr

We remove the dotted line normalization currently configured in the
`normalize.css` file because it actually creates a *second* dotted
line in Firefox 57. The directive there is `text-decoration` which is
what we disable here instead. It looks better without any dotted line.

We leave the trailing commas and strings like `ISO`, `f/` or `mm` but
those could also be managed through CSS. `<br>` tags may similarly be
managed better through CSS as well.
@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 3, 2018

Contributor

oookay... so here's the result:

<a href="original/DSCF0535.JPG">Full size</a>              <br>
<abbr title="film speed" class="iso">ISO 320</abbr> <abbr title="exposure" class="exposure">1/400s</abbr> <abbr title="aperture" class="aperture">f/2.8</abbr> <abbr title="focal length" class="focal">41mm</abbr>                 <br><abbr title="camera make and model" class="camera">FUJIFILM X-T2</abbr>
                <br><abbr title="date" class="date">samedi, 16. décembre 2017</abbr>

it's quite a mouthful, but that's the best i can lay out, because of that weird macro stuff which, btw, made it a bit hard to test; I can't use double-quotes in tags otherwise the whole page layout breaks down.

It's also hard to test this in production because the .min.css file need to be regenerated using the makefile (which, incidentally, fails if your shell is not bash because POSIX doesn't support {foo,bar}) and then reinstalled by hand. How the heck do you test this anyways?

It seems to me it would be easier if CSS would be compressed on the fly when the gallery is build instead of when the program is installed and, in any case, only if necessary... Is it so slow that we want to avoid that?

Anyways, more details about the change are in the commitlog of 48c064d, but basically:

  1. i needed to adjust the style in normalize.css... Not sure it's the right place to do that. I decided to drop the dotted line completely as it looks quite noisy, because everything has a dotted line in there.

  2. I also wonder if we should leave the spacing, mm suffixes, f/ and ISO prefixes in there or we should do this in CSS

Let me know if that works for you.

Contributor

anarcat commented Jan 3, 2018

oookay... so here's the result:

<a href="original/DSCF0535.JPG">Full size</a>              <br>
<abbr title="film speed" class="iso">ISO 320</abbr> <abbr title="exposure" class="exposure">1/400s</abbr> <abbr title="aperture" class="aperture">f/2.8</abbr> <abbr title="focal length" class="focal">41mm</abbr>                 <br><abbr title="camera make and model" class="camera">FUJIFILM X-T2</abbr>
                <br><abbr title="date" class="date">samedi, 16. décembre 2017</abbr>

it's quite a mouthful, but that's the best i can lay out, because of that weird macro stuff which, btw, made it a bit hard to test; I can't use double-quotes in tags otherwise the whole page layout breaks down.

It's also hard to test this in production because the .min.css file need to be regenerated using the makefile (which, incidentally, fails if your shell is not bash because POSIX doesn't support {foo,bar}) and then reinstalled by hand. How the heck do you test this anyways?

It seems to me it would be easier if CSS would be compressed on the fly when the gallery is build instead of when the program is installed and, in any case, only if necessary... Is it so slow that we want to avoid that?

Anyways, more details about the change are in the commitlog of 48c064d, but basically:

  1. i needed to adjust the style in normalize.css... Not sure it's the right place to do that. I decided to drop the dotted line completely as it looks quite noisy, because everything has a dotted line in there.

  2. I also wonder if we should leave the spacing, mm suffixes, f/ and ISO prefixes in there or we should do this in CSS

Let me know if that works for you.

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 3, 2018

Owner

It's also hard to test this in production because the .min.css file need to be regenerated using the makefile (which, incidentally, fails if your shell is not bash because POSIX doesn't support {foo,bar}) and then reinstalled by hand. How the heck do you test this anyways?

I do this manually but I agree it is painful :(

It seems to me it would be easier if CSS would be compressed on the fly when the gallery is build instead of when the program is installed and, in any case, only if necessary... Is it so slow that we want to avoid that?

It's not slow, but it needs some code and an additional dependency (cssmin). I'm thinking about just removing the concatenation/minification step for now.

i needed to adjust the style in normalize.css... Not sure it's the right place to do that. I decided to drop the dotted line completely as it looks quite noisy, because everything has a dotted line in there.

Maybe better to put this in the theme css file to avoid losing the change when updating normalize.css.

I also wonder if we should leave the spacing, mm suffixes, f/ and ISO prefixes in there or we should do this in CSS

No strong feelings, just keep it simple 😉

Owner

saimn commented Jan 3, 2018

It's also hard to test this in production because the .min.css file need to be regenerated using the makefile (which, incidentally, fails if your shell is not bash because POSIX doesn't support {foo,bar}) and then reinstalled by hand. How the heck do you test this anyways?

I do this manually but I agree it is painful :(

It seems to me it would be easier if CSS would be compressed on the fly when the gallery is build instead of when the program is installed and, in any case, only if necessary... Is it so slow that we want to avoid that?

It's not slow, but it needs some code and an additional dependency (cssmin). I'm thinking about just removing the concatenation/minification step for now.

i needed to adjust the style in normalize.css... Not sure it's the right place to do that. I decided to drop the dotted line completely as it looks quite noisy, because everything has a dotted line in there.

Maybe better to put this in the theme css file to avoid losing the change when updating normalize.css.

I also wonder if we should leave the spacing, mm suffixes, f/ and ISO prefixes in there or we should do this in CSS

No strong feelings, just keep it simple 😉

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 5, 2018

Contributor

It's not slow, but it needs some code and an additional dependency (cssmin). I'm thinking about just removing the concatenation/minification step for now.

i think it's still a valuable thing to do. i would do it at build time - i don't think the extra dependency is a problem: it's needed anyways...

Maybe better to put this in the theme css file to avoid losing the change when updating normalize.css.

sorry, what's the theme css file?

No string feelings, just keep it simple 😉

Sorry, what do you mean here? In CSS or inline?

Contributor

anarcat commented Jan 5, 2018

It's not slow, but it needs some code and an additional dependency (cssmin). I'm thinking about just removing the concatenation/minification step for now.

i think it's still a valuable thing to do. i would do it at build time - i don't think the extra dependency is a problem: it's needed anyways...

Maybe better to put this in the theme css file to avoid losing the change when updating normalize.css.

sorry, what's the theme css file?

No string feelings, just keep it simple 😉

Sorry, what do you mean here? In CSS or inline?

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 6, 2018

Owner

i don't think the extra dependency is a problem: it's needed anyways...

Currently it (cssmin) is needed only by theme developers, not by normal users.

sorry, what's the theme css file?

https://github.com/saimn/sigal/blob/master/sigal/themes/galleria/static/css/style.css

Sorry, what do you mean here? In CSS or inline?

Inline seems good enough to me, but if you think you can get a better result with some CSS, it's also fine.

Owner

saimn commented Jan 6, 2018

i don't think the extra dependency is a problem: it's needed anyways...

Currently it (cssmin) is needed only by theme developers, not by normal users.

sorry, what's the theme css file?

https://github.com/saimn/sigal/blob/master/sigal/themes/galleria/static/css/style.css

Sorry, what do you mean here? In CSS or inline?

Inline seems good enough to me, but if you think you can get a better result with some CSS, it's also fine.

anarcat added some commits Jan 7, 2018

move CSS change to theme-specific file
This way our changes will survive eventual normalize.css updates.
move layout and metadata suffixes to CSS
This way this can be override by client-side CSS or more easily controled through the theme
@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 7, 2018

Contributor

okay, i moved it to style.css. i also disabled the dotted line in abbr by default, i found it too noisy. finally, i moved the prefix/suffix stuff (and layout!) into CSS so clients can override it as they wish. it may be a little more obscure for HTML newbies, but i think more people can figure out it and it will make this more flexible.

Contributor

anarcat commented Jan 7, 2018

okay, i moved it to style.css. i also disabled the dotted line in abbr by default, i found it too noisy. finally, i moved the prefix/suffix stuff (and layout!) into CSS so clients can override it as they wish. it may be a little more obscure for HTML newbies, but i think more people can figure out it and it will make this more flexible.

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 8, 2018

Owner

Looks good, thanks !

Owner

saimn commented Jan 8, 2018

Looks good, thanks !

@saimn saimn merged commit 5dfe0ff into saimn:master Jan 8, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 2a9b8a0...2d5cd23
Details
codecov/project 87.48% (+4.12%) compared to 2a9b8a0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@saimn saimn added this to the 1.4 milestone Jan 8, 2018

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Jan 8, 2018

Owner

@anarcat - I forgot to ask you to add yourself to the AUTHOR file, I can do it but I'm not sure if you prefer to have your name or pseudonym?

Owner

saimn commented Jan 8, 2018

@anarcat - I forgot to ask you to add yourself to the AUTHOR file, I can do it but I'm not sure if you prefer to have your name or pseudonym?

@anarcat anarcat deleted the anarcat:more-stats branch Jan 10, 2018

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 10, 2018

Contributor

@saimn either works, thanks!

Contributor

anarcat commented Jan 10, 2018

@saimn either works, thanks!

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jan 31, 2018

Contributor

i opened #291 to followup on the conversation about minifying assets.

Contributor

anarcat commented Jan 31, 2018

i opened #291 to followup on the conversation about minifying assets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment