From kde-core-devel  Sun Aug 30 21:30:27 2009
From: "Jacopo De Simoi" <wilderkde () gmail ! com>
Date: Sun, 30 Aug 2009 21:30:27 +0000
To: kde-core-devel
Subject: Re: Review Request: Jpeg thumbnailer honouring  jpeg rotation info
Message-Id: <20090830213027.14655.26430 () localhost>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=125166785305364



> On 2009-08-30 17:05:15, Peter Penz wrote:
> > I cannot comment on the CMakeList-stuff, but the patch looks fine. Some minor nitpicking:
> > 
> > - I'd replace the following method signature:
> >     const QTransform JpegCreator::orientationMatrix(const int exifOrientation) const
> > by 
> >     QTransform JpegCreator::orientationMatrix(int exifOrientation) const
> > as a passing by value is done and using 'const' only restricts the caller of the method or the implementation inside the method in an unnecessary way (not in this example, but as a general guideline).
> > 
> > - The (!(it == exifData.end())) might be more readable as (it != exifData.end()) (?)
> > 
> > These are just suggestions :-)
> > 
> >

> - The (!(it == exifData.end())) might be more readable as (it != exifData.end()) (?)

Of course, for some reasons, however the compiler complained when I first tried. Now it works.. lsd again?


- Jacopo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1444/#review2191
-----------------------------------------------------------


On 2009-08-30 12:58:47, Jacopo De Simoi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1444/
> -----------------------------------------------------------
> 
> (Updated 2009-08-30 12:58:47)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch makes the jpeg thumbnailer honor jpeg rotation infos stored in exif metadata. 
> The method is quite simple, although I don't like ifdefs, this time they seem to me to be necessary.
> It's my first nontrivial CMake modification; please check that I did not do something stupid there.
> Also, the orientationMatrix method could be ifdeffed if you feel it is necessary; I just didn't want to add another ifdef.
> 
> I am not sure if this is a good solution performance-wise; please comment on that if you have better ideas.
> 
> 
> Diffs
> -----
> 
>   branches/KDE/4.3/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1016603 
>   branches/KDE/4.3/kdebase/runtime/kioslave/thumbnail/jpegcreator.h 1016603 
>   branches/KDE/4.3/kdebase/runtime/kioslave/thumbnail/jpegcreator.cpp 1016603 
> 
> Diff: http://reviewboard.kde.org/r/1444/diff
> 
> 
> Testing
> -------
> 
> Works good with a *clean* .thumbnails directory. Cached thumbnails are indeed a problem; not sure how to solve this issue yet.
> 
> 
> Screenshots
> -----------
> 
> Dolphin showing correctly rotated jpegs
>   http://reviewboard.kde.org/r/1444/s/192/
> 
> 
> Thanks,
> 
> Jacopo
> 
>