KCM Notifications : Manage app-specific notifications with KCconfigXT's magic
ClosedPublic

Authored by crossi on Feb 6 2020, 10:41 AM.

Details

Summary

App-specific notifications' behavior are now managed with KConfigXT. The default values are set in plasmanotifyrc shipped with the libnotificationmanager and can be override.
Immutability can be set at applicatioin level in plasmanotifyrc.
Reinitialize button reset all modifications not applied. Default button reset to default applications' behavior.

Require D27059 D27133 D27155

Test Plan

To set immutable an application or a service specific notifications, add [$i] after an element in plasmanotifyrc

  • Set immutability for all services (same for applications)
[Services][$i]
  • Set immutability for an app (same for a service)
[Applications][org.kde.kdevelop][$i]
  • Set immutability for a specific entry
[Applications][org.kde.kdevelop]
ShowBadges=false
ShowPopupsInDndMode[$i]=true

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.
crossi created this revision.Feb 6 2020, 10:41 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 6 2020, 10:41 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
crossi requested review of this revision.Feb 6 2020, 10:41 AM
bport added inline comments.Feb 10 2020, 8:11 AM
kcms/notifications/kcm.cpp
253

Add a if to emit signal only if needed

crossi updated this revision to Diff 75360.Feb 10 2020, 1:35 PM

Check before assign and emit changed

crossi marked an inline comment as done.Feb 10 2020, 1:35 PM
bport accepted this revision.Feb 11 2020, 9:01 AM
This revision is now accepted and ready to land.Feb 11 2020, 9:01 AM
ervin requested changes to this revision.Feb 12 2020, 3:48 PM
ervin added inline comments.
kcms/notifications/kcm.cpp
273

What about m_behaviorSettingsList is empty? I'm not sure we can guarantee that it'll always contain something.

332

Replace with any_of ? This would break the loop earlier.

kcms/notifications/kcm.h
90

Should be a const ref

105

Space before * not after

kcms/notifications/sourcesmodel.cpp
380 ↗(On Diff #75360)

const auto &

kcms/notifications/sourcesmodel.h
98

It's generally a better idea to define a small trivial struct when roll with QPair

This revision now requires changes to proceed.Feb 12 2020, 3:48 PM
crossi updated this revision to Diff 76004.Feb 19 2020, 3:07 PM
crossi marked 6 inline comments as done.

Consider Kevin's comments

ervin accepted this revision.Feb 24 2020, 12:40 PM
This revision is now accepted and ready to land.Feb 24 2020, 12:40 PM
crossi updated this revision to Diff 80864.Apr 22 2020, 11:22 AM

maange reset to default application notification

crossi edited the summary of this revision. (Show Details)Apr 22 2020, 11:22 AM
ervin requested changes to this revision.Apr 22 2020, 12:11 PM
ervin added inline comments.
kcms/notifications/kcm.cpp
322–324

qAsConst(m_behaviorSettingsList), otherwise it will unfortunately detach.

341

I'd align that (and the next line) with the previous parameter.

342

This starts being a long line, please break it after the opening brace and again after the first semicolon.

This revision now requires changes to proceed.Apr 22 2020, 12:11 PM
crossi updated this revision to Diff 80894.Apr 22 2020, 2:34 PM

use qAsConst, indent

crossi marked 3 inline comments as done.Apr 22 2020, 2:34 PM
ervin accepted this revision.Apr 22 2020, 8:38 PM
This revision is now accepted and ready to land.Apr 22 2020, 8:38 PM
broulik added inline comments.Apr 23 2020, 7:59 AM
kcms/notifications/kcm.cpp
266

This method is only used in this place, so I don't see why we would want a temporary list with a struct and all rather than just iterating here directly.

267

Please auto * with an asterisk

kcms/notifications/package/contents/ui/ApplicationConfiguration.qml
45–46

I've seen duplicate apps in this list, e.g. snap vs properly installed, so I don't think you can rely on this.

47

While we generally do RDN for desktop entries these days I belive there can be cases where there's no telling if it's a desktop entry or notfiyrc? So we would need to pass that in

kcms/notifications/sourcesmodel.h
57

Why not an enum?

crossi planned changes to this revision.Apr 23 2020, 12:59 PM
crossi added inline comments.
kcms/notifications/package/contents/ui/ApplicationConfiguration.qml
45–46

I guess I could use rootIndex instead, and it will solve the issue you mention below as well.

crossi updated this revision to Diff 81009.Apr 23 2020, 3:16 PM

Consider Kai's comments

This revision is now accepted and ready to land.Apr 23 2020, 3:16 PM
broulik accepted this revision.Apr 23 2020, 3:19 PM

Clean up those minor comments and then ship it, thanks!

kcms/notifications/kcm.cpp
256

.value(index.row())

269

Why define outside?

270

QModelIndex() should be the default argument for rowCount()

kcms/notifications/package/contents/ui/ApplicationConfiguration.qml
46

Can we swap that around to be "isOtherApp"

crossi edited the test plan for this revision. (Show Details)Apr 23 2020, 4:14 PM
crossi updated this revision to Diff 81014.Apr 23 2020, 4:15 PM

clear the nitpicks

crossi edited the test plan for this revision. (Show Details)Apr 23 2020, 4:31 PM
This revision was automatically updated to reflect the committed changes.