Warn user before copy/move operation if available space is not enough
ClosedPublic

Authored by shubham on Aug 12 2018, 7:11 AM.

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
shubham created this revision.Aug 12 2018, 7:11 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 12 2018, 7:11 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
shubham requested review of this revision.Aug 12 2018, 7:11 AM
pino requested changes to this revision.Aug 12 2018, 8:41 AM
pino added a subscriber: pino.

Also, considering this is in a job in KIOCore (i.e. non-gui library), I suspect that using a message box directly is the wrong way to do it. Most probably you need to use the UI delegate of the job for this.

src/core/CMakeLists.txt
146

This is a private dependency, so it must go to the PRIVATE section of target_link_libraries.

src/core/copyjob.cpp
66

Unneeded change.

890

The return value of KMessageBox::warningYesNo is ignored, so this will continue regardless of the user choice.
Also, the message itself is not informative enough: what are the space needed, and the space available? And which directory for?

This revision now requires changes to proceed.Aug 12 2018, 8:41 AM
In D14757#306938, @pino wrote:

Also, considering this is in a job in KIOCore (i.e. non-gui library), I suspect that using a message box directly is the wrong way to do it. Most probably you need to use the UI delegate of the job for this.

you mean to use KIO/JobUiDelegate ?

dfaure added a subscriber: dfaure.Aug 12 2018, 10:07 AM

You can just emit the warning signal, from the job.

shubham added a comment.EditedAug 12 2018, 10:15 AM
In D14757#306938, @pino wrote:

Also, considering this is in a job in KIOCore (i.e. non-gui library), I suspect that using a message box directly is the wrong way to do it.

@dfaure what is your opinion on that?

Ah, hmm, my suggestion would allow to display a warning messagebox, but not to offer "continue/cancel" type of interaction.

You can use SimpleJobPrivate::requestMessageBox instead, with the type argument set to JobUiDelegateExtension::WarningContinueCancel, instead.

shubham added a comment.EditedAug 12 2018, 11:10 AM

@dfaure is it that message boxes cant be used in non gui library as suggested by pino. Is there any harm in using kmessagebox ?

Yes, Pino is right. Trust Pino ;)

shubham edited reviewers, added: dfaure; removed: broulik, ngraham.Aug 12 2018, 12:27 PM
shubham updated this revision to Diff 39531.Aug 12 2018, 4:46 PM

use simpleJobPrivate

pino requested changes to this revision.Aug 12 2018, 6:04 PM

Hint: please make sure you build and test the next versions of your patches, otherwise it is just a waste of everybody's time.

src/core/copyjob.cpp
891

Uninitialized pointer, this will crash two lines later...
Also, this is the base class of the private class used for this job, and this function is part of that class already; so why aren't you just invoking it?

893

The order of the parameters does not match at all the actual parameters of the function.
Also, all the text/caption strings must be translated.

899

Assuming m_dest is a local file, then toLocalFile() is the right function to call (path() will give a different result on Windows).

909

The return value is JobUiDelegateExtension::MessageBoxType, not KMessageBox::ButtonCode.

This revision now requires changes to proceed.Aug 12 2018, 6:04 PM
dfaure requested changes to this revision.Aug 13 2018, 1:03 PM
dfaure added inline comments.
src/core/CMakeLists.txt
153

Please remove

src/core/copyjob.cpp
49

Please remove

892

join with next line, this isn't 1980-style C code ;)

shubham added a comment.EditedAug 13 2018, 2:44 PM

A question ....requestMessageBox( ) is a "pure virtual" function inside "jobUiDelegateExtension" class which is implemented in "jobUiDelegate" class. When I resolve requestMessageBox() using : : like this (jobUiDelegate::), I get an error saying jobUiDelegate has not been declared , to correct the problem when I #include <jobUiDelegate.h> it shows unknown file/directory. what's actually the problem?

jobUiDelegate.h actually exists in inside widgets
when I #include <../widgets/jobuidelegate.h> it shows an error saying fatal error: QtGui/QDialog: No such file or directory

shubham added inline comments.Aug 18 2018, 3:33 PM
src/core/copyjob.cpp
909

There is no enum for Button Code inside JobUiDelegatextension. So how to take that into consideration?

shubham added inline comments.Sep 2 2018, 2:48 PM
src/core/copyjob.cpp
891

which class you are talking about? requestMessageBox() exists only in simpleJobPrivate, jobUiDelegate and jobUiDelegateExtension

shubham updated this revision to Diff 41655.EditedSep 14 2018, 4:10 PM
shubham edited reviewers, added: broulik; removed: cfeck.

Done above requested changes.
But can I get help on my inline comments?

shubham retitled this revision from Warn user before copy operation if available space is not enough to Warn user before copy/move operation if available space is not enough.Sep 14 2018, 4:22 PM
shubham marked 3 inline comments as done.Sep 15 2018, 3:39 AM
shubham edited the summary of this revision. (Show Details)Sep 15 2018, 4:10 AM
ngraham added a subscriber: ngraham.EditedSep 15 2018, 3:20 PM

We don't actually need to create a messagebox because it's up to the caller to display the error in an appropriate manner. For example, Dolphin shows errors inline rather than with dialog boxes. So we shouldn't create a messagebox at all.

Let's see if we can figure out what to do:

Elsewhere in src/core/copyjob.cpp, in CopyJobPrivate::copyNextFile(), we already check for the size of each individual file:

if (m_freeSpace < (*it).size) {
    q->setError(ERR_DISK_FULL);
    q->emitResult();
    return;
}

This works, but results in a half-finished copy, as it dies once it encounters the first file that doesn't fit. In CopyJobPrivate::statCurrentSrc(), the comment //TODO warn user beforehand if space is not enough gives us a clue for what to do: just add the same logic there, but check the total size of all copied files rather than the size of each individual file. So you would add the following:

if (m_totalSize > m_freeSpace) {
    q->setError(ERR_DISK_FULL);
    q->emitResult();
    return;
}

For bonus points, set the error text to something appropriate for each error. For example, something like this would work for the "whole transfer is too big" case:

q->setErrorText(
        xi18nc("@info", "There will not be enough free space available at <filename>%1</filename> to hold the file (%2 are required but only %3 are available",
            m_globalDest.toLocalFile(),
            KIO::convertSize(m_totalSize),
            KIO::convertSize(m_freeSpace) ) );
src/core/copyjob.cpp
887

You can remove this comment now. :)

shubham updated this revision to Diff 41746.Sep 16 2018, 4:10 PM
shubham marked 3 inline comments as done.Sep 16 2018, 4:13 PM
shubham marked an inline comment as done.

Looking better, thanks! I will test shortly. I have one string change suggestion; can we change it to:

There is not enough space available at <filename>%1</filename> to hold the file.<nl/><nl/>%2 are required but only %3 are available.
shubham updated this revision to Diff 41747.Sep 16 2018, 4:23 PM
ngraham requested changes to this revision.Sep 16 2018, 5:01 PM

You forgot the error line. :) It needs q->setError(ERR_DISK_FULL); before setErrorText().

This revision now requires changes to proceed.Sep 16 2018, 5:01 PM
shubham updated this revision to Diff 41751.Sep 16 2018, 5:04 PM

Yay, it works now! If I try to copy a folder that is too big, it stops immediately before anything happens, and the following is displayed in Dolphin:

The weird display is probably a bug in Dolphin with how it builds up the string to display to the user.

Now that I see it in action after trying to copy a folder, I think there's one more string change we could make: if the source is a folder, instead display, "There is not enough space available at <filename>%1</filename> to hold the folder." It should only use the word "file" if the source is actually a file. Do you think you could make that change?

shubham updated this revision to Diff 41761.Sep 16 2018, 5:43 PM

done above requested change

Perfect, thanks! I did some investigation and this oddly-formed string is actually built by KIO itself, in core/job_error.cpp:

case KIO::ERR_DISK_FULL:
    result = i18n("Could not write file %1.\nDisk full.",  errorText);
    break;

It looks like KIO::ERR_DISK_FULL is expecting errorText to simply be a filename or path, and our fancier string isn't compatible with its expectations. :( It looks like in this patch, we should pass the source file/directory to q->setErrorText() without any fancy strings, to follow the API.

However, in another patch I'd like to investigate adjusting the formatting in core/job_error.cpp to support fancy error messages (in particular information about required and available space, which I think is a usability improvement over just saying "disk full"), and then change all the error strings accordingly . Nearly all uses are in KIO, but one is in kio-gdrive, one is in kio-extras, and one is in plasma-framework. @what are your thoughts on this, @dfaure, @pino, or anyone else from Frameworks?

shubham updated this revision to Diff 41764.Sep 16 2018, 6:19 PM
broulik added inline comments.Sep 16 2018, 6:23 PM
src/core/copyjob.cpp
887

Please check for m_freeSpace != static_cast<KIO::filesize_t>(-1) (KIO::filesize_t is unsigned) to avoid false errors when free space couldn't be determined

889

Can you assume m_currentScrURL is a local file? Perhaps use toDisplayString

shubham updated this revision to Diff 41765.Sep 16 2018, 6:35 PM

addressed broulik's comments

shubham marked 2 inline comments as done.Sep 16 2018, 6:36 PM
ngraham accepted this revision.Sep 16 2018, 6:51 PM

That's better:

FWIW, I've submitted a patch to slightly improve the error message: D15557

@pino and @dfaure, are you good with this now?

This revision was not accepted when it landed; it landed in state Needs Review.Sep 17 2018, 4:34 AM
This revision was automatically updated to reflect the committed changes.

@shubham, in the future, when there are still reviewers who have marked their status as "Request Changes", please wait for them to approve before pushing.

Since that didn't happen this time, if @pino or @dfaure have any remaining concerns with this patch, please submit a follow-up patch to address their feedback.

@ngraham I thought those "request changes" were against my prior diffs

Yes, but they still hadn't approved the current version. :)

Sorry for that, will take care of that in future.

Looks OK now.