D17794: Pkg-config plugin
noreply at phabricator.kde.org
Tue Jan 8 06:15:36 GMT 2019
blackwarthog added 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?
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...
More information about the KDevelop-devel