schooltool-developers team mailing list archive

[Douglas Cerna <douglascerna@xxxxxxxxx>]

--- On Thu, 2/16/12, Justas <justas@xxxxxx> wrote:

> > I'd appreciate your feedback on functionality and code
> issues.
> 
>   PhotoView.__call___: # XXX: what to do if photo is
> None? redirect?
>   - Maybe throw NotFound?

Done.

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

Done.

>   Nitpicking
>      - do not hide defaults like
> DESIRED_SIZE inside methods.  Make those class
> attributes or something.

Sorry. Moved it to a class attribute.

>      - 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.

Will ask you about this one in the next meeting :)

>      - image.save(f,
> image.format.upper(), **kw).  Is there a point of doing
> .upper() there?

I just noticed that PIL.Image stores several stuff (plugins, mimetypes, etc) using uppercase formats. I moved the string to a class attribute (FORMAT).

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

I'd say yes. In fact, in my CSS TODO list I have a task to add vendor prefixes to some of our css3 rules: -moz-*, -o-*, -webkit-*, etc.

>   May be worth considering:
> 
>     - store all images as PNG.

I used JPEG.

>     - convert all images to RGB or RGBA
> mode.  Paletted (GIF) -> convert to RGB -> pretty
> scaling with resampling and all.

All images are converted to RGB JPEG, even if they're PNGs with transparencies:

http://69.164.203.135:6662/persons/student199

>     - do not accept large data.  No 1GB image
> transforms to thumbnails killing the server.

I limited the upload size to 10 MB, and put the check before processing anything.

>     - do not accept large images.  No
> 10000x10000 px image transforms - it's a photo, not a map of
> Washington D.C.

Maybe the upload size is enough?

>     - 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?

Didn't implement this. Version 2 of the feature? ;)

>     - scale images up, if they're smaller than
> desired size?

Didn't scale them up, just centered them in a 99x132 "canvas":

http://69.164.203.135:6662/persons/student299

>     - when editing person info, it would be nice
> to see the current photo.

Done: http://69.164.203.135:6662/persons/student399/@@edit.html

> 
>   Well, that's my two cents.

Thanks Justas! When you're OK with these changes I'll merge my branch.

Now I need your help with another thing:

I tried to insert a photo in a PDF. Just for testing, I tried to modify the default report template inserting an image directive with its file attribute pointing to the photo url.

The problem is that z3c.rml.attr.File.fromUnicode uses urllib.urlopen to retrieve the resource, which produces a Forbidden page, since the request doesn't have session info :( You end up with an IOError, because PIL.Image tries to open the HTML.

Is there a way to modify z3c.rml behavior when processing <image ...> directives?

Let me know what you think.

Thanks.

Douglas

"... allí­ es cuando te das cuenta que las cosas malas pueden resultar bastante buenas..." - Lionel Messi

Por favor, evite enviarme adjuntos de Word, Excel o PowerPoint.
Vea http://www.gnu.org/philosophy/no-word-attachments.es.html