Review Request 111776: Update URLs copied to clipboard when they change due to KIO operations

[David Faure]

> On Aug. 2, 2013, 12:53 p.m., David Faure wrote:
> > kio/kio/clipboardupdater.cpp, line 108
> > <http://git.reviewboard.kde.org/r/111776/diff/2/?file=175012#file175012line108>
> >
> >     The name sounds like it will clear the clipboard.
> >     
> >     removeUrlsFromClipboard would be better
> 
> Dawit Alemayehu wrote:
>     Well this followed the naming convention used for the other statuc functions. If this one should be renamed, then perhaps it would make sense to rename the others as well, no? Perhaps overwriteUrlsInClipboard and updatedUrlsInClipboard? That way they the naming convention used will be consistent.
> 
> Dawit Alemayehu wrote:
>     Since I have no objection this this change I went ahead and changed the names of all the static functions to
>     
>     overwriteUrlsInClipboard
>     updateUrlsInClipboard
>     removeUrlsFromClipboard
>     
>     I think those names sounds closer to what the functions themselves do.

Sounds good.


> On Aug. 2, 2013, 12:53 p.m., David Faure wrote:
> > kio/kio/copyjob.cpp, line 2190
> > <http://git.reviewboard.kde.org/r/111776/diff/2/?file=175014#file175014line2190>
> >
> >     Where's this instance deleted?
> >     
> >     Should there be a deleteLater() in all code paths of slotResult()?
> 
> Dawit Alemayehu wrote:
>     It should be automatically deleted when the job is gone because ClipboardUpdater sets the job as its parent.

Ah doh, I think I got surprised by this twice already (i.e. you told me once before already).

So either I'm stupid (senile?) or this needs more explicit documentation. Maybe in the docs for the ClipboardUpdater constructor. If there was a QObject* parent it would be obvious, but since it's a KIO::Job* job parameter, it's not that obvious that it will become the parent object. The docu should say so.

Thanks.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111776/#review36984
-----------------------------------------------------------


On Aug. 2, 2013, 12:37 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111776/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 12:37 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Description
> -------
> 
> This patch is an improvement of https://git.reviewboard.kde.org/r/111585/ such that KIO operations also update URLs in the clipboard. As such, all KIO operations that rename, move or delete a file will always update the contents of the clipboard. 
> 
> A couple of notes about this patch:
> 
> - KIO::trash was left out from this patch because I am unsure whether it should be treated like a delete or move operation. 
> - Move, rename and delete operations performed outside of KIO are not covered by this patch and as such will not update URLs in the clipboard. Dealing with non KIO modifications is outside the scope of this patch since it needs to be dealt with outside of KIO.
> 
> 
> This addresses bug 134960.
>     http://bugs.kde.org/show_bug.cgi?id=134960
> 
> 
> Diffs
> -----
> 
>   kio/kio/clipboardupdater.cpp 8ab9210 
>   kio/kio/clipboardupdater_p.h b07c320 
>   kio/kio/copyjob.cpp da19de5 
>   kio/kio/deletejob.cpp 7178424 
>   kio/kio/job.cpp 05d0ba2 
>   kio/kio/paste.cpp b4372ab 
>   kio/tests/CMakeLists.txt b570aac 
>   kio/tests/clipboardupdatertest.h PRE-CREATION 
>   kio/tests/clipboardupdatertest.cpp PRE-CREATION 
>   kio/tests/fileundomanagertest.h e909bb7 
>   kio/tests/fileundomanagertest.cpp 709938d 
> 
> Diff: http://git.reviewboard.kde.org/r/111776/diff/
> 
> 
> Testing
> -------
> 
> Unite tests.
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130802/bf013a9d/attachment.htm>