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
