https://bugs.kde.org/show_bug.cgi?id=254779
--- Comment #2 from Rolf Eike Beer <k...@opensource.sf-tec.de> ---
The patch looks good in principle, however I have some nitpicks:
Optional:
-if you have a KDE account I would welcome if you could put the patch into
Reviewboard instead, referencing this bug id
-if at all possible I would like to have this patch rebased on the frameworks
branch as Applications/16.08 is feature and string frozen and I hope we will
get the frameworks branch ready before 16.12, i.e. there will likely be no
further releases from the master branch
Mandatory:
-as you are using git to create the patch, please do a local "git commit" and
"git format-patch -1" to generate the patch, containing your correct author
information as well as a "BUG:254779" and "REVIEW:…" line
-m_expiry is used but not declared
-the initializer list for KGpgSortFilterProxyModel misses some linebreaks, see
e.g. SearchResult for an example
-KGpgSortFilterProxyModel is a bit too general as class name, as this filter is
about search results I would suggest something like
KGpgSearchResultFilterModel
-since this class can only properly work with a KGpgSearchResultModel as input
model I would add this as a required argument in the constructor, so no
invalid state can accidentially happen
-the logic in filterAcceptsRow() should be rewritten to first check the parent
filterAcceptsRow(). If that returns false, just return false, otherwise don't
bother to check it in the rest of the function. Since both branches of the
final "if" end returning the same value move that code out of the if. The
coding style for the else is also wrong, it should be "} else {" in one line,
although the else can go entirely away now that the if-branch became empty.
-I obviously fail the wtf-test with my model design, so I suggest you remove
your comment in this commit and come up with one that has a better code design
Not that mandatory:
-even more, since KGpgSearchResultModel is now never used "alone" I would make
the former model an implementation detail of the proxy model (i.e. private)
and have the new class as KGpgSearchResultModel
--
You are receiving this mail because:
You are watching all bug changes.
