D16936: Added Windows support to systemvolume plugin

[Albert Vaca Cintora]

albertvaka accepted this revision.
albertvaka added a comment.
This revision is now accepted and ready to land.


  I have a couple minor comments, but looks good to me. It would be nice to implement a really simple MPRIS plugin for Windows now (even if only sending multimedia key inputs), so this can be used form the Android app :P

INLINE COMMENTS

> systemvolumeplugin-win.cpp:260-269
> +{
> +    NetworkPacket np(PACKET_TYPE_SYSTEMVOLUME);
> +    np.set<int>(QStringLiteral("volume"), (int)(pNotify->fMasterVolume * 100));
> +    np.set<QString>(QStringLiteral("name"), name);
> +    enclosing.sendPacket(np);
> +
> +    NetworkPacket np2(PACKET_TYPE_SYSTEMVOLUME);

Can we merge these two into a single packet? Checking the Android side, you will also need to change an "else if" to become an "if", but I think it's cleaner.

> systemvolumeplugin-win.h:28
> +  private:
> +    class CMMNotificationClient : public IMMNotificationClient
> +    {

If these two are private classes, I prefer having them defined in the .ccp file instead of nested here.

REPOSITORY
  R224 KDE Connect

BRANCH
  windows-systemvolume

REVISION DETAIL
  https://phabricator.kde.org/D16936

To: jambon, kdeconnect, #kde_connect, albertvaka
Cc: albertvaka, kdeconnect, shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, Danial0_0, johnq, Pitel, adeen-s, sdepiets, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, mikesomov, tctara, kfunk, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20181119/8c17cf7d/attachment.html>