schooltool-developers team mailing list archive

[Justas <justas@xxxxxx>]

On 02/16/2012 02:53 AM, Douglas Cerna wrote:
> Gentlemen:
>
> I've implemented most of the functionality Justas and I discussed during the sprint for the photo work:
>
> * Use of blobs for storing the images (using zope.file's File objects)
>
> * Data converter that accepts only image mime types (JPEG, PNG, GIF and BMP). If you try to upload a PDF, you should get a validation error. The converter also resizes the image, using 99px for maximum width or 128 for maximum height. This is the ratio of the passport photo. Of course, we can modify these values to store larger images, I just chose those because it's the area of the image in the person index view
>
> * The person index view adapts the general information table width if there's a photo, so the photo doesn't overlap the table
>
> * There's a new @@photo view on person objects that returns the image data setting the appropriate headers, such as content type and length. It also sets a If-Modified-Since header according to the last time the person object was modified. This allows the browser to use its cache instead of requesting the image on every request
>
> * The edit person view allows the user to delete the existing image or upload a new one
  Nice!

> You can see it in action:
>
> http://69.164.203.135:6662/persons/douglas (vertical JPEG)
> http://69.164.203.135:6662/persons/camila (horizontal PNG)
> http://69.164.203.135:6662/persons/teacher001 (GIF)
> http://69.164.203.135:6662/persons/teacher002 (BMP)

  Very nice :)

> The branch is at:
>
> lp:~replaceafill/schooltool/flourish_photos
>
> I'd appreciate your feedback on functionality and code issues.

  PhotoView.__call___: # XXX: what to do if photo is None? redirect?
  - Maybe throw NotFound?

  We should get rid of PhotoDataConverter.getImageInfo, and use 
image.size() and image.format instead.

  Nitpicking
     - do not hide defaults like DESIRED_SIZE inside methods.  Make 
those class attributes or something.
     - PhotoFieldFormAdapter: it's slightly nicer to use 
object.__setattr__ instead of self.__dict__['foo'] = bar -- that said, I 
failed to do this in many many places.
     - image.save(f, image.format.upper(), **kw).  Is there a point of 
doing .upper() there?

  flourish.css:  "border-radius: 4px;".  Is it worth adding 
-moz-border-radius?..  Maybe not anymore.

  May be worth considering:

    - store all images as PNG.
    - convert all images to RGB or RGBA mode.  Paletted (GIF) -> 
convert to RGB -> pretty scaling with resampling and all.
    - do not accept large data.  No 1GB image transforms to thumbnails 
killing the server.
    - do not accept large images.  No 10000x10000 px image transforms - 
it's a photo, not a map of Washington D.C.
    - save image.copy() (after the thumbnail transform) to keep only 
raster data.  Do we really need app headers like image comments, GPS 
coordinates and other waste?
    - scale images up, if they're smaller than desired size?
    - when editing person info, it would be nice to see the current photo.

  Well, that's my two cents.

Nice work!
Justas