D17794: Pkg-config plugin

[Black Warthog]

blackwarthog added inline comments.

INLINE COMMENTS

> arrowd wrote in definesandincludesprovider.cpp:89
> Why not pass by reference? This would eliminate need to `*outIncludes` in the body.

Arguments outIncludes, outDefines and outPackages is optional. In most cases only one of them passed. Because IDefinesAndIncludesManager interface asks defines and includes in separate functions.

> arrowd wrote in kdevpkgconfigmanager.json:17
> Since this plugin has graphical windows, should this really be `NoGUI`?

I don't know. I've watched cmakebuilder, custommake, customdefinesandincludes - all of them have graphical windows, but still contains option "NoGUI".

> arrowd wrote in pkgconfigmanager.cpp:55
> It is a bit strange that you store `DefinesAndIncludesProvider*` in `SettingsManager`. Why not add `static DefinesAndIncludesProvider::getInstance()`, like classic Singleton pattern?

I've copy this style from custom-definesandincludes plugin. Actually i don't know why such method used there. Anyway i see some sense in it:

- It's a two sigletons which cannot work without each other, and looks good to provide only one point  to access them instances. SettingsManager looks more general.
- I prefer to reduce singletons and static methods it makes easier to scale project in future (converting single document app to multidocument and other same things).
- For DefinesAndIncludesProvider we explicitly defined a dependency in constructor (SettingsManager).
- Have no sense how many instances of DefinesAndIncludesProvider are created, only sense how many calls ot registerProvider is done. So we actually can do such call: KDevelop::IDefinesAndIncludesManager::manager()->registerProvider( new DefinesAndIncludesProvider( SettingsManager::globalInstance() ) );

With only one note DefinesAndIncludesProvider provides one function to GUI - list of packages, and one of UI pages should to know about instance of provider.

Still should i to rewrite this?

REPOSITORY
  R32 KDevelop

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

To: blackwarthog, #kdevelop
Cc: apol, rjvbb, arrowd, kdevelop-devel, #kdevelop, glebaccon, hase, antismap, iodelay, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190108/c34425bb/attachment.html>