[libnotificationmanager] introduce the WatchedNotificationsModel
ClosedPublic

Authored by bshah on Apr 2 2020, 9:56 AM.

Details

Summary

This allows one to subscribe to notifications from notification server.

Test Plan

tested using very simple QML

import QtQuick 2.0
import org.kde.notificationmanager 1.1 as Notifications

...
Notifications.NotificationWatchedModel {
    id: model
}

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bshah created this revision.Apr 2 2020, 9:56 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 2 2020, 9:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bshah requested review of this revision.Apr 2 2020, 9:56 AM
broulik added inline comments.Apr 2 2020, 10:07 AM
libnotificationmanager/dbus/org.freedesktop.Notifications.xml
55 ↗(On Diff #79122)

Please don't put that on the fdo service

libnotificationmanager/notificationwatcher.cpp
45 ↗(On Diff #79122)

Do we not want to check if that succeeded? I think having a valid property on the watcher could be nice

libnotificationmanager/notificationwatcher.h
41 ↗(On Diff #79122)

Don't leak this implementation detail into the public API. Instead, put all of the dbus handling into a private class, like is done with Server vs ServerPrivate

47 ↗(On Diff #79122)

Use d-pointer for exported class

libnotificationmanager/server_p.cpp
256

Don't block

283

Don't we want to adhere to an interface?

536

Then you can probably also use the watcher's "watched services" list rather than storing it in a member

bshah updated this revision to Diff 80391.Apr 17 2020, 2:08 PM
bshah marked 3 inline comments as done.
  • kill separate watcher class
  • introduce AbstractNotificationsModel
  • introduce WatchedNotificationsModel
bshah added a comment.Apr 17 2020, 2:30 PM

Great! Since phab ate my nice commit message here we go,

I've re-worked previous revision of patch little bit differently,

  • Instead of originally planned Watcher class, now there is WatchedNotificationsModel
  • Most if not all of original NotificationsModel code is now moved to AbstractNotificationsModel (this move is why the patch looks giant, but there's not that much extra code)
  • WatchedNotificationsModel extends AbstractNM, and instead of connecting to notification server instance directly, it talks with notificationmanager dbus interface
  • And when methods on this bus are called, it adds data in model.

There's some bugs and todos, but they are small enough:

  • Unregister watcher when dbus service disappears
  • Fix the notification body being weird xml
  • Maybe add the test.qml I used for testing in repository

I particularly need direction/help for issue 2.

As for testing this, I made sure that all functions of notifications are functional in the notification applet in plasmoidviewer and on watcher side checked if actions are correctly invoked, notifications are added removed properly.

broulik added inline comments.Apr 21 2020, 8:42 AM
libnotificationmanager/CMakeLists.txt
25

Remove

libnotificationmanager/abstractnotificationsmodel.h
41

protected constructor?

libnotificationmanager/notification.h
133

Maybe add a new rawHints() const?

Not a fan of ref return

libnotificationmanager/notification_p.h
102

No need to explicitly initialize

libnotificationmanager/notificationsmodel.h
4

wrong order?

libnotificationmanager/server_p.cpp
237

const QString &service : qAsConst(m_notificationWatchers)

238

Remove, or use categorized logging

239

Can you use generated XML interface for this, maybe?

278

Same as above

536

Yes, please do.

libnotificationmanager/server_p.h
25

Include QStringList

libnotificationmanager/watchednotificationsmodel.cpp
60

I think we should have a valid property or any means for the user to check whether this succeeds / the model is working

bshah updated this revision to Diff 80832.Apr 22 2020, 3:51 AM
bshah marked 15 inline comments as done.

address various comments

  • adhere to interface name when registering watcher
  • remove unused code
  • use qdbusservicewatcher
  • add valid property
bshah added inline comments.Apr 22 2020, 3:52 AM
libnotificationmanager/notification.h
133

I avoided using "raw" word in the function name, because when we process hints we don't actually modify it. We just process rest of notifications data, so it is still same thing.

I will drop ref return however.

libnotificationmanager/server_p.cpp
239

It would introduce weird dependency loop so I'd rather not.

bshah updated this revision to Diff 80833.Apr 22 2020, 3:53 AM

rebase to master

bshah added a comment.Apr 22 2020, 3:54 AM

One last todo is fixing summary so it is not processed twice.

bshah updated this revision to Diff 80847.Apr 22 2020, 8:07 AM
  • pass raw body data to the watcher
bshah retitled this revision from RFC: [libnotificationmanager] introduce the notification watcher to [libnotificationmanager] introduce the WatchedNotificationsModel.Apr 22 2020, 8:12 AM
bshah edited the summary of this revision. (Show Details)
bshah edited the test plan for this revision. (Show Details)
bshah updated this revision to Diff 80990.Apr 23 2020, 12:00 PM
bshah edited the test plan for this revision. (Show Details)
  • watch for notification expiry and closing
broulik accepted this revision.Apr 23 2020, 12:46 PM
This revision is now accepted and ready to land.Apr 23 2020, 12:46 PM
bshah updated this revision to Diff 82489.Mon, May 11, 3:40 AM

rebase on master

This revision was automatically updated to reflect the committed changes.