[Differential] [Commented On] D3606: Listen for broadcast notifications on system bus

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Note that actions won't work, we'd always be replying on the session bus. (this is solvable)

INLINE COMMENTS

> notificationsengine.cpp:70
> +    // TODO only do that when plasmashellrc has [Notifications] BroadcastAllowed or sth like that
> +    QDBusConnection::systemBus().connect({}, {}, QStringLiteral("org.kde.plasmashell.broadcastNotification"),
> +                                         QStringLiteral("Notify"), this, SLOT(onBroadcastNotification(QMap<QString,QVariant>)));

I'd get rid of the plasmashell in the iface name.
Otherwise you're tying an implementation detail into an API.

Also for whatever reason convention is to be UpperCamelCase (i.e BroadcastNotification not broadcastNotification)

> notificationsengine.cpp:426
> +    auto userId = properties.value(QStringLiteral("uid")).toULongLong();
> +    if (userId) {
> +        userIds.append(userId);

is it intended that you can't send a message to root?

otherwise toULongLong(&rc );
if (rc) {..}

REPOSITORY
  R120 Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: davidedmundson, mart, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161208/e0402a0c/attachment.html>