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

[David Faure]

dfaure added a comment.


  Looks OK now.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:5a48bd21212c: Warn user before copy/move operation if 
available space is not enough (authored by shubham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41765=41792

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  That's better:
  
  F6268143: Better.png 
  
  FWIW, I've submitted a patch to slightly improve the error message: D15557 

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41765.
shubham added a comment.


  addressed broulik's comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41764=41765

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> copyjob.cpp:887
>  qCDebug(KIO_COPYJOB_DEBUG)<<"Stating finished. To 
> copy:"< -//TODO warn user beforehand if space is not enough
> +if (m_totalSize > m_freeSpace) {
> +q->setError(ERR_DISK_FULL);

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

> copyjob.cpp:889
> +q->setError(ERR_DISK_FULL);
> +q->setErrorText(m_currentSrcURL.toLocalFile());
> +q->emitResult();

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41764.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41761=41764

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  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 
?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41761.
shubham added a comment.


  done above requested change

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41751=41761

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  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:
  
  F6268053: It works, mostly.png 
  
  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 %1 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41751.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41747=41751

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik, ngraham
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41747.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41746=41747

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  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 %1 to hold the 
file.%2 are required but only %3 are available.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham updated this revision to Diff 41746.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14757?vs=41655=41746

REVISION DETAIL
  https://phabricator.kde.org/D14757

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  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(
xi18n("There will not be enough free space available at 
%1 to hold the file (%2 are required but only %3 are 
available",
m_globalDest.toLocalFile(),
KIO::convertSize(m_totalSize),
KIO::convertSize(m_freeSpace) ) );

INLINE COMMENTS

> copyjob.cpp:887
>  qCDebug(KIO_COPYJOB_DEBUG)<<"Stating finished. To 
> copy:"<  //TODO warn user beforehand if space is not enough
> +if (m_totalSize > m_freeSpace) {

You can remove this comment now. :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: ngraham, dfaure, pino, kde-frameworks-devel, michaelh, bruns

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

[Shubham]

shubham edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: dfaure, pino, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

shubham marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: dfaure, pino, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14757

To: shubham, pino, dfaure, broulik
Cc: dfaure, pino, kde-frameworks-devel, michaelh, ngraham, bruns