[Differential] [Commented On] D4425: Add support for flatpak portals

[Aleix Pol Gonzalez]

apol added a comment.


  Some coding style suggestions.
  General +1.

INLINE COMMENTS

> knotificationmanager.cpp:85
> +    const QString runtimeDir = qgetenv("XDG_RUNTIME_DIR");
> +    if (!runtimeDir.isEmpty()) {
> +        d->inSandbox = QFile::exists(runtimeDir + QLatin1String("/flatpak-info"));

Use `qEnvironmentVariableIsEmpty`?

> notifybyflatpak.cpp:2
> +/*
> +   Copyright (C) 2005-2006 by Olivier Goffart <ogoffart at kde.org>
> +   Copyright (C) 2008 by Dmitry Suzdalev <dimsuz at gmail.com>

Are you sure so many people? :P

> notifybyflatpak.cpp:112
> +    watcher->addWatchedService(portalDbusServiceName);
> +    connect(watcher, SIGNAL(serviceOwnerChanged(QString,QString,QString)),
> +            SLOT(onServiceOwnerChanged(QString,QString,QString)));

Use new connect syntax.

> notifybyflatpak.cpp:255
> +    for (int i = 0; i < actionList.count(); i += 2) {
> +        QVariantMap button;
> +        button.insert(QStringLiteral("action"), actionList.at(i));

Something like

  QVariantMap button = {
  { StringLiteral("action"), actionList.at(i)},
  {QStringLiteral("label"), actionList.at(i + 1)}
  };

would be more readable?

> notifybyflatpak.cpp:297
> +    args.append(QString::number(id));
> +    m.setArguments(args);
> +

`m.setArguments({ QString::number(id) });`

REPOSITORY
  R289 KNotifications

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

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

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