<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128618/">https://git.reviewboard.kde.org/r/128618/</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 12th, 2016, 9:37 a.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't feel confident about this change, for lack of unit tests.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please add unittests near kio_desktop (not in kio, which can't use kio_desktop).
You can use my (new) tests in kio's jobtest.cpp as starting point:
void createSymlink();
void createSymlinkAsShouldSucceed();
void createSymlinkAsShouldFail();
and add tests for any other code path you see in your code.</p></pre>
</blockquote>
<p>On August 13th, 2016, 8:06 a.m. UTC, <b>Chinmoy Ranjan Pradhan</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think a much simpler way would be to overload kio::link. What do you say?</p></pre>
</blockquote>
<p>On August 13th, 2016, 8:12 a.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't follow, please expand your thought.</p></pre>
</blockquote>
<p>On August 13th, 2016, 8:53 a.m. UTC, <b>Chinmoy Ranjan Pradhan</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are some assumptions in CopyJob. First, the dest is expected to be the destination directory. And then it is assumed that the final file will have the same name as that of source file(unless the file already exist). Although CopyJob can create files with different names but that doesn't comply with these assumptions.
That's why i feel there should be an overload for creating files. This will start the job with source's url and destination directory and will store the filename in a separate variable which can be used after stating the destination to prepare the final dest url.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">/<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">This is what i understood from the code. Please correct me if i am wrong</em>/</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Your assumptions are wrong :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">First, in CopyJob, the dest can be a directory or the final filename. This is just like the "cp" command-line tool:
cp file1 subdir # will create the file subdir/file1
cp file1 subdir/dest # (where dest doesn't exist) will create the file subdir/dest
See the comment about the 6 cases in CopyJobPrivate::sourceStated (the above only mentions case 4 and 6).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Being able to differenciate between these cases is the reason for DEST_IS_DIR vs DEST_IS_FILE vs DEST_DOESNT_EXIST.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The case where "dest" exists already but as a directory is the reason for copyAs/moveAs/linkAs: if we meant to create a file called "dest", we didn't mean to create a "subdir/dest/file1" like KIO::copy would do. This is why KNewFileMenu uses copyAs, and should use linkAs for creating links.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't think any further overloading is needed, all cases are covered, from the API point of view.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The code you mention (adding the filename to the dest dir, when not using copyAs/moveAs/linkAs, is what lines 650 and following do). But if you use linkAs, you shouldn't even have to care about that. Please switch to linkAs, and write unittests -- if needed I can do the actual patch for CopyJob once the tests exist.</p></pre>
<br />
<p>- David</p>
<br />
<p>On August 9th, 2016, 3:20 p.m. UTC, Chinmoy Ranjan Pradhan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Chinmoy Ranjan Pradhan.</div>
<p style="color: grey;"><i>Updated Aug. 9, 2016, 3:20 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KIO::link creates symlink when either protocol+host+port+username+password of the source and the link are same or the link is going to be created locally. In case of plasma's folder view none of the above cases are true therefore creating a symlink in folder view plasmoid gives an error.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This patch aims to fix this issue.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All tests pass.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core/copyjob.cpp <span style="color: grey">(8d6ab05)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128618/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/08/06/d4da6ff3-53d8-49d1-a826-0c8cf12d7aa0__symlink_folderview.png">error message</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>