D21795: [KAuth] Add support for action details in Polkit1 backend.

[Harald Sitter]

sitter requested changes to this revision.
sitter added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> Polkit1Backend.cpp:4
>  *   Copyright (C) 2009 Radek Novacek <rnovacek at redhat.com>
>  *   Copyright (C) 2009-2010 Dario Freddi <drf at kde.org>
>  *

According to the diff of the diff you seem to have lost David's copyright

> Polkit1Backend.cpp:75
>      connect(PolkitQt1::Authority::instance(), SIGNAL(enumerateActionsFinished(PolkitQt1::ActionDescription::List)),
>              this, SLOT(updateCachedActions(PolkitQt1::ActionDescription::List)));
>  

Is there a reason you use stringy connection syntax instead of &member syntax?

> Polkit1Backend.cpp:178
>  
> -bool Polkit1Backend::isCallerAuthorized(const QString &action, QByteArray callerID)
> +bool Polkit1Backend::isCallerAuthorized(const QString& action, const QByteArray &callerID, const QVariantMap& details)
>  {

action has & on the wrong size of the space, same for details ;)

> Polkit1Backend.cpp:239
>              // Wait max 2 seconds
>              QEventLoop e;
>              QTimer::singleShot(200, &e, SLOT(quit()));

Mhhhh. Mhhhhhhhh. I don't really have a suggestion here, but this is an incredibly dangerous change. Nested event loops can cause all sorts of negative effects. That's why the isValid had a note in the documentation, to tell the frontend dev to be careful. By adding a new loop in existing api we add a pit to fall into. I really don't know what can be done about it though. Could we get by with 1 second maybe?

> kauthaction.cpp:23
>  #include <QtGlobal>
>  #include <QRegExp>
>  

I think that is deprecated, or at least not recommended for use anymore. You'll want QRegularExpression

> kauthaction.cpp:53
>      QVariantMap args;
>      bool valid;
>      QWidget *parent = nullptr;

Mh, minor annoyance. valid and timeout should get their defaults set here as a more modern best practice and also because parent is already set for consistency. That default ctor is kinda pointless then and should be dropped IMHO.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D21795

To: feverfew, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter, chinmoyr
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200226/51a8d6cb/attachment-0001.html>