D10568: Handle privilege operation confirmation prompts in SlaveBase

[Chinmoy Ranjan Pradhan]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180402/9e710878/attachment-0001.html>