Dan Vratil | 16 Aug 2012 14:18
Picon
Gravatar

Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

Review request for kdelibs and David Faure.
By Dan Vratil.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Frank Reininghaus | 16 Aug 2012 15:10

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
I'm afraid the users suffering from https://bugs.kde.org/show_bug.cgi?id=206500 will kill us if they get a message box for every single file. Right now, they have the option to wait until the operation is completed, then close the first (and only) message box and be happy.

- Frank


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Ambroz Bizjak | 16 Aug 2012 15:32
Picon

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

On August 16th, 2012, 1:10 p.m., Frank Reininghaus wrote:

kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
I'm afraid the users suffering from https://bugs.kde.org/show_bug.cgi?id=206500 will kill us if they get a message box for every single file. Right now, they have the option to wait until the operation is completed, then close the first (and only) message box and be happy.
How about aggregating all the errors/warnings in a single dialog box as a list? E.g. the list would have fields like "message" and "file", and would allow the user to easily see all that went wrong and what files were involved. He could then select multiple entries and perform the same action on them, if applicable. E.g. if the message was "destination already exists, replace or skip?" he could select some of the entries and perform "ignore", but perform "replace" on others. The copying could continue in the background, and the replacements would be performed only once the user confirms then.

- Ambroz


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Dan Vratil | 16 Aug 2012 15:43
Picon
Gravatar

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

On August 16th, 2012, 1:10 p.m., Frank Reininghaus wrote:

kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
I'm afraid the users suffering from https://bugs.kde.org/show_bug.cgi?id=206500 will kill us if they get a message box for every single file. Right now, they have the option to wait until the operation is completed, then close the first (and only) message box and be happy.

On August 16th, 2012, 1:32 p.m., Ambroz Bizjak wrote:

How about aggregating all the errors/warnings in a single dialog box as a list? E.g. the list would have fields like "message" and "file", and would allow the user to easily see all that went wrong and what files were involved. He could then select multiple entries and perform the same action on them, if applicable. E.g. if the message was "destination already exists, replace or skip?" he could select some of the entries and perform "ignore", but perform "replace" on others. The copying could continue in the background, and the replacements would be performed only once the user confirms then.
But you can get multiple kinds of warnings during the operation. How would you separate them? You would either have to have a separate dialog for each kind of error or a very complex UI dialog. In general I like the idea of a box "These files failed to copy" (similar to the Delete Files dialog), but I think that simple 'OK' would be enough. Ideas?

- Dan


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Ambroz Bizjak | 16 Aug 2012 16:14
Picon

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

On August 16th, 2012, 1:10 p.m., Frank Reininghaus wrote:

kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
I'm afraid the users suffering from https://bugs.kde.org/show_bug.cgi?id=206500 will kill us if they get a message box for every single file. Right now, they have the option to wait until the operation is completed, then close the first (and only) message box and be happy.

On August 16th, 2012, 1:32 p.m., Ambroz Bizjak wrote:

How about aggregating all the errors/warnings in a single dialog box as a list? E.g. the list would have fields like "message" and "file", and would allow the user to easily see all that went wrong and what files were involved. He could then select multiple entries and perform the same action on them, if applicable. E.g. if the message was "destination already exists, replace or skip?" he could select some of the entries and perform "ignore", but perform "replace" on others. The copying could continue in the background, and the replacements would be performed only once the user confirms then.

On August 16th, 2012, 1:43 p.m., Dan Vratil wrote:

But you can get multiple kinds of warnings during the operation. How would you separate them? You would either have to have a separate dialog for each kind of error or a very complex UI dialog. In general I like the idea of a box "These files failed to copy" (similar to the Delete Files dialog), but I think that simple 'OK' would be enough. Ideas?
Yes, that's a good idea. When a file fails to copy, pop up the "These files failed to copy" dialog, but when another file fails, just add it to the list in the existing dialog. User can click "OK" which closes the dialog, and if more files fail, another one pops up. Small problem though: what happens if the user clicks OK, without noticing that another file was added to the list just before he closed the dialog? Maybe a safeguard should be added that no less than N seconds may pass from the time the last file was added to when the user clicks OK.

- Ambroz


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

David Faure | 16 Aug 2012 16:31
Picon
Favicon
Gravatar

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

On August 16th, 2012, 1:10 p.m., Frank Reininghaus wrote:

kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
I'm afraid the users suffering from https://bugs.kde.org/show_bug.cgi?id=206500 will kill us if they get a message box for every single file. Right now, they have the option to wait until the operation is completed, then close the first (and only) message box and be happy.

On August 16th, 2012, 1:32 p.m., Ambroz Bizjak wrote:

How about aggregating all the errors/warnings in a single dialog box as a list? E.g. the list would have fields like "message" and "file", and would allow the user to easily see all that went wrong and what files were involved. He could then select multiple entries and perform the same action on them, if applicable. E.g. if the message was "destination already exists, replace or skip?" he could select some of the entries and perform "ignore", but perform "replace" on others. The copying could continue in the background, and the replacements would be performed only once the user confirms then.

On August 16th, 2012, 1:43 p.m., Dan Vratil wrote:

But you can get multiple kinds of warnings during the operation. How would you separate them? You would either have to have a separate dialog for each kind of error or a very complex UI dialog. In general I like the idea of a box "These files failed to copy" (similar to the Delete Files dialog), but I think that simple 'OK' would be enough. Ideas?

On August 16th, 2012, 2:14 p.m., Ambroz Bizjak wrote:

Yes, that's a good idea. When a file fails to copy, pop up the "These files failed to copy" dialog, but when another file fails, just add it to the list in the existing dialog. User can click "OK" which closes the dialog, and if more files fail, another one pops up. Small problem though: what happens if the user clicks OK, without noticing that another file was added to the list just before he closed the dialog? Maybe a safeguard should be added that no less than N seconds may pass from the time the last file was added to when the user clicks OK.
Aggregating errors is part of Cyril Oblikov's SOC project already. I just asked him to aggregate warnings in that dialog too. This is a longer-term fix though.

- David


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

David Faure | 16 Aug 2012 16:09
Picon
Favicon
Gravatar

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

I think the right thing to do is to fix bug 206500, and then apply this patch, rather than come up with yet another feedback mechanism. Let's make warning() actually work, and let's make sure it's not emitted erroneously. I'll look into fixing bug 206500 as soon as possible (let's say beginning of next week).
kdeui/jobs/kdialogjobuidelegate.cpp (Diff revision 1) 92 92
void KDialogJobUiDelegate::showErrorMessage()
static uint msgBoxDisplayed = 0; KMessageBox::queuedMessageBox ( d->errorParentWidget, KMessageBox::Information, plain );
No space after the method name (this usually happens when copy/pasting from the Qt documentation :-) In fact, no space within the parenthesis either, but you couldn't guess that: existing code doesn't follow the kdelibs coding style. When writing new code, no spaces within (...).

kio/kio/job.cpp (Diff revision 1) 2620
void ListJobPrivate::gotEntries(KIO::Job *, const KIO::UDSEntryList& list )
// Left's emit a signal about this though
Left -> Let. How did this come in? It wasn't in my initial patch :)

- David


On August 16th, 2012, 12:18 p.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 16, 2012, 12:18 p.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Dan Vratil | 22 Aug 2012 10:24
Picon
Gravatar

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 22, 2012, 8:24 a.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs (updated)

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

David Faure | 4 Sep 2012 14:46
Picon
Favicon
Gravatar

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

Ship it!

Ship It!

- David


On August 22nd, 2012, 8:24 a.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 22, 2012, 8:24 a.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff

Commit Hook | 4 Sep 2012 15:29
Picon
Favicon

Re: Review Request: Show warning when CopyJob fails to list a subdir

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106052/

This review has been submitted with commit a1b525d29af172e2c983643ee525bf69f541f34c by Dan Vrátil to branch KDE/4.9.

- Commit


On August 22nd, 2012, 8:24 a.m., Dan Vratil wrote:

Review request for kdelibs and David Faure.
By Dan Vratil.

Updated Aug. 22, 2012, 8:24 a.m.

Description

Display a warning when CopyJob fails to enter a subdirectory and thus can't copy it's content.

Diffs

  • kdeui/jobs/kdialogjobuidelegate.cpp (fe48f87)
  • kio/kio/copyjob.h (eb88c7a)
  • kio/kio/copyjob.cpp (8dde763)
  • kio/kio/job.cpp (a7e1baf)
  • kio/kio/jobclasses.h (de27f40)

View Diff


Gmane