Details
- Reviewers
JochenSaalfeld - Maniphest Tasks
- T1991: Export secret key as Paperkey
Diff Detail
- Repository
- R168 Kleopatra
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Thanks for the Patch. Generally it's a bit much copy and paste in my opinion without questioning if the pasted code really makes sense for this. While we've talked about not basing it on Gnuprocesscommand I think some of that code is redundant and we should probably change gnupgprocesscommand that it's a useful base class for this. I'll take a look at it.
I've added some inline comments but basically your patch is ok. I'll rework your patch a bit and change it to "Print secret key" which will output a PDF or directly open the Print dialog. I don't think we should put paperkey into the user interface as it is an unknown tool for users. We should also inform the user that she should upload the public key or back it up for a full restore.
| src/CMakeLists.txt | ||
|---|---|---|
| 57 | Please remove the line break changes from this patch. | |
| src/commands/exportpaperkeycommand.cpp | ||
| 92 | Why is this necessary and not just part of arguments? | |
| src/commands/exportpaperkeycommand.h | ||
| 2 | filename :-) | |
| src/commands/paperkeyprocesscommand.cpp | ||
| 202 | In new code it's better to use the Q_D Q_Q macros and the according declarations. | |
| 277 | This appears overly complicated to me. You have a specialized class for paperkey export so you don't need generic argument parsing. The whole split between paperkeyexporcommand and paperkeyprocesscommand appears arbitrary and unnecessary to me. | |
| 337 | Probably better to use something like GnuPG Output: and Paperkey Output: here instead of Process1 and 2 as the processes are hardcoded. | |
| 343 | why the else at all. | |
| 373 | instead of the ba.chop stuff you can just call QString::trimmed() here. | |
| 426 | The moc includes should not be necessary. | |
| src/dialogs/exportpaperkeydialog.cpp | ||
| 74 | Copy paste artifact? | |
| 104 | QStringLiteral (" .txt") instead of the two strings and the extensions array. | |
| 114 | if fn.endsWith(QStringLiteral(".txt"). You only have the one :-) | |
| 233 | Should not be necessary thanks to automoc. | |
| src/dialogs/exportpaperkeydialog.h | ||
| 2 | filename :-) | |
| 78 | While I see that this is copied from exportsecretkey I don't think a private class implementation makes much sense in such specialized code. | |
| src/dialogs/exportpaperkeydialog.ui | ||
| 9 | The dialog did not have a correct height for me. The raw and base16 radio buttons were "squashed". | |
| src/kleopatra.rc | ||
| 14 | You need to bump the version of this file if you make changes. Otherwise the new actions are not shown if a user has modified his config. | |
| src/missing-icons.txt | ||
| 12 | I guess until we have an icon for this it would be better to just reuse the export secret key icon. | |
| src/utils/action_data.cpp | ||
| 75 | whitespace error and style only change. | |
| src/utils/paperkey-helper.h | ||
| 2 | filename :-) | |
| 44 | Imo you should just put the three functions here into gnupg-helper the name may not be 100% fitting but from the type of functions it makes sense there. | |
| src/view/keylistcontroller.cpp | ||
| 388 | You are using tabs at least here. Please just use whitespace everywhere for indention. | |
| 428 | ? | |