Add new notifications KCM
ClosedPublic

Authored by broulik on Apr 15 2019, 3:12 PM.

Details

Summary

This completely rewrites the notifications KCM using libnotificationmanager.
All notification-related settings are moved from the widget to a KCM. This includes popup placement settings, history, job management, as well as Task Manager progress and badges.

The main page contains global notification settings. The timeout heuristic based on word count is dropped and instead a fixed configurable timeout value (1-120s, default 5s) is used.

The applications page not only lists KDE applications but can also find GNOME applications having the appropriate X-GNOME-UsesNotifications key set. Moreover, applications that have been observed sending notifications but didn't "register" with the system are also tracked and can be configured. These have a trash icon so they can be removed in case of a false positive. The search field not only finds applications but also events, e.g. searching for "highlight" will find an event titled "user highlighted" in a chat app. I originally planned to rewrite the KNotifyConfigWidget but postponed that for time resource reasons, there's now just a button that opens the old widget.

BUG: 401616
BUG: 398543
BUG: 393123
FIXED-IN: 5.16.0

Test Plan

Depends on D20265 D20266

Task manager changes to respect these settings pending.

Main page:


Custom position selector:

Applications page:

Configuring KDE application:

Events detail config (old widget):

Configuring non-KDE application:

Filter feature finding events:

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Apr 15 2019, 3:12 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 15 2019, 3:12 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 15 2019, 3:12 PM
broulik edited the summary of this revision. (Show Details)Apr 15 2019, 3:18 PM
broulik edited the test plan for this revision. (Show Details)
zzag added a subscriber: zzag.Apr 15 2019, 3:30 PM
zzag added inline comments.
kcms/notifications/sourcesmodel.cpp
244

A bit stupid question: how it gets deleted?

broulik added inline comments.Apr 15 2019, 3:38 PM
kcms/notifications/sourcesmodel.cpp
244

Not a stupid question, most likely a leftover

The trash icons in the app list make me think that it's going to uninstall those apps lol

The trash icons in the app list make me think that it's going to uninstall those apps lol

Suggestions welcome.

The trash icons in the app list make me think that it's going to uninstall those apps lol

Suggestions welcome.

list-remove, maybe?

List-remove looks awful in this context

What about edit-clear-symbolic or window-close (I think this would be the same as for the dismiss action in the widget?)

GB_2 added a subscriber: GB_2.Apr 17 2019, 1:20 PM

Hmm, it seems like the Plasma theme list-remove is different from the normal Breeze icon theme list-remove. I think we should have the icon from the Plasma theme in the Breeze icon theme too, but with a different name, so we don't overwrite the existing one.

ngraham added a subscriber: ndavis.Apr 17 2019, 1:45 PM
In D20576#451796, @GB_2 wrote:

Hmm, it seems like the Plasma theme list-remove is different from the normal Breeze icon theme list-remove. I think we should have the icon from the Plasma theme in the Breeze icon theme too, but with a different name, so we don't overwrite the existing one.

Yeah, I was thinking of the Breeze icon theme one, which IMO looks better:

I agree that they should match each other. I think @ndavis is working on that in D20623, as a matter of fact!

GB_2 added a comment.Apr 17 2019, 1:46 PM

Yeah, I was thinking of the Breeze icon theme one, which IMO looks better:

I agree that they should match each other. I think @ndavis is working on that in D20623, as a matter of fact!

This is the one from the Plasma theme which will be gone with that patch.

GB_2 added a comment.EditedApr 20 2019, 6:40 PM

Here you go: D20700

GB_2 added a comment.EditedTue, Apr 23, 8:01 PM

You can now use edit-delete-remove, which got added in D20700.

mart added a subscriber: mart.Wed, Apr 24, 1:54 PM
mart added inline comments.
kcms/notifications/sourcesmodel.cpp
244

delete?

broulik updated this revision to Diff 57047.Fri, Apr 26, 2:31 PM
  • Drop remove feature for seen apps. They shouldn't really pile up, as discussed with VDG
  • Some cleanup
davidedmundson added inline comments.
kcms/notifications/package/contents/ui/ApplicationConfiguration.qml
104

you shouldn't specify a width inside a layout.

kcms/notifications/package/contents/ui/SourcesPage.qml
36

The kcm.filteredModel?

61

Can you talk me through all this:

We have a listview with a filtered model with a selected item
We change the filter

As long as the proxies are working correctly and track a movement as a movement, the currentIndex of the listview should adjust.

What does happen?

mart added inline comments.Mon, May 6, 9:21 AM
kcms/notifications/package/contents/ui/ApplicationConfiguration.qml
104

implicitWidth or Layout.preferredWidth

broulik added inline comments.Mon, May 6, 9:31 AM
kcms/notifications/package/contents/ui/SourcesPage.qml
61

When I enter a query for which there is no result, the view gets empty and ListView gets all confused and loses the index or sets it to some random item

broulik updated this revision to Diff 57652.Mon, May 6, 3:35 PM
  • Load source model only on sources page but do so immediately there when needed
  • Don't set width inside layout
davidedmundson accepted this revision.Wed, May 8, 11:16 AM

I don't like this thing with source proxy. But I can kinda see what it's doing and it looks like it'll work.

Lets go for it

This revision is now accepted and ready to land.Wed, May 8, 11:16 AM
broulik updated this revision to Diff 57758.Wed, May 8, 1:23 PM
  • Don't leak KNotification
  • Ensure proper transient
ngraham accepted this revision as: VDG.Wed, May 8, 2:12 PM
This revision was automatically updated to reflect the committed changes.