Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 21, 2013, 6:05 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/#review36244 --- Ship it! Ah, now I understand. Before hitting Undo, the user could change the clipboard, and then if it's totally unrelated, KIO shouldn't touch it. Got it. Thanks for your patience :-) - David Faure On July 21, 2013, 6:05 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 21, 2013, 6:05 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 21, 2013, 6:32 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/#review36267 --- This review has been submitted with commit e4b786f6df87aba226931082e5ad2791ac529978 by Dawit Alemayehu to branch KDE/4.11. - Commit Hook On July 21, 2013, 6:05 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 21, 2013, 6:05 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 20, 2013, 2:06 p.m.) Review request for kdelibs and David Faure. Changes --- Uploaded the correct patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/clipboardupdater.cpp PRE-CREATION kio/kio/clipboardupdater_p.h PRE-CREATION kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/#review36226
---
kio/kio/clipboardupdater.cpp
http://git.reviewboard.kde.org/r/111585/#comment26809
remove default: section (to catch missing impl of a future value in the
enum)
kio/kio/clipboardupdater_p.h
http://git.reviewboard.kde.org/r/111585/#comment26810
I'm confused. If the user presses Undo, the whole copy/move operation is
undone, not just one URL out of many URLs, right?
So I don't see why OverwriteContent doesn't work too, for the undo case?
You cut three files A,B,C from dir1, and paste them into dir2.
OverwriteContent updates the clipboard to dir2/{A,B,C}. If you then undo, the 3
files are moved from dir2 back to dir1, and therefore OverwriteContent can be
used to update the clipboard from dir2/{A,B,C} to dir1/{A,B,C}. What am I
missing?
Sounds to me like UpdateContent is there for the purpose of the other cases
you mention in the description (e.g. #134960), but that's not tested yet.
- David Faure
On July 20, 2013, 2:06 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/
---
(Updated July 20, 2013, 2:06 p.m.)
Review request for kdelibs and David Faure.
Description
---
The attached patch fixes a bug where the contents of the clipboard are
prematurely updated during a cut and paste operation. In the process I also
discovered that undoing the operation does not update the clipboard either.
Hence that too is fixed by this patch.
Please note that this patch does not address all the cases where the content
of the clipboard is not updated after a KIO operation. More specifically the
clipboard content will be out of sync if the user performs the following
operations:
- copy/cut a file or a directory and rename it
- copy/cut a file or a directory and move it
- copy/cut a file or a directory and delete it.
In fact there is a ticket for the copy/cut and rename file/directory scenario
(bug# 134960). However, addressing these issues require a careful
consideration of how to do it since delete/rename/move operations can be done
outside of KDE's control. Do we simply fix the KIO jobs to handle this or do
we address it the KDirWatch level so we catch all the scenarios? Probably the
latter. Anyhow, that can wait until for the 134960 fix.
This addresses bug 318757.
http://bugs.kde.org/show_bug.cgi?id=318757
Diffs
-
kio/CMakeLists.txt f7a3767
kio/kio/clipboardupdater.cpp PRE-CREATION
kio/kio/clipboardupdater_p.h PRE-CREATION
kio/kio/fileundomanager.cpp 9f76fef
kio/kio/paste.cpp ca451fb
kio/tests/fileundomanagertest.h ebd02fa
kio/tests/fileundomanagertest.cpp 7c1352c
Diff: http://git.reviewboard.kde.org/r/111585/diff/
Testing
---
Unit and manual tests.
Thanks,
Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
On July 20, 2013, 9:46 p.m., David Faure wrote:
kio/kio/clipboardupdater_p.h, line 34
http://git.reviewboard.kde.org/r/111585/diff/3/?file=172590#file172590line34
I'm confused. If the user presses Undo, the whole copy/move operation
is undone, not just one URL out of many URLs, right?
So I don't see why OverwriteContent doesn't work too, for the undo case?
You cut three files A,B,C from dir1, and paste them into dir2.
OverwriteContent updates the clipboard to dir2/{A,B,C}. If you then undo,
the 3 files are moved from dir2 back to dir1, and therefore
OverwriteContent can be used to update the clipboard from dir2/{A,B,C} to
dir1/{A,B,C}. What am I missing?
Sounds to me like UpdateContent is there for the purpose of the other
cases you mention in the description (e.g. #134960), but that's not tested
yet.
Well I should have probably used a better example or elaborated more in the
documentation. The update mode is the one that should be chosen unless you are
100% certain the user did perform a cut/copy and paste operation. Currently,
the only place I know where that is the case is when KIO::pasteClipboard is
called for a move (cut+paste) operation. Otherwise, you should always use the
update mode because you do not want to inadvertently overwrite the contents of
the clipboard. I will see if I can clarify the documentation a little bit.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/#review36226
---
On July 20, 2013, 2:06 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111585/
---
(Updated July 20, 2013, 2:06 p.m.)
Review request for kdelibs and David Faure.
Description
---
The attached patch fixes a bug where the contents of the clipboard are
prematurely updated during a cut and paste operation. In the process I also
discovered that undoing the operation does not update the clipboard either.
Hence that too is fixed by this patch.
Please note that this patch does not address all the cases where the content
of the clipboard is not updated after a KIO operation. More specifically the
clipboard content will be out of sync if the user performs the following
operations:
- copy/cut a file or a directory and rename it
- copy/cut a file or a directory and move it
- copy/cut a file or a directory and delete it.
In fact there is a ticket for the copy/cut and rename file/directory scenario
(bug# 134960). However, addressing these issues require a careful
consideration of how to do it since delete/rename/move operations can be done
outside of KDE's control. Do we simply fix the KIO jobs to handle this or do
we address it the KDirWatch level so we catch all the scenarios? Probably the
latter. Anyhow, that can wait until for the 134960 fix.
This addresses bug 318757.
http://bugs.kde.org/show_bug.cgi?id=318757
Diffs
-
kio/CMakeLists.txt f7a3767
kio/kio/clipboardupdater.cpp PRE-CREATION
kio/kio/clipboardupdater_p.h PRE-CREATION
kio/kio/fileundomanager.cpp 9f76fef
kio/kio/paste.cpp ca451fb
kio/tests/fileundomanagertest.h ebd02fa
kio/tests/fileundomanagertest.cpp 7c1352c
Diff: http://git.reviewboard.kde.org/r/111585/diff/
Testing
---
Unit and manual tests.
Thanks,
Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/#review36168 --- You mention running tests, but it doesn't really count since no test is testing this clipboard stuff at the moment :-) kio/kio/updateclipboard.cpp http://git.reviewboard.kde.org/r/111585/#comment26774 If job can be NULL, then new UpdateClipboard(job) will leak the instance. If it can't be NULL, then better Q_ASSERT(job); than if(job). kio/kio/updateclipboard_p.h http://git.reviewboard.kde.org/r/111585/#comment26773 The class name is a verb, this is a bit unusual. Maybe call it ClipboardUpdater, rather? Could you document what the class does? OverwriteContent replaces source urls with destination urls after a copy job is done. What does UpdateContent do? What about the case where CopyJob needed some user intervention? E.g. if you get an overwrite/rename dialog, you can choose a different destination than initially planned. Does this update the desturl, i.e. the clipboard updater will do the right thing? kio/kio/updateclipboard_p.h http://git.reviewboard.kde.org/r/111585/#comment26775 I'd remove the default value, to make the mode explicit in the calling code (unless there's a good reason for one mode to be default, but I don't know what UpdateContent does yet :). - David Faure On July 19, 2013, 1:17 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 19, 2013, 1:17 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 19, 2013, 11:56 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs (updated) - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION kio/tests/fileundomanagertest.h ebd02fa kio/tests/fileundomanagertest.cpp 7c1352c Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
Re: Review Request 111585: Don't update clipboard before cut/paste KIO operation succeeds
On July 19, 2013, 6:25 p.m., David Faure wrote: You mention running tests, but it doesn't really count since no test is testing this clipboard stuff at the moment :-) Sorry. I attached an earlier version of the patch by mistake. :( I will attach the right one after making the changes you suggested. On July 19, 2013, 6:25 p.m., David Faure wrote: kio/kio/updateclipboard_p.h, line 30 http://git.reviewboard.kde.org/r/111585/diff/1/?file=172221#file172221line30 The class name is a verb, this is a bit unusual. Maybe call it ClipboardUpdater, rather? Could you document what the class does? OverwriteContent replaces source urls with destination urls after a copy job is done. What does UpdateContent do? What about the case where CopyJob needed some user intervention? E.g. if you get an overwrite/rename dialog, you can choose a different destination than initially planned. Does this update the desturl, i.e. the clipboard updater will do the right thing? UpdateContent preserves the urls in the clipboard and only updates those that changed where as OverwriteContent simply overwrites what is in the clipboard with a new set of urls. Anyhow, I have added documentation with specific use case examples and simplified the code a bit to make even more clearer. As far as the updater doing the right thing when a user intervention is required, it will do the right if the job itself gets updated with the changed destination url. Since we connect to the result signal and carry out the change only upon successful completion, the changes always reflect the state of the job itself when the result signal is emitted. Is it an incorrect assumption that the job will reflect the changed destination url? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/#review36168 --- On July 19, 2013, 1:17 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111585/ --- (Updated July 19, 2013, 1:17 p.m.) Review request for kdelibs and David Faure. Description --- The attached patch fixes a bug where the contents of the clipboard are prematurely updated during a cut and paste operation. In the process I also discovered that undoing the operation does not update the clipboard either. Hence that too is fixed by this patch. Please note that this patch does not address all the cases where the content of the clipboard is not updated after a KIO operation. More specifically the clipboard content will be out of sync if the user performs the following operations: - copy/cut a file or a directory and rename it - copy/cut a file or a directory and move it - copy/cut a file or a directory and delete it. In fact there is a ticket for the copy/cut and rename file/directory scenario (bug# 134960). However, addressing these issues require a careful consideration of how to do it since delete/rename/move operations can be done outside of KDE's control. Do we simply fix the KIO jobs to handle this or do we address it the KDirWatch level so we catch all the scenarios? Probably the latter. Anyhow, that can wait until for the 134960 fix. This addresses bug 318757. http://bugs.kde.org/show_bug.cgi?id=318757 Diffs - kio/CMakeLists.txt f7a3767 kio/kio/fileundomanager.cpp 9f76fef kio/kio/paste.cpp ca451fb kio/kio/updateclipboard.cpp PRE-CREATION kio/kio/updateclipboard_p.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111585/diff/ Testing --- Unit and manual tests. Thanks, Dawit Alemayehu
