D26047: Expose KConfig settings to allow registration in KCM Notification
This revision was automatically updated to reflect the committed changes. Closed by commit R120:ee3176ce5641: Expose KConfig settings to allow registration in KCM Notification (authored by crossi). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26047?vs=74023=74079 REVISION DETAIL https://phabricator.kde.org/D26047 AFFECTED FILES libnotificationmanager/CMakeLists.txt libnotificationmanager/kcfg/badgesettings.kcfg libnotificationmanager/kcfg/badgesettings.kcfgc libnotificationmanager/kcfg/donotdisturbsettings.kcfg libnotificationmanager/kcfg/donotdisturbsettings.kcfgc libnotificationmanager/kcfg/jobsettings.kcfg libnotificationmanager/kcfg/jobsettings.kcfgc libnotificationmanager/kcfg/notificationsettings.kcfg libnotificationmanager/kcfg/notificationsettings.kcfgc libnotificationmanager/settings.cpp To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
ervin accepted this revision. ervin added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > broulik wrote in settings.cpp:173 > I still want to be able to specify what `config` (constructor argument) to > use for autotests 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
broulik added a comment.
Other than the seemingly missing `config`, looks good
INLINE COMMENTS
> settings.cpp:173
> -if (!s_settingsInited) {
> -DoNotDisturbSettings::instance(config);
> -NotificationSettings::instance(config);
I still want to be able to specify what `config` (constructor argument) to use
for autotests
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D26047
To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart
Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh,
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
crossi updated this revision to Diff 74023. crossi added a comment. Remove unneeded forward declaration REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26047?vs=74021=74023 REVISION DETAIL https://phabricator.kde.org/D26047 AFFECTED FILES libnotificationmanager/CMakeLists.txt libnotificationmanager/kcfg/badgesettings.kcfg libnotificationmanager/kcfg/badgesettings.kcfgc libnotificationmanager/kcfg/donotdisturbsettings.kcfg libnotificationmanager/kcfg/donotdisturbsettings.kcfgc libnotificationmanager/kcfg/jobsettings.kcfg libnotificationmanager/kcfg/jobsettings.kcfgc libnotificationmanager/kcfg/notificationsettings.kcfg libnotificationmanager/kcfg/notificationsettings.kcfgc libnotificationmanager/settings.cpp To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. I like it, just an unwanted change to clean up still. @broulik what say you? INLINE COMMENTS > settings.h:33 > > +class KCoreConfigSkeleton; > + This change isn't needed anymore. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
crossi updated this revision to Diff 74021. crossi added a comment. 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. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26047?vs=73214=74021 REVISION DETAIL https://phabricator.kde.org/D26047 AFFECTED FILES libnotificationmanager/CMakeLists.txt libnotificationmanager/kcfg/badgesettings.kcfg libnotificationmanager/kcfg/badgesettings.kcfgc libnotificationmanager/kcfg/donotdisturbsettings.kcfg libnotificationmanager/kcfg/donotdisturbsettings.kcfgc libnotificationmanager/kcfg/jobsettings.kcfg libnotificationmanager/kcfg/jobsettings.kcfgc libnotificationmanager/kcfg/notificationsettings.kcfg libnotificationmanager/kcfg/notificationsettings.kcfgc libnotificationmanager/settings.cpp libnotificationmanager/settings.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
crossi updated this revision to Diff 73214. crossi added a comment. Add API documentation REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26047?vs=71676=73214 REVISION DETAIL https://phabricator.kde.org/D26047 AFFECTED FILES libnotificationmanager/CMakeLists.txt libnotificationmanager/kcfg/badgesettings.kcfgc libnotificationmanager/kcfg/donotdisturbsettings.kcfgc libnotificationmanager/kcfg/jobsettings.kcfgc libnotificationmanager/kcfg/notificationsettings.kcfgc libnotificationmanager/settings.cpp libnotificationmanager/settings.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
ervin added inline comments. INLINE COMMENTS > settings.h:343 > > +QList configSkeletons() const; > + Will need API documentation > broulik wrote in settings.h:343 > Not a fan of this becoming public API @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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
crossi added inline comments. INLINE COMMENTS > broulik wrote in settings.h:343 > Not a fan of this becoming public API Maybe not the best approach. Any suggestion to access the KCoreConfigSkeleton encapsulated to register them in the KCM's ConfigModule ? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
broulik added inline comments. INLINE COMMENTS > settings.h:343 > > +QList configSkeletons() const; > + Not a fan of this becoming public API REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: broulik, meven, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
meven resigned from this revision. meven added a comment. This revision now requires review to proceed. Someone else should valide this. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26047: Expose KConfig settings to allow registration in KCM Notification
crossi created this revision. crossi added reviewers: Plasma, Frameworks, ervin, bport, davidedmundson, mart. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. crossi requested review of this revision. REVISION SUMMARY For KCM Notification, allow to register the generated settings to the ManagedConfigModule machinery TEST PLAN refactor, no change REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26047 AFFECTED FILES libnotificationmanager/CMakeLists.txt libnotificationmanager/kcfg/badgesettings.kcfgc libnotificationmanager/kcfg/donotdisturbsettings.kcfgc libnotificationmanager/kcfg/jobsettings.kcfgc libnotificationmanager/kcfg/notificationsettings.kcfgc libnotificationmanager/settings.cpp libnotificationmanager/settings.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
