Skip to content

Commit

Permalink
Smart, semantic exception handling for image resizing. refs #3379
Browse files Browse the repository at this point in the history
  • Loading branch information
whatisjasongoldstein committed May 3, 2013
1 parent 9ec2750 commit 835f1b2
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 25 deletions.
15 changes: 10 additions & 5 deletions ckeditor/fields.py
@@ -1,8 +1,10 @@
from django.db import models
from django import forms
from django.core.exceptions import ValidationError
from ckeditor.utils.image_resize import ImageResizeError

import ckeditor.signals as signals
from ckeditor.widgets import CKEditorWidget
from ckeditor.utils.image_resize import resize_images


class RichTextField(models.TextField):
Expand All @@ -11,6 +13,13 @@ def __init__(self, *args, **kwargs):
self.dynamic_resize = kwargs.pop("dynamic_resize",False)
super(RichTextField, self).__init__(*args, **kwargs)

def validate(self, value, model_instance):
super(RichTextField, self).validate(value, model_instance)
try:
resize_images(value)
except ImageResizeError as e:
raise ValidationError(unicode(e))

def formfield(self, **kwargs):
defaults = {
'form_class': RichTextFormField,
Expand All @@ -19,10 +28,6 @@ def formfield(self, **kwargs):
defaults.update(kwargs)
return super(RichTextField, self).formfield(**defaults)

def contribute_to_class(self, cls, name):
super(RichTextField, self).contribute_to_class(cls, name)
if self.dynamic_resize:
signals.add_dynamic_resize(cls, name)

class RichTextFormField(forms.fields.Field):
def __init__(self, config_name='default', *args, **kwargs):
Expand Down
14 changes: 0 additions & 14 deletions ckeditor/signals.py

This file was deleted.

Binary file added ckeditor/static/tests/opaque.gif
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ckeditor/static/tests/transparent.gif
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
37 changes: 31 additions & 6 deletions ckeditor/utils/image_resize.py
@@ -1,9 +1,9 @@
import uuid
import os
import shutil
import sys
import urlparse
import re
import hashlib
import logging

from lxml import html
from PIL import Image, ImageFile
Expand All @@ -16,6 +16,11 @@

ImageFile.MAXBLOCKS = 10000000

logger = logging.getLogger(__name__)

class ImageResizeError(Exception):
pass


def match_or_none(string, rx):
"""
Expand Down Expand Up @@ -174,6 +179,9 @@ def transcode_to_jpeg(image, path, width, height):
new_image.save(new_path, quality=80, optimize=1)
return new_path

def _get_resize_error_message(path):
return 'There was a problem resizing this image: {0}'.format(os.path.split(path)[1])

def re_render(path, width, height):
"""
Given an original image, width, and height, creates a thumbnailed image
Expand All @@ -193,6 +201,7 @@ def re_render(path, width, height):
@return: Path to the 'rendered' image.
@rtype: "/path/to/image"
"""

try:
image = Image.open(path)
except IOError:
Expand Down Expand Up @@ -230,15 +239,21 @@ def re_render(path, width, height):
return new_path

# Re-render the image, optimizing for filesize
new_image = image.resize((width, height), Image.ANTIALIAS)
try:
new_image = image.resize((width, height), Image.ANTIALIAS)
except TypeError:
raise ImageResizeError(_get_resize_error_message(path)), None, sys.exc_info()[2]

image_params = {}
if image.format != "GIF":
image_params = dict(
quality = 80,
optimize = 1,
)
new_image.save(new_path, **image_params)
try:
new_image.save(new_path, **image_params)
except IOError:
raise ImageResizeError(_get_resize_error_message(path)), None, sys.exc_info()[2]

This comment has been minimized.

Copy link
@fdintino

fdintino May 3, 2013

Member

It might help to have some information about the exception passed along. Something I used to do in cropduster was catch IOError, then do

try:
    new_image.save(new_path, **image_params)
except IOError, e:
    matches = re.search(r"No such file or directory: u?'(.+)'$", unicode(e))
    if not matches:
        raise e
    msg = u"There was a problem resizing this image: Could not find file: %s" % os.path.split(matches.group(1))
    raise ImageResizeError(msg), None, sys.exc_info()[2]

The rationale behind this is that there could be other, unknown IOError exceptions that we should log rather than present to the user.

For the same reason I might consider passing the message of the TypeError above along to the user after the boilerplate "There was a problem resizing this image"

return new_path

def get_html_tree(content):
Expand All @@ -258,17 +273,27 @@ def resize_images(post_content):
@return: Modified contents.
@rtype: basestring
"""

# Get tree
tree = get_html_tree(post_content)

# Get images
imgs = tree.xpath('//img[starts-with(@src, "%s")]' % settings.STATIC_URL)
for img in imgs:
orig_url = img.attrib['src']
orig_path = get_local_path(orig_url)

width, height = get_dimensions(img)
rendered_path = re_render(orig_path, width, height)
try:
rendered_path = re_render(orig_path, width, height)
except ImageResizeError:
raise
except Exception as e:
# If something goes wrong, just use the original path so as not to
# interrupt the user. However, log it, because it's still a real problem.
logger.error(e, exc_info=True, extra={
'stack': True,
})
rendered_path = orig_path

This comment has been minimized.

Copy link
@fdintino

fdintino May 3, 2013

Member

I think continue would make more sense here. This assumes that the lines immediately following it are

if rendered_path == orig_path:
    continue

which may not always be the case.

When making a minor change like that on a pull request, I checkout the branch, then use the workflow described in this stackoverflow, and then run a git push -f origin branchname. This will update the pull request. Git helpfully retains any previous commits and comments on the pull request page. I used this technique extensively in my pull request for pip, which made the merge two commits, rather than the insane hundred-or-so commits I would have had.


# If we haven't changed the image, move along.
if rendered_path == orig_path:
Expand Down

0 comments on commit 835f1b2

Please sign in to comment.