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