sitter requested changes to this revision.
sitter added a comment.
This revision now requires changes to proceed.
The way details are enumerated needs changing IMO. And this effectively bumps
the required polkitqt version through exactly one call, so maybe an #if
conditional to not have the forced bump might be good
INLINE COMMENTS
> AuthBackend.h:61
> virtual bool isCallerAuthorized(const QString &action, QByteArray
> callerID) = 0;
> + virtual bool isCallerAuthorized(const QString &action, const DetailsMap
> &details, QByteArray callerID) = 0; // KF6 TODO Merge
> virtual bool actionExists(const QString &action);
I didn't check super carefully but at a glance the backend api is not public
API so we could probably refactor this right now already.
Also, shouldn't the callerID be a const ref?
> DBusHelperProxy.cpp:109
>
> + qDebug() << details;
> QList<QVariant> args;
left over debugging? either remove or categorize
> Polkit1Backend.cpp:187
> + QMap<QString, QString> polkit1Details;
> + for (auto it = details.begin(); it != details.end(); ++it) {
> + polkit1Details.insert(it.key(), it.value().toString());
you want constBegin/End here
> Polkit1Backend.cpp:193
> &e, SLOT(requestQuit(PolkitQt1::Authority::Result)));
> - authority->checkAuthorization(action, subject,
> PolkitQt1::Authority::AllowUserInteraction);
> + authority->checkAuthorizationWithDetails(action, subject,
> PolkitQt1::Authority::AllowUserInteraction, polkit1Details);
> e.exec();
looks to me this was only added 4 months ago. so this would probably require a
version bump. seeing as we are fairly conservative with frameworks'
requirements it may be better to `if version>=whatevs` the call.
> kauthaction.cpp:77
> Action::Action(const QString &name, const QString &details)
> : d(new ActionData())
> {
you could just delegate to the new constructor here instead of having two code
duplicated ctors.
> kauthaction.h:236
> + */
> + void setDetails(const DetailsMap &details);
> +
this seems like super leaky abstraction. you are allowing the caller to set
backend specific stuff here. I think it'd be much better to make an enum for
detail types, and have this be a QMap of enum,QVariant.
if the caller sets polkit.message then that won't apply to the mac backend even
if someone were to implement the relevant functionality there. if it is a
general purpose enum key each backend can easily implement or ignore as
necessary
> kauthaction.h:246
> + */
> + QString details() const; // KF6 TODO: Remove
>
Should the old functions maybe be marked deprecated?
REPOSITORY
R283 KAuth
REVISION DETAIL
https://phabricator.kde.org/D21795
To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: sitter, mreeves, kde-frameworks-devel, LeGast00n, sbergeron, michaelh,
ngraham, bruns
