D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-06-28 Thread Dmitri Ovodok
dmitrio abandoned this revision.
dmitrio added a comment.


  No, didn't work on that, sorry. I think I am better to close the revision.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, 
#frameworks, michaelh, bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-06-27 Thread Nathaniel Graham
ngraham added a comment.


  Any progress on that, dmitrio?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, 
#frameworks, michaelh, bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-19 Thread Nathaniel Graham
ngraham added a comment.


  Right, I think that makes sense.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, 
#frameworks, michaelh, bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-19 Thread Dmitri Ovodok
dmitrio added a comment.


  Well, I think it would be logical to implement something like that if we 
intend to make copying cancelable. But we still will have to spoil the 
destination file if there is no enough space left (which is not always 
determined correctly, in fact. One example of this is a remote sftp filesystem 
with a per-user quota set up on a remote machine). So the idea sounds good, but 
it requires careful implementation and adds a bit of inconsistency to the 
program behavior. But perhaps in this case it doesn't matter — a user anyway 
gets a warning when trying to overwrite files.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, 
#frameworks, michaelh, bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-11 Thread Nathaniel Graham
ngraham added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  You're right, the destination file was indeed silently corrupted in the 
"before" case. Yikes, that's bad. Really, both options are bad. I can see now 
that your patch doesn't actually regress anything--it just exposes a dormant 
problem that was previously hidden but becomes very visible with the patch. 
That's a good sign that we ought to resolve that issue first. Perhaps for 
overwrites, if there's enough space on the destination filesystem, we should 
copy to a temp file, then if that completes, swap the target file and the temp 
file, and then finally delete the old target file. What do you think?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, 
#frameworks, michaelh, bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-01 Thread Dmitri Ovodok
dmitrio added a comment.


  If I understand correctly what has happened, without this patch the file at 
destination path existed after the operation was cancelled, and it might even 
not be corrupted. Still I believe that it is the **source** file that may 
remain after the cancellation, but not the one existed at the destination path. 
Try the same test with different files (I mean, with different content) at the 
source and destination paths. After cancelling an overwrite operation you will 
have either a copy of the source file or a corrupted file if the source file 
was sufficiently large (more than 1 GB works in my case, but this probably 
depends on memory size and disk I/O speed).
  
  I agree though that disappearing of the destination file may be confusing to 
user, so some special handling of this case may be useful. But currently I do 
not see any sign of this case being already handled by KIO, neither in the 
source code nor in real tests.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-01 Thread Anthony Fieroni
anthonyfieroni added inline comments.

INLINE COMMENTS

> filecopyjob.cpp:320
> +// It is better to clean up this file.
> +if (!isSuspended() && d->m_bFileWritingIsInProcess) {
> +KIO::file_delete(d->m_dest);

&& metadata(QStringLiteral("overwrite")) != QLatin1String("true")

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-01 Thread Nathaniel Graham
ngraham added a comment.


  Here, let me show you.
  
  Without your patch:
  F5829979: without the patch.webm 
  
  With your patch:
  F5829981: with the patch.webm 

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-05-01 Thread Dmitri Ovodok
dmitrio added a comment.


  Sorry, but I was not able to locate such a behavior. When cancelling an 
overwrite in the middle of the process I got only a partially copied file at 
the destination path. Tested in the latest git-stable KDE Neon, the procedure 
was a copying of the source file to the other folder containing the file with 
the same name. Perhaps you told about some other kind of an overwrite operation?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-26 Thread Nathaniel Graham
ngraham added a comment.


  In D10663#254549 , @dmitrio wrote:
  
  > Then I probably didn't understand what was proposed in the discussed bug 
report. When you start writing to the existing file the data that have been in 
the destination file before the operation become lost. If we want to be able to 
restore content of destination files after overwrite operation has started then 
we need to change the whole mechanism of file copying: we should copy source 
file to some temporary file, delete the destination file an rename temporary 
file to the destination file name.
  
  
  That's what it already does. Without your patch, canceling an overwrite is 
harmless because only the temporary file is deleted. In other words, the case 
in the bug is already handled for overwrite operations.
  
  The case that is NOT handled is for normal move/copy operations, where the 
destination file is not cleaned up after the job is canceled in the middle. 
Essentially, we want your patch to only delete the destination file when it's 
not an overwrite, because overwrites already clean up properly.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-26 Thread Dmitri Ovodok
dmitrio added a comment.


  Then I probably didn't understand what was proposed in the discussed bug 
report. When you start writing to the existing file the data that have been in 
the destination file before the operation become lost. If we want to be able to 
restore content of destination files after overwrite operation has started then 
we need to change the whole mechanism of file copying: we should copy source 
file to some temporary file, delete the destination file an rename temporary 
file to the destination file name. If this is what was meant by this bug report 
then I am probably trying to fix the wrong issue. Is it so, or what is the 
desired behavior in this case?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-26 Thread Nathaniel Graham
ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  When you cancel an overwrite operation in the middle with this patch, the 
destination file is destroyed.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, ngraham
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-25 Thread Dmitri Ovodok
dmitrio added a comment.


  Yes, this code is ready for review.
  As I described in the previous comment, there is still a question concerning 
handling the full disk error case: it is being handled currently in some 
ioslaves (at least in `file_unix` 

 slave), and it needs to be decided whether such handling should be moved from 
the slaves code to `FileCopyJob` level or it should be left as is. But I would 
anyway ask for developers' opinion on this prior to making such changes.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-25 Thread Nathaniel Graham
ngraham added a comment.


  @dmitrio are you ready for review, or still working on this?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-04-17 Thread Nathaniel Graham
ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, 
bruns


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-03-25 Thread Dmitri Ovodok
dmitrio updated this revision to Diff 30570.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10663?vs=27582=30570

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

AFFECTED FILES
  src/core/filecopyjob.cpp
  src/core/filecopyjob.h

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-03-25 Thread Dmitri Ovodok
dmitrio reclaimed this revision.
dmitrio added a comment.
This revision now requires changes to proceed.


  Well, sorry, I did not expect you waiting for me. I had some new version of 
this patch, but thought about working on unit tests for it and never finished 
them. I'll upload now what I have to date, hope it will be useful.
  
  Concerning the `ERR_DISK_FULL` case which was mentioned earlier, it is easy 
to add some handling for this case in the new version of patch . However, I 
should note that at least file ioslave for Unix-like systems also tries to 
remove partially copied file in case of full disk (see e.g. this 
).
 Not sure how it would interfere with more high-level treatment in 
`FileCopyJob` and whether we should keep or remove that code in case we decide 
to handle this case in `FileCopyJob`.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-03-25 Thread Nathaniel Graham
ngraham added a comment.


  @dmitrio, are you still planning to work on this patch, or a new version of 
it?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-27 Thread Elvis Angelaccio
elvisangelaccio added a comment.


  @dmitrio: you can click on the "Plan Changes" action, no need to abandon this 
revision and create a new one.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-27 Thread David Faure
dfaure added a comment.


  "the destination file gets deleted only if some data has been written into 
it" ... ah I see, that's a great solution indeed, I like it.
  
  About moving: if the file was fully copied, (but the source not yet deleted), 
good catch. This requires a similar solution in FileCopyJob, i.e. one layer 
below (with a similar "in progress" flag that is only true while the file copy 
is in progress, and false as soon as it's done, i.e. before the DeleteJob). I 
suggest starting with that, and then in CopyJob when killing a FileCopyJob we 
just let it do the cleanup. As you found out, CopyJob is too high-level for 
this.
  
  Pausing and doing nasty things will never be safe, I don't think we need to 
worry about that.
  
  I think all you have to do is go one more step down the stack :-)

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-26 Thread Dmitri Ovodok
dmitrio abandoned this revision.
dmitrio added a comment.


  In D10663#213898 , @dfaure wrote:
  
  > I don't see any provision for the case I mentioned, where the destination 
file already exists, and should therefore NOT be deleted?
  
  
  In fact, this is exactly the case that I tried to deal with here. In this 
proposal the destination file gets deleted only if some data has been written 
into it, so all the previous data are already lost regardless of our cleanup.
  
  What is not handled here is the case of moving to another partition, where 
subjob may launch original file deletion just after successful file copying and 
before emitting result, which may lead to cleanup being done when the original 
file can potentially be deleted. I also expect some trouble with job pause 
feature when user may pause this job, eventually start copying the same file 
with another tool (be it something like `cp` or another instance of CopyJob), 
and then abort this job. For this case we should probably add something like a 
check on whether size of the destination file has changed since our 
last change of that file. So it seems that to work properly this feature needs 
some more complex solution including, probably, some separate job class dealing 
with cleanup and all the necessary safety checks. For now, I can only apologize 
for underestimating the problem and rolling out such a raw solution for it.
  
  I am probably better to close it, if I am able to come up with better 
solution I will reopen this request (if it is possible) or open a new one.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-25 Thread Nathaniel Graham
ngraham added a comment.


  In D10663#213898 , @dfaure wrote:
  
  > Better make it happen all the time, and better make it work right. A flag 
almost sounds like a excuse for a half-hearted feature ("if it works badly in 
case XYZ, then apps can just opt out"). I don't see why the app would care, 
really. It wants to copy A to B, and wants to know if it worked or not; details 
like cleaning up on cancel are best handled by the KIO library.
  
  
  +1

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-25 Thread David Faure
dfaure requested changes to this revision.
dfaure added a comment.


  I don't like the idea of a flag for this in the API. It just moves the 
problem (of whether it's safe / a good idea to clean up) to the applications, 
who are not in a better place to decide about this. Better make it happen all 
the time, and better make it work right. A flag almost sounds like a excuse for 
a half-hearted feature ("if it works badly in case XYZ, then apps can just opt 
out"). I don't see why the app would care, really. It wants to copy A to B, and 
wants to know if it worked or not; details like cleaning up on cancel are best 
handled by the KIO library.
  
  I don't see any provision for the case I mentioned, where the destination 
file already exists, and should therefore NOT be deleted?
  
  I'm also missing a unittest for this. Now it should be quite easy to 
unittest. Copy a dir with two files, connect to copyingDone() to know when the 
first file was copied, then kill the job in the slot [possibly as a Queued 
invocation]. Dest dir should have only the first file. Then the same when 
connecting to copying(), i.e. before the copy of the first file. Dest dir 
should be empty. And then unittests with existing files at destination.
  
  Thanks for your work on this.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh, kmorwinski


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-22 Thread Dmitri Ovodok
dmitrio planned changes to this revision.
dmitrio added a comment.


  It seems that the proposed patch in its current state does not fully prevent 
possible problems with move operation (did not catch any problems while 
testing, but looking at the code again I don't think that all is good with it). 
I suppose I am better to revise it again.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Nathaniel Graham
ngraham added a comment.


  I would prefer to have the cleanup behavior happen by default, if we end up 
making this optional.
  
  It would be nice if we could also also handle the `KIO::ERR_DISK_FULL` case.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: ngraham, anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Dmitri Ovodok
dmitrio marked an inline comment as done.
dmitrio added a comment.


  In D10663#209708 , @meven wrote:
  
  > Add this to your commit message
  >  FIXED-IN: 5.44
  
  
  Thank you, added it to the description.
  
  In D10663#209711 , @anthonyfieroni 
wrote:
  
  > It looks D10635  is duplicate.
  
  
  In fact, this revision is a duplicate of D10635 
. I am a bit surprised why this one has a 
smaller number :)
  
  In D10663#209720 , @meven wrote:
  
  > It was suggested on IRC to hide this behavior behind a flag such as the 
jobFlag enum, so that it can be opt-in/opt-out in applications.
  
  
  I agree with this proposal, but I can suggest that the added flag should 
rather turn off the new behavior, so the default behavior will be to do this 
cleanup on job cancel.
  However, if more people think that it should not be turned on by default, it 
would be easy to change this diff to achieve an opposite behavior.
  
  In D10663#209720 , @meven wrote:
  
  > We could consider changing the job state during the clean up, something like
  >
  >   d->state == STATE_CLEANING_INCOMPLETE_FILE
  >
  
  
  For now, I see no point in this change, since, to avoid breaking a logic of 
doKill() method, the deletion job is currently launched not as subjob, but as a 
separate job. So, after doKill() execution the CopyJob does nothing anymore (as 
it is supposed to be) and has no need in tracking its state. Feel free to 
correct me if I am wrong.

INLINE COMMENTS

> meven wrote in copyjob.cpp:559
> I believe this is because files are copied in sequence, so at any given time 
> only the first file is being copied.
> 
> Maybe you could use m_currentSrcURL to avoid accessing the iterator.

You are right, updated a diff.

On the question on deleting first file only, yes, we need to delete only the 
file that was being copied before the user canceled the job. Perhaps using 
m_currentDestURL would make this intention more clear.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Dmitri Ovodok
dmitrio updated this revision to Diff 27582.
dmitrio edited the summary of this revision.
dmitrio added a comment.


  Add a flag which turns off cleaning up on job cancel.
  Use m_currentDestURL instead of obtaining the destination file name from an 
iterator.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10663?vs=27546=27582

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/copyjob.h
  src/core/job_base.h

To: dmitrio, #frameworks, dfaure
Cc: anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Méven Car
meven added a comment.


  It was suggested on IRC to hide this behavior behind a flag such as the 
jobFlag enum, so that it can be opt-in/opt-out in applications.
  
  We could consider changing the job state during the clean up, something like
  
d->state == STATE_CLEANING_INCOMPLETE_FILE

INLINE COMMENTS

> anthonyfieroni wrote in copyjob.cpp:559
> Why only first not all?

I believe this is because files are copied in sequence, so at any given time 
only the first file is being copied.

Maybe you could use m_currentSrcURL to avoid accessing the iterator.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure
Cc: anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Anthony Fieroni
anthonyfieroni edited reviewers, added: dfaure; removed: Dolphin.
anthonyfieroni added a comment.


  It looks D10635  is duplicate.

INLINE COMMENTS

> copyjob.cpp:559
> +if (d->state == STATE_COPYING_FILES && d->m_bFileCopyingIsInProcess) {
> +QList::Iterator it = d->files.begin();
> +const QUrl dest = (*it).uDest;

Why only first not all?

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, dfaure, #dolphin
Cc: anthonyfieroni, meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Méven Car
meven added a comment.


  Add this to your commit message
  FIXED-IN: 5.44

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, #dolphin
Cc: meven, #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Nathaniel Graham
ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, #dolphin
Cc: #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Nathaniel Graham
ngraham added reviewers: Frameworks, Dolphin.

REPOSITORY
  R241 KIO

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

To: dmitrio, #frameworks, #dolphin
Cc: #frameworks, michaelh


D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

2018-02-19 Thread Dmitri Ovodok
dmitrio created this revision.
dmitrio added a project: Frameworks.
dmitrio requested review of this revision.

REVISION SUMMARY
  BUG: 383764
  
  Remove a partially copied file if copyjob was cancelled in the middle of file 
copying.
  
  File is considered to be in the process of being copied after some data block 
is actually written (or, to be more precise, slotProcessedSize is called). This 
should help us avoid cleaning up by mistake files that existed before the 
operation. This also means that in some very rare occasions when user cancels 
copying in the very beginning of the operation cleaning up procedure may not 
work — which is, I believe, the best result we can obtain without more 
significant code changes.
  
  Unlike this proposal , my option does not 
include handling of the full disk or any other errors. In its current state it 
applies only to the case when job gets killed by user.

TEST PLAN
  While I am not sure about automated testing, various manual testing scenarios 
can be suggested:
  
  I.
  
  1. Start copying a large file(s) to  empty directory
  2. Cancel copying
  3. File which copying was cancelled should **not** be present at the 
destination.
  
  II.
  
  1. Start copying a large file(s) to the directory with existing files with 
colliding names
  2. Cancel copying (not via "File exists" dialog)
  3. Original file should be present at the destination
  
  One may also want to test behavior in situation when it is not possible to 
delete the file.
  For example:
  
  1. Start copying a file to some network folder
  2. Turn off network connection
  3. Cancel copying
  4. File deletion UI should appear in the notifications area. You should now 
be able to cancel deletion or wait a bit for an error message.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/copyjob.h

To: dmitrio
Cc: #frameworks, michaelh