Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-06-05 Thread Sebastian Kügler 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/
---

(Updated June 6, 2015, 4:59 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Alex Richardson and David Faure.


Changes
---

Submitted with commit 42bed7bb3b9eeacfa447c110f89aa8b4e9564b11 by Sebastian 
Kügler to branch master.


Repository: kcoreaddons


Description
---

Add findPluginsById convenience API

It's a quite common case to load a plugin from an ID. This makes it
easy.

CHANGELOG:New KPluginLoader::findPluginById() convenience API
REVIEW:123669


Diffs
-

  autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
  src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
  src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 

Diff: https://git.reviewboard.kde.org/r/123669/diff/


Testing
---

Added autotests, everything passes.


Thanks,

Sebastian Kügler

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-06-05 Thread Marco Martin 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review81254
---

Ship it!


Ship It!

- Marco Martin


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-06-05 Thread Sebastian Kügler 


> On May 7, 2015, 7:24 a.m., Alex Richardson wrote:
> > Seems like this is duplicated in a few places already so I agree we should 
> > add it. But won't most users of the API want only a single plugin returned?
> > Maybe also add a function `KPluginMetaData 
> > KPluginLoader::findPluginById(const QString& directory, const QString& 
> > pluginId)`?
> > Do we also need the function that returns a vector for a given ID?
> 
> Sebastian Kügler wrote:
> At least our changes in libplasma need a QVector. 
> Otherwise, a list seems easy enough to check if something's found. Returning 
> a single metadata won't be very useful for us at this point (but I see it 
> making sense).
> 
> Sebastian Kügler wrote:
> Ow, also, and perhaps more importantly, multiple ids are technically 
> perfectly valid (only the plugin name and directory are important here). I 
> think that fact should be reflected in the API. Perhaps a word on ordering 
> would be in place in the API docs, plugin locations are cascaded properly in 
> code using it. The most local plugin is at the end of the list, system-widely 
> installed ones at the beginning, so code that uses plugins.first() would not 
> allow the user to override plugins installed for example into /usr/lib with a 
> plugin with the same id and relative path installed into ~/.local). So an 
> extra method returning the .last() plugin found might take away this caveat 
> from the API. We'll still need the method returning a vector for libplasma, 
> though (and I think it's a semantically useful addition).
> 
> About adding another method to return the most-local plugin, I'm on the 
> edge. If others think it's useful and we think the additional API (with its 
> long-term maintainance implications) is worth it, I'm happy to add it as 
> well. (Perhaps in a separate review.)
> 
> Opinions welcome.
> 
> Alex Richardson wrote:
> The problem is that .last() won't work either. 
> QCoreApplication::libraryPaths() has this order 
> (http://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#_ZN16QCoreApplication12libraryPathsEv):
>  $QT_INSTALLDIR/plugins, then the current executable directory and then 
> QT_PLUGIN_PATH. This means that the lowest priority entry from QT_PLUGIN_PATH 
> will be chosen in that case.

So, can I ship it with the QVector<>? Patch has been lingering for a bit, would 
be nice to get it in (we have just written another potential user of this 
method).


- Sebastian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80013
---


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-08 Thread Alex Richardson 


> On May 7, 2015, 8:24 a.m., Alex Richardson wrote:
> > Seems like this is duplicated in a few places already so I agree we should 
> > add it. But won't most users of the API want only a single plugin returned?
> > Maybe also add a function `KPluginMetaData 
> > KPluginLoader::findPluginById(const QString& directory, const QString& 
> > pluginId)`?
> > Do we also need the function that returns a vector for a given ID?
> 
> Sebastian Kügler wrote:
> At least our changes in libplasma need a QVector. 
> Otherwise, a list seems easy enough to check if something's found. Returning 
> a single metadata won't be very useful for us at this point (but I see it 
> making sense).
> 
> Sebastian Kügler wrote:
> Ow, also, and perhaps more importantly, multiple ids are technically 
> perfectly valid (only the plugin name and directory are important here). I 
> think that fact should be reflected in the API. Perhaps a word on ordering 
> would be in place in the API docs, plugin locations are cascaded properly in 
> code using it. The most local plugin is at the end of the list, system-widely 
> installed ones at the beginning, so code that uses plugins.first() would not 
> allow the user to override plugins installed for example into /usr/lib with a 
> plugin with the same id and relative path installed into ~/.local). So an 
> extra method returning the .last() plugin found might take away this caveat 
> from the API. We'll still need the method returning a vector for libplasma, 
> though (and I think it's a semantically useful addition).
> 
> About adding another method to return the most-local plugin, I'm on the 
> edge. If others think it's useful and we think the additional API (with its 
> long-term maintainance implications) is worth it, I'm happy to add it as 
> well. (Perhaps in a separate review.)
> 
> Opinions welcome.

The problem is that .last() won't work either. QCoreApplication::libraryPaths() 
has this order 
(http://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#_ZN16QCoreApplication12libraryPathsEv):
 $QT_INSTALLDIR/plugins, then the current executable directory and then 
QT_PLUGIN_PATH. This means that the lowest priority entry from QT_PLUGIN_PATH 
will be chosen in that case.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80013
---


On May 7, 2015, 12:21 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 7, 2015, 12:21 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-07 Thread Sebastian Kügler 


> On May 7, 2015, 7:24 a.m., Alex Richardson wrote:
> > Seems like this is duplicated in a few places already so I agree we should 
> > add it. But won't most users of the API want only a single plugin returned?
> > Maybe also add a function `KPluginMetaData 
> > KPluginLoader::findPluginById(const QString& directory, const QString& 
> > pluginId)`?
> > Do we also need the function that returns a vector for a given ID?
> 
> Sebastian Kügler wrote:
> At least our changes in libplasma need a QVector. 
> Otherwise, a list seems easy enough to check if something's found. Returning 
> a single metadata won't be very useful for us at this point (but I see it 
> making sense).

Ow, also, and perhaps more importantly, multiple ids are technically perfectly 
valid (only the plugin name and directory are important here). I think that 
fact should be reflected in the API. Perhaps a word on ordering would be in 
place in the API docs, plugin locations are cascaded properly in code using it. 
The most local plugin is at the end of the list, system-widely installed ones 
at the beginning, so code that uses plugins.first() would not allow the user to 
override plugins installed for example into /usr/lib with a plugin with the 
same id and relative path installed into ~/.local). So an extra method 
returning the .last() plugin found might take away this caveat from the API. 
We'll still need the method returning a vector for libplasma, though (and I 
think it's a semantically useful addition).

About adding another method to return the most-local plugin, I'm on the edge. 
If others think it's useful and we think the additional API (with its long-term 
maintainance implications) is worth it, I'm happy to add it as well. (Perhaps 
in a separate review.)

Opinions welcome.


- Sebastian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80013
---


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-07 Thread Sebastian Kügler 


> On May 6, 2015, 11:24 p.m., David Edmundson wrote:
> > autotests/kpluginloadertest.cpp, line 357
> > 
> >
> > not that it really matters, but invalid is spelt wrong.
> > Double the invalidity.

it says "invalid id", without the space. :D


- Sebastian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80001
---


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-07 Thread Sebastian Kügler 


> On May 7, 2015, 7:24 a.m., Alex Richardson wrote:
> > Seems like this is duplicated in a few places already so I agree we should 
> > add it. But won't most users of the API want only a single plugin returned?
> > Maybe also add a function `KPluginMetaData 
> > KPluginLoader::findPluginById(const QString& directory, const QString& 
> > pluginId)`?
> > Do we also need the function that returns a vector for a given ID?

At least our changes in libplasma need a QVector. Otherwise, a 
list seems easy enough to check if something's found. Returning a single 
metadata won't be very useful for us at this point (but I see it making sense).


> On May 7, 2015, 7:24 a.m., Alex Richardson wrote:
> > src/lib/plugin/kpluginloader.cpp, line 278
> > 
> >
> > QVector< KPluginMetaData >  -> QVector

I'll forego submitting a patch with just two spaces removed. Fixed it locally.


- Sebastian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80013
---


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-07 Thread Alex Richardson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80013
---


Seems like this is duplicated in a few places already so I agree we should add 
it. But won't most users of the API want only a single plugin returned?
Maybe also add a function `KPluginMetaData KPluginLoader::findPluginById(const 
QString& directory, const QString& pluginId)`?
Do we also need the function that returns a vector for a given ID?


src/lib/plugin/kpluginloader.cpp (line 278)


QVector< KPluginMetaData >  -> QVector


- Alex Richardson


On May 7, 2015, 12:21 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 7, 2015, 12:21 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-06 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/#review80001
---


+1 seems sensible.


autotests/kpluginloadertest.cpp (line 357)


not that it really matters, but invalid is spelt wrong.
Double the invalidity.


- David Edmundson


On May 6, 2015, 11:21 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123669/
> ---
> 
> (Updated May 6, 2015, 11:21 p.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Add findPluginsById convenience API
> 
> It's a quite common case to load a plugin from an ID. This makes it
> easy.
> 
> CHANGELOG:New KPluginLoader::findPluginById() convenience API
> REVIEW:123669
> 
> 
> Diffs
> -
> 
>   autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
>   src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
>   src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 
> 
> Diff: https://git.reviewboard.kde.org/r/123669/diff/
> 
> 
> Testing
> ---
> 
> Added autotests, everything passes.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 123669: Add KPluginLoader::findPluginsById convenience API

2015-05-06 Thread Sebastian Kügler 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123669/
---

Review request for KDE Frameworks, Alex Richardson and David Faure.


Repository: kcoreaddons


Description
---

Add findPluginsById convenience API

It's a quite common case to load a plugin from an ID. This makes it
easy.

CHANGELOG:New KPluginLoader::findPluginById() convenience API
REVIEW:123669


Diffs
-

  autotests/kpluginloadertest.cpp 3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46 
  src/lib/plugin/kpluginloader.h 124bfab7e207b17d3c8ab4d5a88321af476aad42 
  src/lib/plugin/kpluginloader.cpp 4310d6cd7792329511f12b28d7c68b0fdd013538 

Diff: https://git.reviewboard.kde.org/r/123669/diff/


Testing
---

Added autotests, everything passes.


Thanks,

Sebastian Kügler

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel