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 <rnova...@redhat.com> > * Copyright (C) 2009-2010 Dario Freddi <d...@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