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

add size property into Image object containing width and height of each image (thumbnail, big). #164

Merged
merged 4 commits into from Jun 22, 2015

Conversation

2 participants
@franek
Contributor

franek commented Jun 12, 2015

Can be useful for some themes to disinguish orientation of the image.

add size property into Image object containing width and height of ea…
…ch image (thumbnail, big).

Can be useful for some themes to disinguish orientation of the image.

@franek franek referenced this pull request Jun 12, 2015

Merged

add a theme using photoswipe (http://photoswipe.com/). #163

5 of 6 tasks complete
@saimn

This comment has been minimized.

Owner

saimn commented Jun 13, 2015

It would be a good thing to add this, thanks for proposing it. However I would like a different api: get_size should work on only one file (not specific case for the thumbnail, which is just another file), and there should be two properties, Image.size and Image.thumb_size for the thumbnail.
Actually I'm not sure if the get_size function is needed, but one advantage of putting this in a function should be to have a unit test, it would great if you can add one ;).

@@ -371,7 +379,7 @@ def thumbnail(self):
except:
self.logger.error("Failed to open %s", f.src_path)
else:
if im.size[0] > im.size[1]:
if im.size[0] > im.size[1]:

This comment has been minimized.

@saimn

saimn Jun 13, 2015

Owner

In this block of code there is a call to PILImage.open which could be simplified by using the new property, if you want to change that.

This comment has been minimized.

@franek

franek Jun 14, 2015

Contributor

I have tried but I am a python "hacker" (bidouilleur) and did not succeed (got some errors with tests). Sorry about that.

This comment has been minimized.

@saimn

saimn Jun 14, 2015

Owner

Ok, no problem, I will take a look.

Refactor get_size method and add 2 properties to Image (size and thum…
…b_size).

Add tests for get_size method
try:
im = PILImage.open(file_path)
except:
logger.error("Failed to open %s", file_path)

This comment has been minimized.

@saimn

saimn Jun 14, 2015

Owner

With this try/except, the try/except in the properties is useless and can be removed. Also the error message could be more specific, like above (Could not read size ...), and specifying the list of exceptions (IOError, IndexError, TypeError, AttributeError should be fine) is better practise.

test_image)
result = get_size(src_file)
assert result == {'height': 800, 'width': 600}

This comment has been minimized.

@saimn

saimn Jun 14, 2015

Owner

Please add a test for a missing file to test the except clause.

@saimn saimn merged commit c84a621 into saimn:master Jun 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saimn

This comment has been minimized.

Owner

saimn commented Jun 22, 2015

Thanks François !

@saimn saimn referenced this pull request Jun 22, 2015

Closed

Colorbox improvements #152

@saimn saimn modified the milestone: 0.10 Aug 31, 2015

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