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 GIF support — Fixes #140 #185

Merged
merged 6 commits into from Jan 8, 2016

Conversation

3 participants
@Natim
Contributor

Natim commented Dec 15, 2015

No description provided.

@saimn

This comment has been minimized.

Owner

saimn commented Dec 15, 2015

Great to see some work on this, thanks Rémy !
Last time I looked at GIF support, I was not sure how to handle the resize operations. You solved the first one, using a copy for the normal size image, which is fine, but there is also the generated thumbnail. Is it working ?
It would be nice to have a test image in tests/sample/pictures/ to check if it works.
Also please add yourself to AUTHORS :)

@saimn

This comment has been minimized.

Owner

saimn commented Dec 15, 2015

Ref #140

@saimn saimn added this to the 1.1.0 milestone Dec 15, 2015

@Natim

This comment has been minimized.

Contributor

Natim commented Dec 16, 2015

As for video I didn't created an animated thumbnail.
Also it looks like GIF are already optimised (and are probably shipped in a size lower than the one we want to resize them in).

Can you help me to list the tests you want to add?

  • One to see that gif aren't ignored
  • One to see that gif file are thumbnailed
  • One to see that gif image are copied and not resized.
@saimn

This comment has been minimized.

Owner

saimn commented Dec 16, 2015

Your list seems good. There is a test that simply ensures that the test gallery can be build, so if there is a GIF it is already a start. Then I just want to be sure that we can generate a thumbnail for a GIF file. The thumbnail should probably be a PNG file ?

@Natim

This comment has been minimized.

Contributor

Natim commented Dec 16, 2015

Apparently for now they are gif files as well but not animated ones.

@saimn

This comment has been minimized.

Owner

saimn commented Dec 16, 2015

Ok, then it's fine. I was not sure that Pillow is able to resize a GIF.

@saimn

This comment has been minimized.

Owner

saimn commented Jan 7, 2016

Hi @Natim, any update on this ?

Rémy HUBSCHER

@Natim Natim force-pushed the Natim:140-support-gif-images branch from ec2cf13 to 7f3ee7a Jan 8, 2016

@Natim

This comment has been minimized.

Contributor

Natim commented Jan 8, 2016

I tried but I didn't really find out where to put my file.

I added tests/sample/pictures/dir1/test1/50a1d0bc-763d-457e-b634-c87f16a64270.gif but only test_zipped_correctly is failing did I miss something?

Rémy HUBSCHER added some commits Jan 8, 2016

@Natim

This comment has been minimized.

Contributor

Natim commented Jan 8, 2016

Ok so my last commit seems to do what you wanted as a first test. I am going to write one for the thumbnail as well.

@saimn

This comment has been minimized.

Owner

saimn commented Jan 8, 2016

Yeah the tests are not complete :(
test_album_medias just tests that the Album objet is correctly created but it doesn't parse the directory, the list of files is given in input. So actually the REF dict is not used to test that all the files have been parsed.
And test_gallery just tests that the gallery builds without errors. It would be a good idea to improve this test to check with REF that all files have been parsed, but you can leave that for now. Having at least one GIF file processed in these tests is enough for now.

@saimn

This comment has been minimized.

Owner

saimn commented Jan 8, 2016

Please add yourself to AUTHORS :)

Rémy HUBSCHER added some commits Jan 8, 2016

@Natim

This comment has been minimized.

Contributor

Natim commented Jan 8, 2016

Here you go, fully tested 👍

@saimn

This comment has been minimized.

Owner

saimn commented Jan 8, 2016

Thanks for adding GIF in the image tests, though it can be made simpler to avoid code duplication. As the tests you added seems exactly the same as test_generate_thumbnail and test_generate_image_passthrough, you could use pytest's parametrize feature:

@pytest.mark.parametrize("image", [TEST_IMAGE, TEST_GIF_IMAGE])
def test_generate_image_passthrough(tmpdir, image):
    dstfile = str(tmpdir.join(image))
    ...
@Natim

This comment has been minimized.

Contributor

Natim commented Jan 8, 2016

Yes sure let's do that 👍

@Natim

This comment has been minimized.

Contributor

Natim commented Jan 8, 2016

That's not awful but not really pretty either...

@saimn

This comment has been minimized.

Owner

saimn commented Jan 8, 2016

Seems good, many thanks Rémy !

saimn added a commit that referenced this pull request Jan 8, 2016

@saimn saimn merged commit 6a21251 into saimn:master Jan 8, 2016

1 check passed

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

This comment has been minimized.

almet commented Jan 8, 2016

You both rock :-)

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