Add support for flatpak portals
ClosedPublic

Authored by jgrulich on Feb 3 2017, 9:02 AM.

Details

Summary

Creates a new flatpak notification plugin, which just forwards messages through
DBus to xdg-desktop-portal service, otherwise there is no way for sandboxed applications
to send notifications because of limited access. This plugin is used instead of
notify by popup plugin if we detect we are in sandbox and the xdg-desktop-portal
service is running

Diff Detail

Repository
R289 KNotifications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
jgrulich updated this revision to Diff 10890.Feb 3 2017, 9:02 AM
jgrulich retitled this revision from to Add support for flatpak portals.
jgrulich updated this object.
jgrulich edited the test plan for this revision. (Show Details)
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 3 2017, 9:02 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
apol added a subscriber: apol.Feb 4 2017, 10:18 AM

Some coding style suggestions.
General +1.

src/knotificationmanager.cpp
85 ↗(On Diff #10890)

Use qEnvironmentVariableIsEmpty?

src/notifybyflatpak.cpp
2 ↗(On Diff #10890)

Are you sure so many people? :P

112 ↗(On Diff #10890)

Use new connect syntax.

255 ↗(On Diff #10890)

Something like

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

would be more readable?

297 ↗(On Diff #10890)

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

broulik added inline comments.
src/knotificationmanager.cpp
86 ↗(On Diff #10890)

QFileInfo::exists supposedly is faster

91 ↗(On Diff #10890)

Note that isServiceRegistered is a blocking DBus call

Also, is the null check for interface required?

src/notifybyflatpak.cpp
97 ↗(On Diff #10890)

Do that in the constructor of NotifyByFlatpakPrivate above

159 ↗(On Diff #10890)

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

161 ↗(On Diff #10890)

emit?

224 ↗(On Diff #10890)

QVariantList

247 ↗(On Diff #10890)

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.

jgrulich updated this revision to Diff 10951.Feb 6 2017, 7:38 AM
jgrulich marked 11 inline comments as done.
  • Fix mentioned issues
apol added inline comments.Feb 6 2017, 2:02 PM
src/knotificationmanager.cpp
93

Maybe instead of querying for dbus things we could figure out a what QPlatformTheme is in use somehow?

jgrulich added inline comments.Feb 6 2017, 2:10 PM
src/knotificationmanager.cpp
93

Not sure how that would help. It would help maybe to detect whether we are in sandbox if we check whether the platform plugin is "flatpak", but with this check I want to make notifications work at least somehow, even in case we are in sandbox and the portal service is not available from some reason. If you use NotifyByPopup plugin in the sandbox then it will show the ugly notification on top of the screen instead so it's at least some notification. In case you would like to avoid displaying this ugly notification and rely on portals only then we can ignore this check and use NotifyByFlatpak regardless availability of the portal service.

src/notifybyflatpak.cpp
2 ↗(On Diff #10890)

It's a modified "NotifyByPopup" plugin and I didn't change the copyright except adding myself.

apol added inline comments.Feb 8 2017, 10:31 AM
src/knotificationmanager.cpp
56

These could be a local variable, it's not being used outside for figuring out the backend.

jgrulich updated this revision to Diff 11052.Feb 8 2017, 10:37 AM

Fix mentioned issues

jgrulich marked an inline comment as done.Feb 8 2017, 10:39 AM

Patch updated to address all mentioned issues. Re-check it please. Thank you.

broulik accepted this revision.Feb 8 2017, 10:48 AM
broulik added a reviewer: broulik.

Good stuff!

This revision is now accepted and ready to land.Feb 8 2017, 10:48 AM
This revision was automatically updated to reflect the committed changes.