<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111776/">http://git.reviewboard.kde.org/r/111776/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 2nd, 2013, 12:53 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111776/diff/2/?file=175012#file175012line108" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/clipboardupdater.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">103</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">void</span> <span class="n">removeClipboardContent</span><span class="p">(</span><span class="n">KJob</span><span class="o">*</span> <span class="n">job</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The name sounds like it will clear the clipboard.
removeUrlsFromClipboard would be better</pre>
</blockquote>
<p>On August 2nd, 2013, 1:23 p.m. UTC, <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 2nd, 2013, 1:28 p.m. UTC, <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sounds good.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 2nd, 2013, 12:53 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111776/diff/2/?file=175014#file175014line2190" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/copyjob.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">CopyJob *KIO::copy( const KUrl::List& src, const KUrl& dest, JobFlags flags )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2190</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">new</span> <span class="nf">ClipboardUpdater</span><span class="p">(</span><span class="n">job</span><span class="p">,</span> <span class="n">ClipboardUpdater</span><span class="o">::</span><span class="n">UpdateContent</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Where's this instance deleted?
Should there be a deleteLater() in all code paths of slotResult()?</pre>
</blockquote>
<p>On August 2nd, 2013, 1:23 p.m. UTC, <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It should be automatically deleted when the job is gone because ClipboardUpdater sets the job as its parent.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- David</p>
<br />
<p>On August 2nd, 2013, 12:37 p.m. UTC, Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs and David Faure.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Aug. 2, 2013, 12:37 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Unite tests.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=134960">134960</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/kio/clipboardupdater.cpp <span style="color: grey">(8ab9210)</span></li>
<li>kio/kio/clipboardupdater_p.h <span style="color: grey">(b07c320)</span></li>
<li>kio/kio/copyjob.cpp <span style="color: grey">(da19de5)</span></li>
<li>kio/kio/deletejob.cpp <span style="color: grey">(7178424)</span></li>
<li>kio/kio/job.cpp <span style="color: grey">(05d0ba2)</span></li>
<li>kio/kio/paste.cpp <span style="color: grey">(b4372ab)</span></li>
<li>kio/tests/CMakeLists.txt <span style="color: grey">(b570aac)</span></li>
<li>kio/tests/clipboardupdatertest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kio/tests/clipboardupdatertest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kio/tests/fileundomanagertest.h <span style="color: grey">(e909bb7)</span></li>
<li>kio/tests/fileundomanagertest.cpp <span style="color: grey">(709938d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111776/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>