D10568: Handle privilege operation confirmation prompts in SlaveBase

[David Faure]

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