D6832: Integrate new file ioslave in KIO job

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> job.cpp:345
> +    m_confirmationAsked = true;
> +    return status == 5;
> +}

This hardcoded 5 is awful, please use SlaveBase::Continue instead.

> job_base.h:222
> +     */
> +    bool isPrivilegeExecutionEnabled() const;
> +

This doesn't need to be public, does it? Could be in the private class.

> job_base.h:229
> +     */
> +    bool wasConfirmationAsked() const;
> +

This doesn't need to be public, does it? Could be in the private class.

> job_p.h:94
> +    FileOperationType m_operationType;
> +    bool tryAskPrivilegeOpConfirmation();
> +

Please keep all the member variables together. In the orig code it's already ugly that there are methods after the variables, but this makes it slightly worse with the enum in the middle and no empty line between vars and methods.

Ideally this should be
[...] private enum, private methods, all private member vars, end of class

> job_p.h:195
>      /**
> +     * Called when privilegeExecData() is emmited by the slave.
> +     *

typo: emitted

> simplejob.cpp:151
>  
> +    q->connect(slave, SIGNAL(privilegeExecData(QByteArray)),
> +               SLOT(slotPrivilegeExecData(QByteArray)));

This is not a very good signal name.
Signals notify a state changed and usually end with "ed".
Would it make sense to call this privilegeOperationRequested() or something?
I need to find more about when this is emitted and what the data is ;)

> simplejob.cpp:346
> +    QByteArray reply;
> +    if (data == QStringLiteral("CanElevatePrivilege")) {
> +        if (m_privilegeExecutionEnabled

use QLatin1String rather than QStringLiteral for comparisons

> simplejob.cpp:354
> +        if (confirmed) {
> +            reply = QByteArray("ActionConfirmed");
> +        }

OK, so the data is two possible strings, and the reply is two possible strings, why not use enums instead? You can define them in SlaveBase for instance.

You might have to use lambdas to avoid the enum appearing in Q_PRIVATE_SLOT in public headers (and requiring slavebase.h)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D6832

To: chinmoyr, dfaure, #frameworks
Cc: #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170724/eb89e0db/attachment.html>