Add KCModuleData as base class for plugin
ClosedPublic
Actions

Authored by bport on Mar 31 2020, 9:37 AM.

Details

Reviewers

Group Reviewers

Summary

This class will allow to get data of a module without loading the UI.
For now it's used to get if a module have default value applied, but we can imagine more interaction in the future

Diff Detail

Repository

R295 KCMUtils

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 25026
Build 25044: arc lint + arc unit

bport created this revision.Mar 31 2020, 9:37 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 31 2020, 9:37 AM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

bport requested review of this revision.Mar 31 2020, 9:37 AM

Harbormaster completed remote builds in B24518: Diff 78969.Mar 31 2020, 9:37 AM

bport added a dependent revision: D28461: In sidebar mode show if a module is in default state or not.Mar 31 2020, 9:41 AM

broulik added a subscriber: broulik.Mar 31 2020, 9:48 AM

broulik added inline comments.

src/kcmoduleloader.cpp
165	Use an early return here
176	This looks like it leaks
182	Error can be `nullptr`. Either print a warning or remove it

broulik added inline comments.Mar 31 2020, 10:35 AM

src/kcmodulestateprobe.h
21 ↗	(On Diff #78969)	`KCMODULESTATEPROBE_H`
25 ↗	(On Diff #78969)	Forward-declare
39 ↗	(On Diff #78969)	Please add a `virtual_hook` so we can extend this class in the future without breaking ABI should we have the need to extract more data out of a settings module: virtual void virtual_hook(int id, void *data)

ngraham mentioned this in D28461: In sidebar mode show if a module is in default state or not.Mar 31 2020, 6:32 PM

Take in consideration feedbacks

Harbormaster completed remote builds in B24852: Diff 79496.Apr 6 2020, 3:42 PM

ervin requested changes to this revision.Apr 7 2020, 4:37 PM

ervin added inline comments.

src/kcmoduleloader.cpp
161	Curly brace should be on the previous line
167	Wouldn't it be better to initialize args2 with arg iterators? i.e. `QVariantList args2(arg.cbegin(), arg.cend());`
src/kcmodulestateprobe.cpp
47 ↗	(On Diff #79496)	Curly brace on the next line
55 ↗	(On Diff #79496)	Curly brace on the next line
src/kcmodulestateprobe.h
44 ↗	(On Diff #79496)	Should be protected not public
39 ↗	(On Diff #78969)	I'd slightly disagree here though, if that inherits from QObject anyway I'd just rely on meta call dispatching. But OK, let's go virtual_hook.

This revision now requires changes to proceed.Apr 7 2020, 4:37 PM

ervin feedback

Harbormaster completed remote builds in B25020: Diff 79772.Apr 10 2020, 3:32 PM

bport marked 10 inline comments as done.Apr 10 2020, 3:32 PM

Rename state probe to data (because can be useful for more than only state probe)

Harbormaster completed remote builds in B25026: Diff 79780.Apr 10 2020, 5:25 PM

bport retitled this revision from Add KCModuleStateProbe as base class for plugin to Add KCModuleDada as base class for plugin.Apr 15 2020, 9:20 AM

bport edited the summary of this revision. (Show Details)

Please fix the typo in the commit title before pushing, otherwise looks fine to me.

This revision is now accepted and ready to land.Apr 21 2020, 6:40 PM

bport retitled this revision from Add KCModuleDada as base class for plugin to Add KCModuleData as base class for plugin.Apr 21 2020, 8:08 PM

meven added a subscriber: meven.May 11 2020, 10:15 AM

meven added inline comments.

src/kcmoduledata.h
35	@since 5.71
src/kcmoduleloader.h
113	@since 5.71

ahmadsamir added a subscriber: ahmadsamir.Aug 14 2020, 1:30 PM

ahmadsamir added inline comments.

src/kcmoduleloader.cpp
164	Hello. This breaks the build for Qt 5.12 on the CI, (the min. required version in KF is still 5.12): https://build.kde.org/job/Frameworks/job/kcmutils/job/kf5-qt5%20SUSEQt5.12/155/console

Has been moved to https://invent.kde.org/frameworks/kcmutils/-/merge_requests/10

Revision Contents
Changeset List

		Path
M		src/CMakeLists.txt (2 lines)
A	M	src/kcmoduledata.h (67 lines)
A	M	src/kcmoduledata.cpp (82 lines)
M		src/kcmoduleloader.h (9 lines)
M		src/kcmoduleloader.cpp (27 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	78969	fc639ac		Mar 31 2020, 9:37 AM	★	★
Diff 2	79496	9e7d06b	Take in consideration feedbacks	Apr 6 2020, 3:42 PM	★	★
Diff 3	79772	d16f10e	ervin feedback	Apr 10 2020, 3:32 PM	★	★
Diff 4	79780	d16f10e	Rename state probe to data (because can be useful for more than only state…	Apr 10 2020, 5:25 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
a34d634681ff	cfa223b986a6	d16f10e2ee1e	Benjamin Port	Add KCModuleStateProbe as base class for plugin (Show More…)	Mar 31 2020, 9:34 AM

Status	Author	Revision
Needs Revision	bport	D28462 [KCM][WIP] Add KCModuleData
Needs Review	bport	D28461 In sidebar mode show if a module is in default state or not
Closed	bport	D28460 Add KCModuleData as base class for plugin