<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/111585/">http://git.reviewboard.kde.org/r/111585/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You mention running tests, but it doesn't really count since no test is testing this clipboard stuff at the moment :-)</pre>
<br />
<div>
<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/111585/diff/1/?file=172220#file172220line34" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/updateclipboard.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">34</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">job</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</div>
<br />
<div>
<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/111585/diff/1/?file=172221#file172221line30" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/updateclipboard_p.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">30</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">class</span> <span class="n">UpdateClipboard</span> <span class="o">:</span> <span class="n">public</span> <span class="n">QObject</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</div>
<br />
<div>
<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/111585/diff/1/?file=172221#file172221line40" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/updateclipboard_p.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">explicit</span> <span class="nf">UpdateClipboard</span><span class="p">(</span><span class="n">Job</span><span class="o">*</span> <span class="n">job</span><span class="p">,</span> <span class="n">UpdateMode</span> <span class="n">mode</span> <span class="o">=</span> <span class="n">UpdateContent</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :).</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On July 19th, 2013, 1:17 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 July 19, 2013, 1:17 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;">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.</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;">Unit and manual 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=318757">318757</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/CMakeLists.txt <span style="color: grey">(f7a3767)</span></li>
<li>kio/kio/fileundomanager.cpp <span style="color: grey">(9f76fef)</span></li>
<li>kio/kio/paste.cpp <span style="color: grey">(ca451fb)</span></li>
<li>kio/kio/updateclipboard.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kio/kio/updateclipboard_p.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111585/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>