Make PersonPluginManager API public
ClosedPublic
Actions

Authored by poboiko on Sep 11 2018, 2:18 PM.

Details

Reviewers

dvratil
apol

Group Reviewers

KDE PIM

Commits

R307:73a72d53325f: Make PersonPluginManager API public

Summary

This patch makes PersonPluginManager available to third-party applications, allowing them to use KPeople in a stand-alone way, without interacting with system-wide plugins.

It is part of T9595: [KAddressbook] Use KPeople model for contact list. Currently we only want to use only Akonadi plugin (to be developed), and then possibly add support for other (i.e. KTelepathy) plugins.
For Akonadi plugin, we want to pass an EntityTreeModel to use it as data source, which would be impossible if we use system-wide plugin system.

I've also noted several issues not directly related to this patch, but it would be nice to fix those anyways.

There some ancient compatibility code inside PersonPluginManagerPrivate::loadDataSourcePlugins(), which was added 3 years ago. Apparently, it's not needed anymore.
Inside widgets/CMakeLists.txt, its headers are installed with PREFIX KPeople instead of KPeople/Widgets. Because of that #include <KPeople/Widgets/PersonDetailsView> does not work, because it refers to non-existent header kpeople/persondetailsview.h.
In the same file, there is redundant install(FILES [...]), which does not install any file (line 46)

If it's OK, I can add those changes without posting a differential revision (since they're trivial).

Test Plan

It compiles, it works.

Diff Detail

Repository

R307 KPeople

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

poboiko created this revision.Sep 11 2018, 2:18 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptSep 11 2018, 2:18 PM

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

poboiko requested review of this revision.Sep 11 2018, 2:18 PM

poboiko mentioned this in T9595: [KAddressbook] Use KPeople model for contact list.Sep 11 2018, 2:21 PM

Looks good, just some minor nitpicks.

There some ancient compatibility code inside PersonPluginManagerPrivate::loadDataSourcePlugins(), which was added 3 years ago. Apparently, it's not needed anymore.

Hard to tell :) We probably shouldn't break backwards compatibility?

Inside widgets/CMakeLists.txt, its headers are installed with PREFIX KPeople instead of KPeople/Widgets. Because of that #include <KPeople/Widgets/PersonDetailsView> does not work, because it refers to non-existent header kpeople/persondetailsview.h.

Well spotted. I think it's safe to just fix it. It shouldn't break any code, since no code using this would compile atm.

In the same file, there is redundant install(FILES [...]), which does not install any file (line 46)

I see it on line 48, but indeed looks redundant :)

Technically, all changes to Frameworks should go through review, but if you don't want to upload another Phab diff you can just refer to this review in the commit message.

src/personpluginmanager.cpp
103	Use `QMutexLocker` please
107	This may lead to memory leaks since we are transferring ownership into of the `source` into `PersonPluginManager`. I would suggest, in case of duplication, to `deleteLater` the original source and replace it with the new one.
src/personpluginmanager.h
43	How about `setAutoloadDataSourcePlugins`? They don't necessarily have to be "system plugins" and we are not preventing them from loading, just from autoloading :)
45	I would call this `addDataSource`. Also please document that it takes ownership of the `source`.

This revision now requires changes to proceed.Sep 14 2018, 8:09 AM

Switched to QMutexLocker
Changed behavior of addDataSource to override existing (and delete it)
Added documentation for addDataSource
Switched to KPeople logging category introduced recently

poboiko marked 4 inline comments as done.Sep 14 2018, 9:26 AM

Just some minor details and we are good to go :)

src/personpluginmanager.h
34	Sorry, forgot to mention this in the first round. Since this class is now going to be part of public API, we should add at least a brief documentation and @since 5.51 (the version of Frameworks that this will land in)
43	Could you rename that argument as well, please?
48	Use `@p sourceId` and `@p source` instead of quotes, doxygen will then know it refers to the arguments

This revision now requires changes to proceed.Sep 14 2018, 10:33 AM

Added some documentation

poboiko marked 3 inline comments as done.Sep 14 2018, 11:49 AM

Nice, thank you!

This revision is now accepted and ready to land.Sep 14 2018, 12:26 PM

Closed by commit R307:73a72d53325f: Make PersonPluginManager API public (authored by poboiko). · Explain WhySep 14 2018, 12:38 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			autotests/persondatatests.cpp (2 lines)
M			autotests/personsmodeltest.cpp (2 lines)
M			autotests/personsproxymodeltest.cpp (2 lines)
M			src/CMakeLists.txt (1 line)
M			src/global.cpp (2 lines)
M			src/persondata.cpp (2 lines)
V			src/{personpluginmanager.h ← personpluginmanager_p.h} (26 lines)
M			src/personpluginmanager.cpp (48 lines)
V			src/{personpluginmanager_p.h → personpluginmanager.h}
M			src/personsmodel.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	41413			Sep 11 2018, 2:05 PM	★	★
Diff 2	41616			Sep 14 2018, 9:09 AM	★	★
Diff 3	41640			Sep 14 2018, 11:48 AM	★	★
Diff 4	41645	13d9337	R307:73a72d53325f73fbf00117a78da120baedb42fdf	Sep 14 2018, 12:37 PM	★	★