Differential D7270 Diff 25228 src/widgets/fileundomanager.cpp

Changeset View

Standalone View

View Options

src/widgets/fileundomanager.cpp

Show All 28 Lines
29#include <kio/mkdirjob.h> 29​#include <kio/mkdirjob.h>
30#include <kio/mkpathjob.h> 30​#include <kio/mkpathjob.h>
31#include <kio/batchrenamejob.h> 31​#include <kio/batchrenamejob.h>
32#include <kjobwidgets.h> 32​#include <kjobwidgets.h>
33#include <klocalizedstring.h> 33​#include <klocalizedstring.h>
34#include <kmessagebox.h> 34​#include <kmessagebox.h>
35#include <kjobtrackerinterface.h> 35​#include <kjobtrackerinterface.h>
36#include <kio/jobuidelegate.h> 36​#include <kio/jobuidelegate.h>
37​#include <job_p.h>
37 38​
38#include <QDateTime> 39​#include <QDateTime>
39#include <QDBusConnection> 40​#include <QDBusConnection>
40 41​
41#include <assert.h> 42​#include <assert.h>
42 43​
43using namespace KIO; 44​using namespace KIO;
44 45​
▲ Show 20 Lines • Show All 58 Lines • ▼ Show 20 Line(s)
103 * -> see also fileundomanagertest, which tests some of the above (but not renaming). 104​ * -> see also fileundomanagertest, which tests some of the above (but not renaming).
104 * 105​ *
105 */ 106​ */
106 107​
107class KIO::UndoJob : public KIO::Job 108​class KIO::UndoJob : public KIO::Job
108{ 109​{
109 Q_OBJECT 110​ Q_OBJECT
110public: 111​public:
111 UndoJob(bool showProgressInfo) : KIO::Job() 112​ UndoJob(bool showProgressInfo) : KIO::Job()
dfaureUnsubmitted
Not Done

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

dfaure: Is this subclass needed? You could just move the code of its constructor to the UndoJob…
112 { 113​ {
113 if (showProgressInfo) { 114​ if (showProgressInfo) {
114 KIO::getJobTracker()->registerJob(this); 115​ KIO::getJobTracker()->registerJob(this);
115 } 116​ }
117​
dfaureUnsubmitted
Not Done

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 ;)

dfaure: It seems to me that we only want to undo with "privilege execution enabled" if the original job…
118​ d_ptr->m_privilegeExecutionEnabled = true;
119​ d_ptr->m_operationType = d_ptr->Other;
dfaureUnsubmitted
Not Done

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.

dfaure: The FileUndoManager is the one creating UndoJobs, isn't it? So this line seems to me like it's…
120​ d_ptr->m_caption = i18n("Undo Changes");
121​ d_ptr->m_message = i18n("Undoing this operation requires root privileges. Do you want to continue?");
116 } 122​ }
dfaureUnsubmitted
Not Done

"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..."

dfaure: "from this folder" is possibly incorrect. The root privileges might be needed at destination…
117 virtual ~UndoJob() {} 123​ virtual ~UndoJob() {}
118 124​
119 virtual void kill(bool) 125​ virtual void kill(bool)
120 { 126​ {
121 FileUndoManager::self()->d->stopUndo(true); 127​ FileUndoManager::self()->d->stopUndo(true);
122 KIO::Job::doKill(); 128​ KIO::Job::doKill();
123 } 129​ }
124 130​
▲ Show 20 Lines • Show All 396 Lines • ▼ Show 20 Line(s)
521} 527​}
522 528​
523void FileUndoManagerPrivate::stepMakingDirectories() 529​void FileUndoManagerPrivate::stepMakingDirectories()
524{ 530​{
525 if (!m_dirStack.isEmpty()) { 531​ if (!m_dirStack.isEmpty()) {
526 QUrl dir = m_dirStack.pop(); 532​ QUrl dir = m_dirStack.pop();
527 //qDebug() << "creatingDir" << dir; 533​ //qDebug() << "creatingDir" << dir;
528 m_currentJob = KIO::mkdir(dir); 534​ m_currentJob = KIO::mkdir(dir);
535​ m_currentJob->setParentJob(m_undoJob);
529 m_undoJob->emitCreatingDir(dir); 536​ m_undoJob->emitCreatingDir(dir);
530 } else { 537​ } else {
531 m_undoState = MOVINGFILES; 538​ m_undoState = MOVINGFILES;
532 } 539​ }
533} 540​}
534 541​
535// Misnamed method: It moves files back, but it also 542​// Misnamed method: It moves files back, but it also
536// renames directories back, recreates symlinks, 543​// renames directories back, recreates symlinks,
Show All 31 Lines547 if (!m_current.m_opStack.isEmpty()) {
568 } else if (m_current.isMoveCommand() 575​ } else if (m_current.isMoveCommand()
569 || m_current.m_type == FileUndoManager::Trash) { 576​ || m_current.m_type == FileUndoManager::Trash) {
570 //qDebug() << "file_move" << op.m_dst << op.m_src; 577​ //qDebug() << "file_move" << op.m_dst << op.m_src;
571 m_currentJob = KIO::file_move(op.m_dst, op.m_src, -1, KIO::Overwrite | KIO::HideProgressInfo); 578​ m_currentJob = KIO::file_move(op.m_dst, op.m_src, -1, KIO::Overwrite | KIO::HideProgressInfo);
572 m_currentJob->uiDelegateExtension()->createClipboardUpdater(m_currentJob, JobUiDelegateExtension::UpdateContent); 579​ m_currentJob->uiDelegateExtension()->createClipboardUpdater(m_currentJob, JobUiDelegateExtension::UpdateContent);
573 m_undoJob->emitMoving(op.m_dst, op.m_src); 580​ m_undoJob->emitMoving(op.m_dst, op.m_src);
574 } 581​ }
575 582​
583​ if (m_currentJob) {
584​ m_currentJob->setParentJob(m_undoJob);
585​ }
586​
576 m_current.m_opStack.removeLast(); 587​ m_current.m_opStack.removeLast();
577 // The above KIO jobs are lowlevel, they don't trigger KDirNotify notification 588​ // The above KIO jobs are lowlevel, they don't trigger KDirNotify notification
578 // So we need to do it ourselves (but schedule it to the end of the undo, to compress them) 589​ // So we need to do it ourselves (but schedule it to the end of the undo, to compress them)
579 QUrl url = op.m_dst.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash); 590​ QUrl url = op.m_dst.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash);
580 addDirToUpdate(url); 591​ addDirToUpdate(url);
581 592​
582 url = op.m_src.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash); 593​ url = op.m_src.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash);
583 addDirToUpdate(url); 594​ addDirToUpdate(url);
584 } else { 595​ } else {
585 m_undoState = REMOVINGLINKS; 596​ m_undoState = REMOVINGLINKS;
586 } 597​ }
587} 598​}
588 599​
589void FileUndoManagerPrivate::stepRemovingLinks() 600​void FileUndoManagerPrivate::stepRemovingLinks()
590{ 601​{
591 //qDebug() << "REMOVINGLINKS"; 602​ //qDebug() << "REMOVINGLINKS";
592 if (!m_fileCleanupStack.isEmpty()) { 603​ if (!m_fileCleanupStack.isEmpty()) {
593 QUrl file = m_fileCleanupStack.pop(); 604​ QUrl file = m_fileCleanupStack.pop();
594 //qDebug() << "file_delete" << file; 605​ //qDebug() << "file_delete" << file;
595 m_currentJob = KIO::file_delete(file, KIO::HideProgressInfo); 606​ m_currentJob = KIO::file_delete(file, KIO::HideProgressInfo);
607​ m_currentJob->setParentJob(m_undoJob);
596 m_undoJob->emitDeleting(file); 608​ m_undoJob->emitDeleting(file);
597 609​
598 QUrl url = file.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash); 610​ QUrl url = file.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash);
599 addDirToUpdate(url); 611​ addDirToUpdate(url);
600 } else { 612​ } else {
601 m_undoState = REMOVINGDIRS; 613​ m_undoState = REMOVINGDIRS;
602 614​
603 if (m_dirCleanupStack.isEmpty() && m_current.m_type == FileUndoManager::Mkdir) { 615​ if (m_dirCleanupStack.isEmpty() && m_current.m_type == FileUndoManager::Mkdir) {
604 m_dirCleanupStack << m_current.m_dst; 616​ m_dirCleanupStack << m_current.m_dst;
605 } 617​ }
606 } 618​ }
607} 619​}
608 620​
609void FileUndoManagerPrivate::stepRemovingDirectories() 621​void FileUndoManagerPrivate::stepRemovingDirectories()
610{ 622​{
611 if (!m_dirCleanupStack.isEmpty()) { 623​ if (!m_dirCleanupStack.isEmpty()) {
612 QUrl dir = m_dirCleanupStack.pop(); 624​ QUrl dir = m_dirCleanupStack.pop();
613 //qDebug() << "rmdir" << dir; 625​ //qDebug() << "rmdir" << dir;
614 m_currentJob = KIO::rmdir(dir); 626​ m_currentJob = KIO::rmdir(dir);
627​ m_currentJob->setParentJob(m_undoJob);
615 m_undoJob->emitDeleting(dir); 628​ m_undoJob->emitDeleting(dir);
616 addDirToUpdate(dir); 629​ addDirToUpdate(dir);
617 } else { 630​ } else {
618 m_current.m_valid = false; 631​ m_current.m_valid = false;
619 m_currentJob = nullptr; 632​ m_currentJob = nullptr;
620 if (m_undoJob) { 633​ if (m_undoJob) {
621 //qDebug() << "deleting undojob"; 634​ //qDebug() << "deleting undojob";
622 m_undoJob->emitResult(); 635​ m_undoJob->emitResult();
▲ Show 20 Lines • Show All 147 Lines • Show Last 20 Lines