[Kde-pim] Review Request: Don't always recompress contact photos when saving

[Kevin Krammer]

> On Oct. 13, 2012, 8:38 a.m., Kevin Krammer wrote:
> > kabc/picture.cpp, line 218
> > <http://git.reviewboard.kde.org/r/106745/diff/3/?file=88606#file88606line218>
> >
> >     If only mData is set and mRawData is empty, then this would not be the same output.
> >     Better keep the old code, but use data() for retrieveing the data (thus covering both cases)
> 
> Sebastian Scheibner wrote:
>     you're right, I missed that, but I'd call rawData(), then the image is not recompressed if mRawData is already set.

hmm, why would it not being recompressed being a problem? can't we assume that the data is in the format indicated by mType, thus actually making the toString() output more applicable than it is now (currently data is always PNG, not matter what type says)


> On Oct. 13, 2012, 8:38 a.m., Kevin Krammer wrote:
> > kabc/picture.cpp, line 233
> > <http://git.reviewboard.kde.org/r/106745/diff/3/?file=88606#file88606line233>
> >
> >     I think for compatibility you'll need to have it like this
> >     return s << picture.d->mIntern << picture.d->mUrl << picture.d->mType << picture.data()
> 
> Sebastian Scheibner wrote:
>     ok, just one thing, my test serializeTestInternRawData() won't work anymore, because the image will be recompressed as jpeg.

the QDataStream operator<< for QImage recompresses as JPEG?


> On Oct. 13, 2012, 8:38 a.m., Kevin Krammer wrote:
> > kabc/tests/picturetest.cpp, line 78
> > <http://git.reviewboard.kde.org/r/106745/diff/3/?file=88608#file88608line78>
> >
> >     I am not sure here, this changes the test.
> >     Maybe setData should not overrwite mType if it is set and use mType when serializing if set?
> 
> Sebastian Scheibner wrote:
>     well the thing is, the QImage that is set with setData doesn't have a type itself. the type is only determined when the image is encoded to png/jpg and that happens in rawData(). If set Data doesn't overwrite the type, and e.g. type() returns "image/png" but rawData() returns a jpeg.

Right. What I meant is that if you setType( image/png) and setData(image), then setData would not overrwrite mType because it is not empty.
And rawData() uses mType to determine target format, thus returning image/png, thus satisfying the test.
if you don't call setType, mType is empty, setData() determines mType depending on alpha channel, again rawData() uses that


> On Oct. 13, 2012, 8:38 a.m., Kevin Krammer wrote:
> > kabc/vcardtool.cpp, line 805
> > <http://git.reviewboard.kde.org/r/106745/diff/3/?file=88609#file88609line805>
> >
> >     I think you'll also need to save the type, otherwise pic will have rawData but no type and saving that would save rawData and empty type information.
> >     
> >     Make sure you have a roundtrip unittest for that
> 
> Sebastian Scheibner wrote:
>     the type is set a few lines below in line 814.
>     I'll try to add a new test file vcardtooltest.cpp to test the parsePicture and createPicture function.

Right, didn't see that, sorry. disregard the issue but add a test just to ensure this in the future


- Kevin


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


On Oct. 7, 2012, 9:52 a.m., Sebastian Scheibner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106745/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2012, 9:52 a.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> When changing a contact with a photo, the photo is recompressed when saving (even though the photo wasn't changed). That's because the photo is loaded into a QImage and when the contact is saved, the QImage is compressed as JPEG again (or PNG if it contains an alpha-channel, which most don't).
> 
> This patch stores the raw base64 photo data from the vcard as a QByteArray in KABC::Picture. If the photo is not changed, this QByteArray is simply used when saving the contact, so there's no recompression and quality loss. If the photo was changed it gets compressed as JPEG as before.
> 
> fixes this bug: https://bugs.kde.org/show_bug.cgi?id=307570
> 
> 
> This addresses bug 307570.
>     http://bugs.kde.org/show_bug.cgi?id=307570
> 
> 
> Diffs
> -----
> 
>   kabc/picture.h bf9745c 
>   kabc/picture.cpp 0a51c8b 
>   kabc/tests/picturetest.h 2b33e18 
>   kabc/tests/picturetest.cpp b599b48 
>   kabc/vcardtool.cpp a742788 
> 
> Diff: http://git.reviewboard.kde.org/r/106745/diff/
> 
> 
> Testing
> -------
> 
> compiled kdepimlibs/kabc with kde 4.9.2
> 
> - created a new contact in kaddressbook and added a photo. exported the contact.
> - changed the photo and exported the contact again
> - the image data in the exported file didn't change
> 
> 
> Thanks,
> 
> Sebastian Scheibner
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/