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