Differential D6516

Fix selecting the color from a set of filters
ClosedPublic
Actions

Authored by justusw on Jul 5 2017, 3:09 PM.

Tags

Subscribers

Tokens

"Pterodactyl" token, awarded by aheinecke.

Details

Reviewers

Summary

Invert the condition. Previously, filters were ignored that presented
a valid color. Now those with a valid color are selected. This was
likely a copy&paste error from 'get_string'.

Signed-off-by: Justus Winter <justus@gnupg.org>

Refactor compliance code

Refactor the code checking whether a key is compliant into a new
function and export that.

Signed-off-by: Justus Winter <justus@gnupg.org>

New function checking if a key is valid

A key is valid if all UIDs have at least the validity 'full'.

Signed-off-by: Justus Winter <justus@gnupg.org>

Tweak complianceStringForKey

According to the design spec, a key should only be usable for
VS-compliant conversations, if it is valid.

Signed-off-by: Justus Winter <justus@gnupg.org>

Separate filter code from method to helper function

In the same way that 'bgColor' uses 'get_color'. This makes it
possible to search through two sets of filters.

Signed-off-by: Justus Winter <justus@gnupg.org>

Add and use a second set of key filters

The second set is not part of the model (i.e. it will not be displayed
in Kleopatras drop down list in the default view), and will only be
used for styling keys.

Signed-off-by: Justus Winter <justus@gnupg.org>

Add filters for keys that are (not) VS-compliant

Add a filter that selects VS-compliant keys, and two that highlight
the validity of the keys using a background color.

Signed-off-by: Justus Winter <justus@gnupg.org>

Add and use function providing a short compliance string for keys

Include short compliance information in key lists and combo boxes.

Signed-off-by: Justus Winter <justus@gnupg.org>

Add a field 'Validity' to the key model

Add a new field for the validity of keys, and populate it with
validity and compliance information.

Signed-off-by: Justus Winter <justus@gnupg.org>

Rework the keyfilter model

Rework the 'data(..)' method, use the filter ids as UserRole, so that
we can match on them.

Signed-off-by: Justus Winter <justus@gnupg.org>

Add a filter for keys that are not yet certified

And also plug it into the list it if the trust model is "direct".

Signed-off-by: Justus Winter <justus@gnupg.org>

Here arc, butcher this commit...

Diff Detail

Repository

R90 PIM: Kleo Library

Branch

justus/compliance

Lint

No Linters Available

Unit

No Unit Test Coverage

justusw created this revision.Jul 5 2017, 3:09 PM

Restricted Application added a project: KDE PIM. · View Herald TranscriptJul 5 2017, 3:09 PM

Restricted Application added a subscriber: KDE PIM. · View Herald Transcript

justusw added inline comments.Jul 6 2017, 10:18 AM

src/utils/formatting.cpp
825 ↗	(On Diff #16206)	So obviously, a predicate is not about formatting, so it shouldn't reside here. I thought about adding a module, e.g. 'compliance' or something, but I did not feel confident enough to do so and decide how to name it and where to put it. Guidance appreciated.

Looks good in general to me apart from my inline comments. The filter stuff I will test now because I can't see from the code if it works as intended.

Regarding style: Please replace all tabs with 4 spaces. And we don't have a space between "!" and the statement.

The Style is documented under:
https://techbase.kde.org/Policies/Kdelibs_Coding_Style

It even has an Emacs Mode for KDE ;-P

src/utils/formatting.cpp
825 ↗	(On Diff #16206)	I think it's ok to have it in Formatting: isKeyValid -> Should we display this as a valid key? Is kind of formatting for me. A better place might be GpgMEpp directly, we might add it there but still add it here to avoid a new round of release / dependency bump. I'd like you to rename the function though to something like "uidsHaveFullValidity" to avoid confusion because "valid" in other contexts is used as some kind of "is key usable" (e.g. not expired, not revoked, etc.)" and the use of just "valid" tends to be confusing.
829 ↗	(On Diff #16206)	I think it's ok for a key not to have any user id. In that case this function would return true. Also I think we need to skip revoked or expired UserID's here similar to isDeVs As a minor nitpick I would just return false once a uid.validity() < UserID::Validity::Full is found.
885 ↗	(On Diff #16206)	I think we can use the now called isKeyValid function for this.

This revision now requires changes to proceed.Jul 6 2017, 12:51 PM

Refactor compliance code
New function checking if a key is valid
Tweak complianceStringForKey
Separate filter code from method to helper function
Add and use a second set of key filters
Add filters for keys that are (not) VS-compliant
Add and use function providing a short compliance string for keys
Add a field 'Validity' to the key model
Rework the keyfilter model
Add a filter for keys that are not yet certified
Do not display too advanced options in CO_DE_VS mode
Here arc, butcher this commit...

In D6516#122148, @aheinecke wrote:

Looks good in general to me apart from my inline comments. The filter stuff I will test now because I can't see from the code if it works as intended.

Cool.

Regarding style: Please replace all tabs with 4 spaces. And we don't have a space between "!" and the statement.

Right.

The Style is documented under:
https://techbase.kde.org/Policies/Kdelibs_Coding_Style

It even has an Emacs Mode for KDE ;-P

The kde-emacs stuff doesn't work for me, also, it changes the indentation mode unconditionally, which I despise for obvious reasons. Usually, I have a 'do-the-right-thing' mode, I'm not sure why it failed for the formatting.cpp, maybe because of the local variables, which I took the liberty to update now.

Looks good to me. But we bump the version for new API even in master.

So please also add:
{{{
diff --git a/CMakeLists.txt b/CMakeLists.txt
index fbba805..21ae7fc 100644

a/CMakeLists.txt

+++ b/CMakeLists.txt
@@ -1,5 +1,5 @@
cmake_minimum_required(VERSION 3.0)
-set(PIM_VERSION "5.5.80")
+set(PIM_VERSION "5.5.81")

project(libkleo VERSION ${PIM_VERSION})

}}}

And please wait ~10 minutes before pushing D6517 to give build.kde.org a chance to provide the needed libkleo version.

This revision is now accepted and ready to land.Jul 10 2017, 11:54 AM

aheinecke mentioned this in D6517: Add helper for dealing with compliance modes.Jul 10 2017, 11:55 AM

I fixed some more coding style violations, bumped the version, and pushed it. Thanks for guiding me :)

Revision Contents
Changeset List

			Path	Packages
M			src/kleo/keyfiltermanager.cpp (166 lines)
M			src/models/keylistmodel.cpp (3 lines)
M			src/models/keylistmodelinterface.h (1 line)
M			src/ui/cryptoconfigmodule.cpp (7 lines)
M			src/ui/keyselectioncombo.cpp (3 lines)
M			src/utils/formatting.h (11 lines)
M			src/utils/formatting.cpp (66 lines)

Diff 16235

src/kleo/keyfiltermanager.cpp

Loading...

src/models/keylistmodel.cpp

Loading...

src/models/keylistmodelinterface.h

Loading...

src/ui/cryptoconfigmodule.cpp

Loading...

src/ui/keyselectioncombo.cpp

Loading...

src/utils/formatting.h

Loading...

src/utils/formatting.cpp

Loading...