Introduce libnotificationmanager
ClosedPublic

Authored by broulik on Apr 5 2019, 1:33 PM.

Details

Reviewers
sitter
Group Reviewers
Plasma
Summary

Introduce libnotification

A library revolving around notifications.

This serves as a replacement for the notification dataengine and logic inside the notification plasmoid. Its architecture is inspired by libtaskmanager.

There's three main classes:

  • Notifications: The main model providing notifications and jobs combined with fine-grained filter, grouping, and sorting capabilities. This is what you want to use for showing notifications. The model itself is very generic and doesn't make any assumptions on representation (e.g. popup or history) and it is up to the user to set the appropriate filters and other properties based on the user's preferences and desired presentation.
  • Settings: This encapsulates all global settings related to notifications. It will be used by the KCM to write into as well e.g. task manager and notification plasmoid to read settings from. It basically presents a "current state" in which the notification system is in, that includes do not disturb settings/mode.
  • Server: This class handles the DBus stuff and acts as a dumb proxy between DBus and the consumer, e.g. the NotificationModel or the old dataengine.
Test Plan

Depends on D20196 (for live signal of settings change, shouldnt really matter for testing)

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Apr 5 2019, 1:33 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 5 2019, 1:33 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 5 2019, 1:33 PM
broulik added a dependent revision: D20266: Add new notification plasmoid.
ngraham added a subscriber: ngraham.Apr 5 2019, 4:29 PM
ngraham added inline comments.
libnotificationmanager/dbus/org.freedesktop.Notifications.xml
57

Why is this commented out? Should it just be deleted instead? Or is it supposed to be used?

libnotificationmanager/job.cpp
176

What's this all about?

libnotificationmanager/notification.cpp
351

This would seem like an example of a notification that should not time out. I think what you've got here is a sane default.

libnotificationmanager/server_p.h
68

What's this about?

libnotificationmanager/settings.h
214

I like CloseToWidget

It's a big review :/

The general infrastructure and design makes sense.
I shall review the individual classes separately during the week.

libnotificationmanager/CMakeLists.txt
4

That's one way to avoid test failures.

27

I don't know the status of this, but please make sure we don't depend on something in review

libnotificationmanager/notificationgroupcollapsingproxymodel.cpp
127

It seems safer to take the pesistent index from the sourceModel

We're invalidating this layer repeatedly which could invalidate our persistent index

134

you shouldn't need this, the data is changing so it'll get re-evaluated

nicolasfella added inline comments.
libnotificationmanager/CMakeLists.txt
27

It's in Extragear

Then we can't use it.

broulik marked an inline comment as done.Apr 11 2019, 10:34 AM
broulik added inline comments.
libnotificationmanager/CMakeLists.txt
27

Changed code to use plasma-pa QML only now like in task manager

libnotificationmanager/dbus/org.freedesktop.Notifications.xml
57

Was supposed to be a way for apps to query who's holding an inhibition but since it's inside plasmashell anyway I can just use that directly from the plasmoid without "polluting" the public API. Apps, if even needed, can query Inhibited property to see whether it's enabled or not

libnotificationmanager/notificationgroupcollapsingproxymodel.cpp
134

filterAcceptsRow doesn't actually read IsGroupExpanded since filterAcceptsRow evaluates the source row, not our own

broulik updated this revision to Diff 56056.Apr 12 2019, 11:45 AM
broulik edited the summary of this revision. (Show Details)
  • Address review comments
  • Rewrite job handling to be kuiserver and kill it

I removed the dataengine rewrite from the diff to make it a bit less overwhelming, will come up as separate patch
kuiserver removal is also not in the diff as that's just deleting the folder and removing it from CMakeLists

mart added a subscriber: mart.Apr 24 2019, 10:50 AM
mart added inline comments.
libnotificationmanager/jobsmodel.cpp
72

any reason to go this way instead of Q_GLOBAL_STATIC ?
I guess you want to have cases in which none of it is ever created? tough the case would be when there are no notification plasmoids, but in that case the plugin containing the library wouldn't be loaded?

broulik added inline comments.Apr 28 2019, 7:14 PM
libnotificationmanager/jobsmodel.cpp
72

This library is not just for plasmoids, it is also used in the jobs dataengine for instance.
But yeah I want some refcounting so the model stuff is only done when there's actually someone using it.

davidedmundson added inline comments.May 2 2019, 1:01 PM
libnotificationmanager/jobsmodel.cpp
215
libnotificationmanager/jobsmodel_p.cpp
358

you're not filtering the jobs based on the desktopEntry

every app gets notified about anyones percentage?

hein added a subscriber: hein.May 5 2019, 8:52 AM
hein added inline comments.
libnotificationmanager/notifications.h
217

This is a bit unusual, I assume it's because you're flattening back out so you don't want rowCount/canFetchMore?

hein added inline comments.May 5 2019, 9:18 AM
libnotificationmanager/notifications.cpp
828

While this isn't an objection, I usually use QMetaEnum to compute this from the enum instead of having a block of stuff that needs to be synced.

libnotificationmanager/notifications.h
211

I recommend making this Q_ENUM and registering it; with libtm it turned out that it's useful to be able to call data() with a role from QML sometimes.

hein added inline comments.May 5 2019, 9:49 AM
libnotificationmanager/notificationsmodel.cpp
57

Could be worth making inline maybe?

hein added inline comments.May 5 2019, 9:51 AM
libnotificationmanager/notificationgroupingproxymodel.cpp
38

Careful there - these will match when both are empty, which is why the libtm version tests for that. Are you sure this data cannot ever be missing?

hein added inline comments.May 5 2019, 9:54 AM
libnotificationmanager/notificationgroupcollapsingproxymodel.cpp
52

Not actually unused.

bottomRight: You sure the source model will emit for single cells? Otherwise you need to loop over the range.

hein added inline comments.May 5 2019, 9:55 AM
libnotificationmanager/notificationgroupcollapsingproxymodel.cpp
175

I know this really sucks, and I'm not going to be pushy on this because you've been waiting on this review forever.

But: Qt 5.13 has deprecated QModelIndex::child (they want us to use QAIM::index now always), leading to noisy builds. For new code it'd be amazing to avoid it.

broulik added inline comments.May 5 2019, 10:29 AM
libnotificationmanager/jobsmodel_p.cpp
358

Good catch, forgot a check for desktopEntry there

libnotificationmanager/notificationgroupingproxymodel.cpp
38

I'm pretty sure it can never be empty as if all else fails I use the binary name of the process that sent the notification dbus message (which might fail after all?), I'll add a check nonetheless.

libnotificationmanager/notifications.cpp
828

There's a gazillion places that need to be synced when we add something:

  • add enum in header
  • add new field to struct that holds the actual data
  • let data() return the new role

as well as actually forwarding it in a bunch of places in QML, so I don't think automating this particular part really reduces the workload for that. I can give it a shot, though.

libnotificationmanager/notifications.h
211

I did exactly that in a later commit that isn't on Phab, sorry :)

217

I guess. I use it for showing an expand button with "show n more" written on it

hein added inline comments.May 6 2019, 11:45 AM
libnotificationmanager/job.cpp
30

Unused?

libnotificationmanager/notificationgroupcollapsingproxymodel.cpp
194

Check m_limit first for a tiny speedup.

libnotificationmanager/notifications.cpp
76

Have you considered using KModelIndexProxyMapper from KF5::ItemModels?

broulik marked an inline comment as done.May 6 2019, 11:51 AM
broulik marked 8 inline comments as done.May 6 2019, 12:52 PM
broulik added inline comments.
libnotificationmanager/notifications.cpp
76

I have, it doesn't work for this usecase. It can only map a tree via a single model, we have it the other way round, we have a single proxy chain diverging into two. Also, KConcatenateRowsProxyModel isn't a proper proxy model, so it completely breaks KModelIndexProxyMapper

I have a bunch of minor comments, but nothing that's a big show-stopper.
Then I think we're probably best shipping it soon.

libnotificationmanager/declarative/notificationmanagerplugin.cpp
37

uncreatabletype?

libnotificationmanager/job_p.cpp
291

descriptionUrlChanged

libnotificationmanager/jobsmodel.cpp
163

Why?

job::setDismissed has a signal that should propagate through

202

emit is wrong

libnotificationmanager/jobsmodel_p.cpp
81

If it's pending then it won't be in the model, which means we won't call removeAt

which means we won't actually delete it.

Possible fix

void JobsModelPrivate::remove(Job *job)
{
    const int row = m_jobViews.indexOf(job);
    if (row > -1) {
        removeAt(row);
    } else {
     delete job;
}
}
139

can you move this to the ctor for the same reason as the other code

423

you can use the enum, AFAIK it was hardcoded so we could backport this patch into 5.8 where anything non-zero was enough for what we wanted.

libnotificationmanager/limitedrowcountproxymodel.cpp
26

This is a good candidate for KItemModels

45

I assume we need this as a row being inserted earlier in the list doesn't cause us to re-evaluate existing rows that previously passed the filter and are unchanged?

We should have this for rowsMoved

libnotificationmanager/notification.h
53

yes

libnotificationmanager/server_p.cpp
71

there's a very obscure crash here.

If we fail to register the service, we don't create m_inhibitionWatcher and usages later will crash.

It's possible we'd still get into this code as one could call this the notify method on our existing service name.

broulik marked 23 inline comments as done.May 9 2019, 8:52 AM
sitter accepted this revision.May 9 2019, 1:56 PM
This revision is now accepted and ready to land.May 9 2019, 1:56 PM
sitter closed this revision.May 9 2019, 1:56 PM