| tbaumgart |
Add QT_VERSION checks so that the code can still be built with Qt versions
older than 5.10 (that's where QSortFilterProxyModel gained recursive
filtering capabilities).
Also add some defines to ease porting in the future; add a dud
setRecursiveFilteringEnabled() method to keep backward compatibility.
For details see the code the review.
make && ctest; all unit tests pass except qsqlcipher-test, but it fails
on master too.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Basically, this is the correct thing to do but not all (including my development) distros have Qt 5.10 as a base but rely older Qt versions (5.9.7 in my case). Therefore, we should simply postpone this patch or amend it to support both worlds. I know, this would involve a bit of #ifdef logic, but we have that in other places as well.
If I see this right, you can make all changes in the .cpp files so that they don't have to be changed in the future and only the #if / #endif in the .h files have to cleaned up.
| kmymoney/models/accountsproxymodel.cpp | ||
|---|---|---|
| 44 | See above. | |
| 48 | I would add the following to the header file: #if QT_VERSION < QT_VERSION_CHECK(5,10,0)
// provide the interface for backward compatbility
void setRecursiveFilteringEnabled(bool) {}
#endifand remove the enclosing #if/#endif in the code so it is prepared for the future. | |
| 57 | same here | |
| kmymoney/models/accountsproxymodel.h | ||
| 31 | I would add the following here #define QSortFilterProxyModel KRecursiveFilterProxyModel and use QSortFilterProxyModel throughout this file. This would make the code easier to read and adjustable in the future. | |
| 72 | This whole change is not necessary with the above mentioned #define | |
| kmymoney/models/equitiesmodel.h | ||
| 32 | The same applies here. I don't repeat all the above comments. | |
Almost. It works on Qt < 5.10 only with the patch below on top of your change. Do you have write access to the KDE repo?
diff --git a/kmymoney/models/accountsproxymodel.h b/kmymoney/models/accountsproxymodel.h index 3f5931c47..717bdf1e7 100644 --- a/kmymoney/models/accountsproxymodel.h +++ b/kmymoney/models/accountsproxymodel.h @@ -25,11 +25,12 @@ // QT Includes #include <QtGlobal> // for QT_VERSION macro +#include <QSortFilterProxyModel> #if QT_VERSION < QT_VERSION_CHECK(5,10,0) +#include <KItemModels/KRecursiveFilterProxyModel> #define QSortFilterProxyModel KRecursiveFilterProxyModel #endif -#include <QSortFilterProxyModel> // ---------------------------------------------------------------------------- // KDE Includes diff --git a/kmymoney/models/equitiesmodel.h b/kmymoney/models/equitiesmodel.h index 11ff0a256..5e9da4b0d 100644 --- a/kmymoney/models/equitiesmodel.h +++ b/kmymoney/models/equitiesmodel.h @@ -24,11 +24,13 @@ // QT Includes #include <QStandardItemModel> +#include <QSortFilterProxyModel> #if QT_VERSION < QT_VERSION_CHECK(5,10,0) +#include <KItemModels/KRecursiveFilterProxyModel> #define QSortFilterProxyModel KRecursiveFilterProxyModel #endif -#include <QSortFilterProxyModel> + // ---------------------------------------------------------------------------- // KDE Includes diff --git a/kmymoney/models/securitiesmodel.h b/kmymoney/models/securitiesmodel.h index ea028fef4..28b170b22 100644 --- a/kmymoney/models/securitiesmodel.h +++ b/kmymoney/models/securitiesmodel.h @@ -24,10 +24,11 @@ // QT Includes #include <QStandardItemModel> +#include <QSortFilterProxyModel> #if QT_VERSION < QT_VERSION_CHECK(5,10,0) +#include <KItemModels/KRecursiveFilterProxyModel> #define QSortFilterProxyModel KRecursiveFilterProxyModel #endif -#include <QSortFilterProxyModel> // ---------------------------------------------------------------------------- // KDE Includes
I'd checked the documentation and I know <KRecursiveFilterProxyModel> works with other apps, but indeed it doesn't work here; I tried to figure out why, then gave up after grep'ping in too many cmake files :)
Anyway, added your patch and updated the diff. And yes, I have write access to KDE repo.
From a cursory glance it could be because you are defining qsfpm to be krfpm but qsfpm is also mentioned within its header file.
No, that's not it; I tested by changing one of the .h files in master (where QSFPM isn't there at all) and <KRecursive....> doesn't work either.
It just crosses my mind, that it would be a lot safer if we add
#undef QSortFilterProxyModel
to the end of the header files and enclose all the source code in the affected cpp files also with
#if QT_VERSION < QT_VERSION_CHECK(5,10,0) #define QSortFilterProxyModel KRecursiveFilterProxyModel #endif : : : #undef QSortFilterProxyModel
This way, the redefinition of QSFP cannot escape this context which would otherwise happen if the header file is included in another source file and that one references the real QSFP.
Or (just a suggestion) we could go with the original diff, which is safer but uglier to read? your call. :)
| kmymoney/models/accountsproxymodel.h | ||
|---|---|---|
| 116 | This needs to be removed, because it causes an infinite recursive call. We only need an empty method here in the KRecursiveFilterProxyModel case, because the method does not exist there. | |
| kmymoney/models/equitiesmodel.h | ||
| 105 | Remove this call. | |
| kmymoney/models/securitiesmodel.h | ||
| 100 | Remove this call | |
setRecursiveFilteringEnabled() method provided for backward compatibility should be empty
| kmymoney/models/accountsproxymodel.h | ||
|---|---|---|
| 116 | Right, I should have figured it out... :) | |
I tested the accountsproxymodel here with Qt 5.14.0, it works; and all unit tests pass (except qsqlcipher-test, which also fails on master). I couldn't figure out the Equities and Securities models, I haven't used KMyMoney before, so I don't know it that well.