BUG: 243160
Details
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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 ↗ | (On Diff #39492) | 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. |
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.
@dfaure is it that message boxes cant be used in non gui library as suggested by pino. Is there any harm in using kmessagebox ?
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... | |
893 | The order of the parameters does not match at all the actual parameters of the function. | |
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. |
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
src/core/copyjob.cpp | ||
---|---|---|
909 | There is no enum for Button Code inside JobUiDelegatextension. So how to take that into consideration? |
src/core/copyjob.cpp | ||
---|---|---|
891 | which class you are talking about? requestMessageBox() exists only in simpleJobPrivate, jobUiDelegate and jobUiDelegateExtension |
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. :) |
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.
You forgot the error line. :) It needs q->setError(ERR_DISK_FULL); before setErrorText().
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?
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, 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.