D7270: [FileUndoManager] Enable undoing changes in read-only folders

[David Faure]

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

INLINE COMMENTS

> fileundomanager.cpp:111
> +private:
> +    class UndoJobPrivate : public JobPrivate
> +    {

Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no?  (using Q_D of course)

> fileundomanager.cpp:116
> +        {
> +            // Do we need this variable at all?
> +            if (useElevatedPrivilege) {

It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no?
This requires storing that information together with the undo information...

On the other hand I can't think of a use case where the undo manager is used and we wouldn't want this
 (if I understand correctly, the flag in kio jobs is mostly so that jobs triggered programmatically rather than via user interaction never prompt with polkit; but only jobs from the user get undo support, and only jobs where polkit was used would need polkit during undo ... unless I'm missing something).

So I'm a bit undecided, but I welcome any thoughts in this reflection ;)

> fileundomanager.cpp:121
> +                m_caption = i18n("Undo Changes");
> +                m_message = i18n("Undoing from this folder requires root privileges. Do you want to continue?");
> +            }

"from this folder" is possibly incorrect.

The root privileges might be needed at destination, rather than "here".
So I think it would be more correct to say "Undoing this operation requires..."

> fileundomanager.cpp:135
> +        if (useElevatedPrivilege) {
> +            FileUndoManager::self()->d->m_privilegeExecFlag = KIO::PrivilegeExecution;
> +        }

The FileUndoManager is the one creating UndoJobs, isn't it?

So this line seems to me like it's the wrong way around, the job setting something in the undomanager.
Surely the undomanager knows already if it should set that flag or not, without needing every job to do that for him.

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

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