Expose KConfig settings to allow registration in KCM Notification
ClosedPublic

Authored by crossi on Dec 16 2019, 3:53 PM.

Details

Summary

For KCM Notification, allow to register the generated settings to the ManagedConfigModule machinery

Test Plan

refactor, no change

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 21439
Build 21457: arc lint + arc unit
crossi created this revision.Dec 16 2019, 3:53 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 16 2019, 3:53 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
crossi requested review of this revision.Dec 16 2019, 3:53 PM
meven accepted this revision.Dec 16 2019, 4:21 PM
This revision is now accepted and ready to land.Dec 16 2019, 4:21 PM
meven resigned from this revision.Dec 16 2019, 4:56 PM

Someone else should valide this.

This revision now requires review to proceed.Dec 16 2019, 4:56 PM
meven added a subscriber: meven.Dec 16 2019, 4:56 PM
broulik added inline comments.
libnotificationmanager/settings.h
343

Not a fan of this becoming public API

crossi added inline comments.Dec 18 2019, 2:02 PM
libnotificationmanager/settings.h
343

Maybe not the best approach.

Any suggestion to access the KCoreConfigSkeleton encapsulated to register them in the KCM's ConfigModule ?

ervin added inline comments.Dec 23 2019, 2:56 PM
libnotificationmanager/settings.h
343

Will need API documentation

343

@broulik I understand you don't like much this becoming exposed as one could abuse it to kill encapsulation and state... but we can't have it both ways either. This facade makes it impossible to plug as is in existing systems around KCM or ConfigModule without large efforts. I don't think we can have it both ways here.

crossi updated this revision to Diff 73214.Jan 10 2020, 5:07 PM

Add API documentation

crossi planned changes to this revision.Jan 20 2020, 1:17 PM
crossi updated this revision to Diff 74021.Jan 21 2020, 3:50 PM

Following discussion with @ervin and @broulik, export generated KConfig settings, remove singleton option. The KCM will have its own KConfig settings' instance like other KCMs.

ervin requested changes to this revision.Jan 21 2020, 3:57 PM

I like it, just an unwanted change to clean up still. @broulik what say you?

libnotificationmanager/settings.h
33

This change isn't needed anymore.

This revision now requires changes to proceed.Jan 21 2020, 3:57 PM
crossi updated this revision to Diff 74023.Jan 21 2020, 4:01 PM

Remove unneeded forward declaration

Other than the seemingly missing config, looks good

libnotificationmanager/settings.cpp
173

I still want to be able to specify what config (constructor argument) to use for autotests

ervin accepted this revision.Jan 22 2020, 9:08 AM
ervin added inline comments.
libnotificationmanager/settings.cpp
173

After discussing with kai, let's mark the ctor taking the config parameter as deprecated instead. Should be done in another patch, this one can go as is.

This revision is now accepted and ready to land.Jan 22 2020, 9:08 AM
This revision was automatically updated to reflect the committed changes.