T6445 Fix order of the GnuPG options
ClosedPublic

Authored by andreylegayev on Apr 11 2020, 6:36 PM.

Details

Summary

Fix random order of GnuPG configuration components by defining pre-defined order.

Test Plan
  1. Build Kleoptra with new version of libkleo
  2. Open Kleopatra configuration window several times.

It can be done from Kleopatra main menu or by running kcmshell5 --desktopfile kleopatra_config_gnupgsystem.desktop kleopatra_config_gnupgsystem

Expected result: order of tabs should be fixed to:
gpg, gpgsm, gpg-agent, dirmngr, pinentry, scdaemon

Any other components (if any) should go after predefined order in alphabetical order.

Diff Detail

Repository
R90 PIM: Kleo Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
andreylegayev created this revision.Apr 11 2020, 6:36 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 11 2020, 6:36 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript

Added #if QT_VERSION >= QT_VERSION_CHECK(5,14,0) to make it buildable in gpg4win

mlaurent requested changes to this revision.Apr 19 2020, 6:22 PM
mlaurent added inline comments.
src/ui/cryptoconfigmodule.cpp
231

new line after ')'

235

QStringLiteral(...) around each element

248

(const auto &item : order)

This revision now requires changes to proceed.Apr 19 2020, 6:22 PM
andreylegayev marked 3 inline comments as done.

Fixed code style and added QStringLiteral

dvratil requested changes to this revision.Apr 20 2020, 2:04 PM
dvratil added a subscriber: dvratil.
dvratil added inline comments.
src/ui/cryptoconfigmodule.cpp
236

Make it static, there's no need to initialize this every time. It could also be something more efficient than QStringList, like std::array (since we know the size of the list beforehand).

263

You could just resolve the others list using another for loop and !result.contains() and get rid of all the QSets completely. The order list as well as the components list will always be short enough so that the two QSets (and the cost of constructing and subtracting them) will not be measurably faster than two for loops and a linear check.

This revision now requires changes to proceed.Apr 20 2020, 2:04 PM
andreylegayev marked an inline comment as done.
andreylegayev added inline comments.
src/ui/cryptoconfigmodule.cpp
236

Makes sense. Done.
I'm not writing on C++ every day and I'm curious is there difference: const static or static const in this particular place ?

263

You're right. I didn't think about small amount of items, tried to write universal code.
Added another for. Pls check new version.

dvratil accepted this revision.Apr 24 2020, 7:38 AM
dvratil added inline comments.
src/ui/cryptoconfigmodule.cpp
236

Technically there's no difference, the meaning is the same. But the are people who would argue with you endlessly about which way is the truly correct way of writing static const :)

@mlaurent I still see red "x" in Reviewers section. Could you approve it?

mlaurent accepted this revision.Apr 24 2020, 10:00 AM

Seems ok for me too

This revision is now accepted and ready to land.Apr 24 2020, 10:00 AM
This revision was automatically updated to reflect the committed changes.
andreylegayev retitled this revision from T6446 Fix order of the GnuPG options to T6445 Fix order of the GnuPG options.Apr 25 2020, 8:24 PM