[Differential] [Changed Subscribers] D4425: Add support for flatpak portals

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> knotificationmanager.cpp:86
> +    if (!runtimeDir.isEmpty()) {
> +        d->inSandbox = QFile::exists(runtimeDir + QLatin1String("/flatpak-info"));
> +    }

`QFileInfo::exists` supposedly is faster

> knotificationmanager.cpp:91
> +    // Also check whether we don't see org.freedesktop.Notifications outside the sandbox
> +    d->portalDBusServiceExists = interface && interface->isServiceRegistered(QStringLiteral("org.freedesktop.portal.Desktop"));
> +

Note that `isServiceRegistered` is a blocking DBus call

Also, is the null check for `interface` required?

> notifybyflatpak.cpp:97
> +{
> +    d->dbusServiceExists = false;
> +

Do that in the constructor of `NotifyByFlatpakPrivate` above

> notifybyflatpak.cpp:159
> +    // close all notifications we currently hold reference to
> +    Q_FOREACH (KNotification *n, d->flatpakNotifications.values()) {
> +        if (n) {

Don't iterate `values()` (creates a temporary list just to iterate it) since `foreach` already does that anyways

> notifybyflatpak.cpp:161
> +        if (n) {
> +            finished(n);
> +        }

`emit`?

> notifybyflatpak.cpp:224
> +
> +    QList<QVariant> args;
> +    // Will be used only with flatpak portal

`QVariantList`

> notifybyflatpak.cpp:247
> +    int actId = 0;
> +    Q_FOREACH (const QString &actionName, notification->actions()) {
> +        actId++;

Can't you do this in one go? You first create a temporary list of actions and then you create a `QVariantMap` from them.

Also, please use `reserve()` if you already know in advance how many items you're going to add to a container.

REPOSITORY
  R289 KNotifications

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

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

To: jgrulich, mck182
Cc: broulik, apol, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170204/1c186bbf/attachment.html>