D10568: Handle privilege operation confirmation prompts in SlaveBase
This revision was automatically updated to reflect the committed changes. Closed by commit R241:0e45fcffd182: Handle privilege operation confirmation prompts in SlaveBase (authored by chinmoyr). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10568?vs=31156&id=31225 REVISION DETAIL https://phabricator.kde.org/D10568 AFFECTED FILES src/core/slavebase.cpp To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh, ngraham
D10568: Handle privilege operation confirmation prompts in SlaveBase
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH arcpatch-D10568 REVISION DETAIL https://phabricator.kde.org/D10568 To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh, ngraham
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in slavebase.cpp:517
> BTW now that there are 5 duplicated lines below the //reset comment (in error
> and finished), it would be worth extracting a reset function...
I think it will be better to have a separate commit for that.
> dfaure wrote in slavebase.cpp:1495
> This reads like it's going to ask confirmation every time this method is
> called (once we are in OperationAllowed state).
>
> The method impl uses a bool to ask only once, but that doesn't show here.
>
> One solution is to rename the method to maybeAskConfirmation, but that's not
> great.
> Better might be to test the bool here?
>
> if (d->m_privilegeOperationStatus == OperationAllowed &&
> !d->m_confirmationAsked) {
> d->m_confirmationAsked = true;
> d->m_privilegeOperationStatus = d->askConfirmation();
> }
>
> This implies a small behavior change: in your patch, if the user presses
> Cancel, then he might still get asked again, while in my case he wouldn't.
> But, unless I'm wrong, after Cancel we'll go to SlaveBase::error() which will
> reset both member vars anyway, right?
Yes, clicking cancel will reset the variables. So setting the bool here is
totally fine and semantically more correct.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10568
To: chinmoyr, dfaure
Cc: fvogt, #frameworks, michaelh, ngraham
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr updated this revision to Diff 31156. chinmoyr marked 2 inline comments as done. chinmoyr added a comment. Addressed David's issues. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10568?vs=27316&id=31156 BRANCH arcpatch-D10568 REVISION DETAIL https://phabricator.kde.org/D10568 AFFECTED FILES src/core/slavebase.cpp To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh, ngraham
D10568: Handle privilege operation confirmation prompts in SlaveBase
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Just minor requests
INLINE COMMENTS
> slavebase.cpp:127
> +QString m_warningMessage;
> +bool m_confirmationAsked;
> int m_privilegeOperationStatus;
move next to the other bool (-> less padding)
> slavebase.cpp:517
> d->m_rootEntryListed = false;
> +d->m_confirmationAsked = false;
> d->m_privilegeOperationStatus = OperationNotAllowed;
BTW now that there are 5 duplicated lines below the //reset comment (in error
and finished), it would be worth extracting a reset function...
> slavebase.cpp:1495
> +if (d->m_privilegeOperationStatus == OperationAllowed) {
> +d->m_privilegeOperationStatus = d->askConfirmation();
> }
This reads like it's going to ask confirmation every time this method is called
(once we are in OperationAllowed state).
The method impl uses a bool to ask only once, but that doesn't show here.
One solution is to rename the method to maybeAskConfirmation, but that's not
great.
Better might be to test the bool here?
if (d->m_privilegeOperationStatus == OperationAllowed &&
!d->m_confirmationAsked) {
d->m_confirmationAsked = true;
d->m_privilegeOperationStatus = d->askConfirmation();
}
This implies a small behavior change: in your patch, if the user presses
Cancel, then he might still get asked again, while in my case he wouldn't. But,
unless I'm wrong, after Cancel we'll go to SlaveBase::error() which will reset
both member vars anyway, right?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10568
To: chinmoyr, dfaure
Cc: fvogt, #frameworks, michaelh
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr added a dependent revision: D10818: Store PolicyKit action which the slave is authorized to perform. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10568 To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh, kmorwinski
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr removed a dependent revision: D10641: Revoke temporary authorization of KIO slave before sending status to IdleSlave. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10568 To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh, kmorwinski
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr edited the summary of this revision. chinmoyr added a dependency: D10567: Remove handling of privilege operation confirmation prompts from KIO::Job. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10568 To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr added a dependent revision: D10641: Revoke temporary authorization of KIO slave before sending it to klauncher. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10568 To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh
D10568: Handle privilege operation confirmation prompts in SlaveBase
chinmoyr created this revision. chinmoyr added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY This way a confirmation dialog is always shown if the application allows privilege operation. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D10568 AFFECTED FILES src/core/slavebase.cpp To: chinmoyr, dfaure Cc: fvogt, #frameworks, michaelh
