Possible solution for issue #175 #199

Merged
merged 9 commits into from Jun 16, 2013

Projects

None yet

2 participants

@jrief
Collaborator
jrief commented Feb 1, 2013

This is a proof of concept how this issue could be solved.
If settings.THUMBNAIL_HIGH_RESOLUTION is True (default), always two images are generated, the default one and a high resolution thumbnail.
The HTML code always references the default one. This avoids caching issues, since HTML pages very often are cached.

Now, if the client uses this Javascript library http://retinajs.com/ all img src tags are rewritten to the high resolution image if the client runs on a high quality display. Otherwise, or, if the hq-image can't be found, the default images is loaded.

This solution also avoids all caching issues for images.

@SmileyChris SmileyChris and 1 other commented on an outdated diff Feb 3, 2013
easy_thumbnails/files.py
filename = self.get_thumbnail_name(thumbnail_options,
transparent=utils.is_transparent(thumbnail_image))
+ if high_resolution:
+ filename = self.get_high_resolution_filename(filename)
@SmileyChris
SmileyChris Feb 3, 2013 Owner

Let's combine the high resolution name logic into the standard get_thumbnail_name with a new high_resolution=False parameter.

@jrief
jrief Feb 3, 2013 Collaborator

OK, I will change this.

@jrief
jrief Feb 6, 2013 Collaborator

This doesn't work out. The problem is, that self.get_thumbnail_name can be overloaded by another class - in my case thats ThumbnailerNameMixin from https://github.com/stefanfoulis/django-filer/blob/develop/filer/utils/filer_easy_thumbnails.py. Unless I change that code too, I can't fix it on the easy-thumbnails side. Sure, splitting a just recombined filename again, is not the cleanest solution.
I will ask stefanfoulis about his opinion.

@SmileyChris
SmileyChris Feb 6, 2013 Owner

Fine, just stick with the get_high_resolution_filename then.

@SmileyChris SmileyChris and 1 other commented on an outdated diff Feb 3, 2013
easy_thumbnails/files.py
@@ -296,8 +300,13 @@ def generate_thumbnail(self, thumbnail_options):
self.thumbnail_processors)
quality = thumbnail_options.get('quality', self.thumbnail_quality)
+ # restore original size to fake smaller image
+ thumbnail_options['size'] = orig_size
@SmileyChris
SmileyChris Feb 3, 2013 Owner

Just for clarity, do this inside an if high_resolution: conditional.

@jrief
jrief Feb 3, 2013 Collaborator

It is safe to restore the orig_size, even if it did not change. But yes, code readability counts.

@SmileyChris SmileyChris and 1 other commented on an outdated diff Feb 3, 2013
easy_thumbnails/files.py
@@ -310,6 +319,13 @@ def generate_thumbnail(self, thumbnail_options):
return thumbnail
+ @staticmethod
+ def get_high_resolution_filename(filename):
+ filename_parts = filename.split('.')
+ filename_parts[-2] += '@2x'
@SmileyChris
SmileyChris Feb 3, 2013 Owner

Just an idea, I wonder if we could have the name format as part of the actual setting? Like THUMBNAIL_HIGH_RESOLUTION = '%(name)s.hd.%(ext)s' ?
Is '@2x' really that much of a standard?

@jrief
jrief Feb 3, 2013 Collaborator

This is a convention introduced by Apple: http://developer.apple.com/library/ios/#documentation/2DDrawing/Conceptual/DrawingPrintingiOS/SupportingHiResScreensInViews/SupportingHiResScreensInViews.html#//apple_ref/doc/uid/TP40010156-CH15-SW8
This convention is also used by this client side Javascript library: http://retinajs.com/ so from my point of view is is not a good idea to make this configurable.

@SmileyChris
SmileyChris Feb 3, 2013 Owner

That's standard enough for me. No need for arbitrary extra configuration options.

@SmileyChris
SmileyChris Feb 6, 2013 Owner

It'd be much better to actually use the os commands to split/join the base and extension than assume the filename doesn't contain any extra dots

@SmileyChris SmileyChris and 1 other commented on an outdated diff Feb 3, 2013
easy_thumbnails/conf.py
@@ -120,5 +120,6 @@ class Settings(AppSettings):
THUMBNAIL_DEFAULT_OPTIONS = None
+ THUMBNAIL_HIGH_RESOLUTION = True # enables thumbnails for retina displays
@SmileyChris
SmileyChris Feb 3, 2013 Owner

Set it to False by default.

@jrief
jrief Feb 3, 2013 Collaborator

Its OK, if you prefer performance over usability.
My thought was that having this enabled does not any harm (except some milliseconds of wasted server CPU time).
My intention is to enable it by default, because many developers do not have the ability to distinguish between high- and standard resolution images, so they might forget (or not think about) to enable this. But a few percent (increasing) of their clients will benefit from this setting.

@SmileyChris
SmileyChris Feb 3, 2013 Owner

Having this setting as True doesn't magically make retina images work. If the developer is going to implement the javascript, they can switch this on at the same time. This is an additional feature which takes additional CPU and storage, when only a small percentage of people will actually implement.

@jrief
Collaborator
jrief commented Feb 6, 2013

I just just response from django-filer: divio/django-filer#296
This means, I will also make a pull request there.

  • Jacob
@jrief
Collaborator
jrief commented Feb 20, 2013

Just created a pull request for django-filer (see divio/django-filer#299) which solves annotation 1.
Solved annotation 2.
Added unit test to check if high resolution image is created and has double size.

@SmileyChris SmileyChris commented on an outdated diff Feb 20, 2013
easy_thumbnails/tests/files.py
@@ -111,6 +111,17 @@ def test_extensions(self):
thumb = self.ext_thumbnailer.get_thumbnail({'size': (100, 100)})
self.assertEqual(path.splitext(thumb.name)[1], '.jpg')
+ def test_high_resolution(self):
+ thumbnail_high_resolution = getattr(settings, 'THUMBNAIL_HIGH_RESOLUTION', False)
+ setattr(settings, 'THUMBNAIL_HIGH_RESOLUTION', True)
+ self.ext_thumbnailer.thumbnail_extension = 'jpg'
+ thumb = self.ext_thumbnailer.get_thumbnail({'size': (100, 100)})
+ hires_thumb_file = path.join(path.dirname(thumb.path), path.splitext(thumb.name)[0] + '@2x.jpg')
+ self.assertTrue(path.exists(hires_thumb_file))
+ thumb = Image.open(hires_thumb_file)
+ self.assertEqual(thumb.size, (200, 150))
+ settings.THUMBNAIL_HIGH_RESOLUTION = thumbnail_high_resolution
@SmileyChris
SmileyChris Feb 20, 2013 Owner

It's good practice to ensure that settings will be changed back even if the test fails. Put the reversion of settings in the finally of a try/finally block

@SmileyChris SmileyChris commented on an outdated diff Feb 20, 2013
easy_thumbnails/files.py
@@ -420,6 +435,11 @@ def thumbnail_exists(self, thumbnail_name):
# Try to use the local file modification times first.
source_modtime = self.get_source_modtime()
thumbnail_modtime = self.get_thumbnail_modtime(thumbnail_name)
+ if settings.THUMBNAIL_HIGH_RESOLUTION:
+ filename_parts = thumbnail_name.split('.')
@SmileyChris
SmileyChris Feb 20, 2013 Owner

This still needs to be changed to using os.splitext

@jrief
Collaborator
jrief commented Feb 20, 2013

Fixed the latest two annotations.

@jrief
Collaborator
jrief commented Mar 11, 2013

It's advisable to merge this pull request since the corresponding pull request in django-filer divio/django-filer#299 has already been merged 15 days ago.
django-filer currently relies on this pull request of easy-thumbnails, which has not yet been merged. This causes some compatibility issues.

@SmileyChris
Owner

Thanks for the reminder, I'll do a final review and merge today

@jrief jrief referenced this pull request in jrief/angular-retina Mar 21, 2013
Closed

Add configuration option to customize retina path #2

@jrief
Collaborator
jrief commented Jun 6, 2013

Is there any objection on this pull request?
The problem is, that without this merge, django-filer is incompatible.

@SmileyChris SmileyChris merged commit 4b94240 into SmileyChris:master Jun 16, 2013
@SmileyChris
Owner

Merged (and released as part of 1.3)

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