D7992: Add systemvolume plugin

[Matthijs Tijink]

mtijink requested changes to this revision.
mtijink added a comment.
This revision now requires changes to proceed.


  Looks good. Is depending on PulseAudioQt okay already?

INLINE COMMENTS

> systemvolumeplugin.cpp:62
> +{
> +    if (np.type() == PACKET_TYPE_SYSTEMVOLUME) {
> +

Do we need to check the packet type here? We already know it's destined for this plugin.

> systemvolumeplugin.cpp:69
> +             } else if (np.has(QStringLiteral("volume"))) {
> +                sinksMap[np.get<QString>(QStringLiteral("name"))]->setVolume(np.get<int>(QStringLiteral("volume")));
> +             } else if (np.has(QStringLiteral("muted"))) {

Needs a check to see if the requested sink still exists.

> systemvolumeplugin.cpp:72
> +                 qCDebug(KDECONNECT_PLUGIN_SYSTEMVOLUME) << "set muted" << np.get<bool>(QStringLiteral("muted"));
> +                 sinksMap[np.get<QString>(QStringLiteral("name"))]->setMuted(np.get<bool>(QStringLiteral("muted")));
> +             }

Same here.

> systemvolumeplugin.cpp:93
> +
> +        connect(sink, &QPulseAudio::Sink::volumeChanged, this, [this, sink] {
> +            NetworkPacket np(PACKET_TYPE_SYSTEMVOLUME);

What happens if we already had that sink? Do we connect to this signal twice?

> systemvolumeplugin.cpp:131
> +        QPulseAudio::Sink* sink = i.value();
> +        sinksMap.insert(sink->name(), sink);
> +    }

This code doesn't connect to the signals, so that's not correct, right?

REPOSITORY
  R224 KDE Connect

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

To: nicolasfella, #kde_connect, mtijink
Cc: zhigalin, albertvaka, davidedmundson, mtijink, #kde_connect, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180324/04e2203d/attachment-0001.html>