| Summary: | kgpg: prevent import of outdated keys | ||
|---|---|---|---|
| Product: | [Applications] kgpg | Reporter: | bernhard |
| Component: | general | Assignee: | Rolf Eike Beer <kde> |
| Status: | RESOLVED FIXED | ||
| Severity: | wishlist | CC: | diazona |
| Priority: | NOR | ||
| Version First Reported In: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | openSUSE | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/kgpg/95c6901392c17ef174a63188447f58d02338aeef | Version Fixed/Implemented In: | |
| Sentry Crash Report: | |||
| Attachments: | Patch that adds a checkbox to filter out invalid keys | ||
|
Description
bernhard
2010-10-20 18:31:20 UTC
Created attachment 100193 [details]
Patch that adds a checkbox to filter out invalid keys
I've made some progress on a patch for this issue. It adds a checkbox which restricts the display to only valid keys when checked. Also, the label giving the number of keys found now reflects how many are actually shown in the list. The patch was prepared relative to revision 89cbf56.
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
OK, I'll make those changes and try to submit the patch to Reviewboard once I'm done. I didn't change the storage model (the part that fails the "wtf test") because I thought it best to limit my changes to those actually necessary to add the feature. But since you mentioned it, I will look into changing the model, and if it works out, I'll include both changes in the revised patch. m_expiry was already declared as a private field of SearchResult; it was just never used before. > I didn't change the storage model (the part that fails the "wtf test") > because > I thought it best to limit my changes to those actually necessary to > add the > feature. But since you mentioned it, I will look into changing the > model, and > if it works out, I'll include both changes in the revised patch. Please do that in 2 patches for easier review, in any order that fits your needs. > m_expiry was already declared as a private field of SearchResult; it > was just > never used before. Sorry, I was looking at the frameworks branch, where it got removed for exactly that reason ;) Eike I've completed the new patches (subject to coding style cleanup or design changes), incorporating the feedback from above. I wound up not changing the code design of the model, but I did add some comments to explain why the model used is preferable to alternatives, and I also refactored some code into methods to make it less confusing. So I have two commits: one that implements these preliminary changes, and another one that adds the key validity filtering on top of that. The thing is, as far as I can tell I can't upload the second patch to Reviewboard because it's based on a commit that's not in the kgpg git repository. Should I upload the first patch and wait for it to be reviewed and added to the repository before submitting the next one, or submit one patch with all the changes together, or upload one or both patches here, or something else? Put a merged patch (i.e. changes of both patches) into RB, but later commit the separated ones. OK, it's uploaded as review 128701. Git commit 95c6901392c17ef174a63188447f58d02338aeef by Rolf Eike Beer, on behalf of David Zaslavsky. Committed on 20/08/2016 at 07:07. Pushed by dakon into branch 'frameworks'. Show only valid keys in search results This adds - fields to SearchResult for expiration and revocation status - a proxy model to filter only valid keys from the search results - a checkbox to the keyserver search result dialog to toggle validity filtering REVIEW:128701 M +38 -19 keyservers.cpp M +3 -3 keyservers.h M +111 -14 model/kgpgsearchresultmodel.cpp M +87 -15 model/kgpgsearchresultmodel.h M +10 -0 searchres.ui http://commits.kde.org/kgpg/95c6901392c17ef174a63188447f58d02338aeef |