[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Jpeg thumbnailer honouring  jpeg rotation info
From:       "Jacopo De Simoi" <wilderkde () gmail ! com>
Date:       2009-08-30 21:30:27
Message-ID: 20090830213027.14655.26430 () localhost
[Download RAW message or body]



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


[prev in list] [next in list] [prev in thread] [next in thread]