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
