Port KRecursiveFilterProxyModel to QSortFilterProxyModel
Needs ReviewPublic

Authored by ahmadsamir on Sat, Jan 11, 9:38 AM.

Details

Summary

Add QT_VERSION checks so that the code can still be built on Qt versions
older than 5.10 (that's where QSortFilterProxyModel gained recursive
filtering capabilities).

Test Plan

make && ctest; all unit tests pass except qsqlcipher-test, but it fails
on master too.

Diff Detail

Repository
R261 KMyMoney
Branch
l-qsortproxy (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21319
Build 21337: arc lint + arc unit
ahmadsamir requested review of this revision.Sat, Jan 11, 9:38 AM
ahmadsamir created this revision.

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.

OK, no problem. I'll add the necessary #ifdef's and update the diff.

ahmadsamir updated this revision to Diff 73444.Mon, Jan 13, 5:48 PM
ahmadsamir retitled this revision from Replace KRecursiveFilterProxyModel with QSortFilterProxyModel to Port KRecursiveFilterProxyModel to QSortFilterProxyModel.
ahmadsamir edited the summary of this revision. (Show Details)

Add QT_VERSION checks to keep the code buildable with older Qt versions

tbaumgart requested changes to this revision.Mon, Jan 13, 7:01 PM

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
41

See above.

45

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) {}
#endif

and remove the enclosing #if/#endif in the code so it is prepared for the future.

54

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.

This revision now requires changes to proceed.Mon, Jan 13, 7:01 PM
ahmadsamir updated this revision to Diff 73811.Sat, Jan 18, 6:32 AM

Address comments

tbaumgart requested changes to this revision.Sat, Jan 18, 1:57 PM

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
This revision now requires changes to proceed.Sat, Jan 18, 1:57 PM
ahmadsamir updated this revision to Diff 73833.Sat, Jan 18, 3:26 PM
  • Add patch from review to fix build on Qt < 5.10.0

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?

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.

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?

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.

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?

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.

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. :)

I raise my hand for beautiful code