[Kde-pim] Review Request 127181: Add a 'trust owner' dialog for newly imported secret keys

[Andre Heinecke]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127181/#review92800
-----------------------------------------------------------




kleopatra/commands/importcertificatescommand.cpp (line 5)
<https://git.reviewboard.kde.org/r/127181/#comment63230>

    Please add Intevation Copyright.



kleopatra/commands/importcertificatescommand.cpp (line 37)
<https://git.reviewboard.kde.org/r/127181/#comment63218>

    Such comments are unneccessary in most cases. It might make sense if you include some strange header but for the standard library headers it's uneccessary.



kleopatra/commands/importcertificatescommand.cpp (line 410)
<https://git.reviewboard.kde.org/r/127181/#comment63219>

    I prefer Q_FOREACH where possible in new code to iterate over a complete list.
    
    Q_FOREACH(const ImportResult &result, results) {
    
    Also (and this goes for the full patch) coding style. Please take a look at https://techbase.kde.org/Policies/Kdelibs_Coding_Style



kleopatra/commands/importcertificatescommand.cpp (line 414)
<https://git.reviewboard.kde.org/r/127181/#comment63220>

    Are you sure this is always the finterprint of the primary key? E.g. what happens if you import a secret key that was already imported but only had a subkey added?



kleopatra/commands/importcertificatescommand.cpp (line 416)
<https://git.reviewboard.kde.org/r/127181/#comment63221>

    You need to delete the allocated context after usage.
    
    Maybe a QScopedPointer?



kleopatra/commands/importcertificatescommand.cpp (line 423)
<https://git.reviewboard.kde.org/r/127181/#comment63223>

    const Key



kleopatra/commands/importcertificatescommand.cpp (line 430)
<https://git.reviewboard.kde.org/r/127181/#comment63222>

    const QString



kleopatra/commands/importcertificatescommand.cpp (line 432)
<https://git.reviewboard.kde.org/r/127181/#comment63225>

    While fingerprint is neccessary here it's not super user friendly.
    
    If you change owner trust on the command line it also shows you all the UID's of that key. We should do that, too.



kleopatra/commands/importcertificatescommand.cpp (line 433)
<https://git.reviewboard.kde.org/r/127181/#comment63224>

    Fully Trusted and Ultimately Trusted are different values in OpenPGP.
    
    I would avoid using the word Ultimately though and just ask "Is this your own key?"



kleopatra/commands/importcertificatescommand.cpp (line 443)
<https://git.reviewboard.kde.org/r/127181/#comment63226>

    Why this indrection? The macros are just wrong here. If you want 0 or 1 you should write that. Especially if you later check for 0 and not the macro.
    But better just store the MessageBox result.



kleopatra/commands/importcertificatescommand.cpp (line 445)
<https://git.reviewboard.kde.org/r/127181/#comment63227>

    k == KMessageBox::Yes



kleopatra/commands/importcertificatescommand.cpp (line 450)
<https://git.reviewboard.kde.org/r/127181/#comment63229>

    You can hardcode this as Protocol::OpenPGP



kleopatra/commands/importcertificatescommand.cpp (line 453)
<https://git.reviewboard.kde.org/r/127181/#comment63228>

    return to avoid crash.


- Andre Heinecke


On Feb. 25, 2016, 6:23 p.m., Sean Engelhardt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127181/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2016, 6:23 p.m.)
> 
> 
> Review request for KDEPIM and Andre Heinecke.
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> When importing a new secret key in Kleopatra, a dialog-window will pop up to determinate if the owner of the newly imported key should be trusted or not.
> Since imported secret keys are usually the own ones it would make sense to enhance the trusting-process.
> 
> 
> Diffs
> -----
> 
>   kleopatra/commands/importcertificatescommand.cpp 18a65d1 
> 
> Diff: https://git.reviewboard.kde.org/r/127181/diff/
> 
> 
> Testing
> -------
> 
> Imported newly generated keys to determinate if the script gets triggered and works as requested.
> 
> 
> Thanks,
> 
> Sean Engelhardt
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/