D15431: Make PersonPluginManager API public

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> personpluginmanager.cpp:103
> +{
> +    s_instance->m_mutex.lock();
> +    if (!s_instance->dataSourcePlugins.contains(sourceId)) {

Use `QMutexLocker` please

> personpluginmanager.cpp:107
> +    } else {
> +        qWarning() << "Attempting to load data source that is already loaded!";
> +    }

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.

> personpluginmanager.h:43
> +     */
> +    static void setLoadSystemDataSourcePlugins(bool loadSystemDataSourcePlugins);
>      static QList<BasePersonsDataSource *> dataSourcePlugins();

How about `setAutoloadDataSourcePlugins`? They don't necessarily have to be "system plugins" and we are not preventing them from loading, just from autoloading :)

> personpluginmanager.h:45
>      static QList<BasePersonsDataSource *> dataSourcePlugins();
> +    static void setDataSource(const QString &sourceId, BasePersonsDataSource *source);
>      static BasePersonsDataSource *dataSource(const QString &sourceId);

I would call this `addDataSource`. Also please document that it takes ownership of the `source`.

REPOSITORY
  R307 KPeople

REVISION DETAIL
  https://phabricator.kde.org/D15431

To: poboiko, #kde_pim, dvratil, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180914/2c735c76/attachment-0001.html>