Review Request 126722: notifications: also sync icon-data from notfication hints

[Holger Kaelberer]

> On Jan. 13, 2016, 12:27 a.m., Aleix Pol Gonzalez wrote:
> > plugins/notifications/notificationslistener.cpp, line 115
> > <https://git.reviewboard.kde.org/r/126722/diff/2/?file=430490#file430490line115>
> >
> >     Couldn't this guy just return the QImage?

Hm, this function wasn't there before and was added as virtual to be able to add test-code for the image-data parsing. The problem is that the QDBusArgument does not allow for reading *and* writing without making an actual dbus call, therefore I could not simply inject a QDBusArgument in the hints in the testing code. ATM the test-class overrides this method and uses a QVariantMap instead.

If it would return QImage, I'd need to duplicate code in the test-class. Therefore I'd prefer keeping it as is -- or eliminate it again at the cost of dropping the corresponding test-code.


> On Jan. 13, 2016, 12:27 a.m., Aleix Pol Gonzalez wrote:
> > plugins/notifications/notificationslistener.cpp, line 119
> > <https://git.reviewboard.kde.org/r/126722/diff/2/?file=430490#file430490line119>
> >
> >     Isn't 8 a bit random here?

It's a restriction imposed because I only use QImage::Format_ARGB32 and QImage::Format_RGB32 as QImage target formats. I only saw applications producing 4x8 bit rgba image formats in image-data hints and wonder if there is any that uses color formats with other than 8 bits per color channel. So without knowing which combinations of bitsPerSample and hasAlpha and channels we can realistically expect and given the pretty specific formats in QImage::Format I thought this restriction would be a good tradeoff.

The notifications spec is also not very verbose about what formats one should use.

There are probably some more we could handle easily (without needing to rewrite the data for QImage compatibility), like

bitsPerSample==6 && channels==3 && !hasAlpha --> QImage::Format_RGB666
etc.

Although I don't know how to test these ones. And I doubt that there are many applications out there that produce other than 8bit per channel.

Shall I try to be more more complete here?


> On Jan. 13, 2016, 12:27 a.m., Aleix Pol Gonzalez wrote:
> > plugins/notifications/notificationslistener.cpp, line 227
> > <https://git.reviewboard.kde.org/r/126722/diff/2/?file=430490#file430490line227>
> >
> >     Why don't you use `QScopedPointer<QIODevice>` everywhere?
> >     
> >     Construction is cheap and right now it's hard to tell how are the QBuffers deleted.

Ok.

As payload is attached to the NetworkPackage as QSharedPointer<QIODevice> I guess I better use this smart pointer directly everywhere.


- Holger


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126722/#review90983
-----------------------------------------------------------


On Jan. 12, 2016, 6:18 p.m., Holger Kaelberer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126722/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 6:18 p.m.)
> 
> 
> Review request for kdeconnect.
> 
> 
> Repository: kdeconnect-kde
> 
> 
> Description
> -------
> 
> - follows the priorities defined in the notifications spec version 1.2
> - seems that payload can only be attached via QIODevices/files, therefore needed to create a temp file from the in-memory image data first; if there is another way - teach me pls!
> - QDBusArgument-s can't be faked for testing without a real DBUS-call because they can be created either only for reading or for writing. Therefore added the virtual parseImageData function to use another datatype in the unit-tests
> 
> 
> Diffs
> -----
> 
>   plugins/notifications/notificationslistener.h dfa37b8 
>   plugins/notifications/notificationslistener.cpp d21f3d7 
>   tests/testnotificationlistener.cpp b43a7d3 
> 
> Diff: https://git.reviewboard.kde.org/r/126722/diff/
> 
> 
> Testing
> -------
> 
> sure
> 
> 
> Thanks,
> 
> Holger Kaelberer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20160113/ffd925f6/attachment.html>