| wojnilowicz |
Besides the move which really allows to have singleton throughout all
shared objects and libraries of the application, I moved all additional
settings function into the KMyMoneySettings object. This allows to
remove the KMyMoneyGlobalSettings class completely.
Compiling this in an existing build directory may cause trouble as some
old generated files are left behind and might be used instead of new
ones. You have been warned.
| No Linters Available |
| No Unit Test Coverage |
| kmymoney/CMakeLists.txt | ||
|---|---|---|
| 38 | settings library is required by money subdir - should it not be listed before ? | |
| kmymoney/settings/CMakeLists.txt | ||
| 14 | Are the following libraries only for solving references inside kmm_settings or do you want to have it published by kmm_settings ? In the first case 'PRIVATE' should be used | |
| kmymoney/settings/kmymoneysettings.kcfgc | ||
| 5 | Is it known that using dpointer creates an additional indirection for acessing members which may affect performance ? without dpointer self()->autoFillUseMemos; with dpointer self()->d->autoFillUseMemos; | |
| kmymoney/settings/CMakeLists.txt | ||
|---|---|---|
| 14 | The second case applies if any api of the following libraries is used in the public API of kmm_settings and other libraries using kmm_settings need to link to them too. See https://cmake.org/pipermail/cmake/2016-May/063400.html for more details. | |
| kmymoney/CMakeLists.txt | ||
|---|---|---|
| 38 | Not anymore, see my changes to kmymoney/mymoney/mymoneyreport.cpp as part of this diff | |
| kmymoney/settings/kmymoneysettings.kcfgc | ||
| 5 | In theory yes, but it does not count here as the number of calls is just way too low. Since we changed much of the code to use the pimpl paradigm I just followed that route. | |
The boring part of patch looks to be solid. Please read my comments, review the code yourself and in case you know it all, commit.
| kmymoney/settings/CMakeLists.txt | ||
|---|---|---|
| 15 | Do we still need this? | |
| 17 | Do we still need this? | |
| kmymoney/settings/kmymoneysettings.kcfgc | ||
| 5 | I don't want to be nitpicky, but do we have any advantage from using dpointer here? | |
| kmymoney/settings/kmymoneysettings_addon.h | ||
| 29 | I believe, we don't need this here anymore. | |
| kmymoney/settings/kmymoneysettings_addons.cpp | ||
| 16 | Why no kmymoneysettings_addons.h here? As I understand you declare functions in .h and define them in .cpp. I see no such thing here. Is it an other scheme? | |
| kmymoney/settings/kmymoneysettings_addons.h | ||
| 16 | Why have you removed guards from here? I believe it will cause us multiple defines somewhere in time and with it headaches. | |
| 54 | This is no longer valid. | |
| kmymoney/settings/kmymoneysettings_addon.h | ||
|---|---|---|
| 29 | No, it is needed. | |
| kmymoney/settings/kmymoneysettings_addons.cpp | ||
| 16 | See KConfig API and the compiler's source to understand what is going on here. You might also want to take a look at the generated code. | |
| kmymoney/settings/kmymoneysettings_addons.h | ||
| 16 | Because this file is included into the class decleration of a generated include file (kmymoneysettings.h) and this is protected. Include guards will not work here. | |
| kmymoney/settings/kmymoneysettings_addons.cpp | ||
|---|---|---|
| 16 | Ok, I see it now. Thanks for the explanation. | |
An updated version is committed to master
commit e2448f62ed3da50010ae7a987a33873e0d44ca05
Author: Thomas Baumgart <thb@net-bembel.de>
Date: Sun Jan 28 16:24:12 2018 +0100