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
