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
