-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128618/#review98236
-----------------------------------------------------------
I don't feel confident about this change, for lack of unit tests.
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.
src/core/copyjob.cpp (line 208)
<https://git.reviewboard.kde.org/r/128618/#comment66252>
Should have a much more descriptive name, say m_statingLinkDestDir
src/core/copyjob.cpp (line 411)
<https://git.reviewboard.kde.org/r/128618/#comment66256>
You're not setting this anymore. But what if the dest dir doesn't exist
either ?
-> add a test for KIO::link(src, "file:///does/not/exist"). Well, I just
added JobTest::createSymlinkTargetDirDoesntExist, do the same with kio_desktop.
src/core/copyjob.cpp (line 415)
<https://git.reviewboard.kde.org/r/128618/#comment66257>
I am not confident about this. This code says "the destination exists, as a
file", when in fact the (updated) destination is the final file (link) name,
which doesn't exist yet.
Granted, DEST_IS_FILE isn't used right now, but if one day we use this as a
shortcut (to avoid calling the slave to perform a copy over a file that already
exists), this code path will lead to bugs.
I think DEST_DOESNT_EXIST would be a much better representation of the
reality, when the dest (file) indeed doesn't exist (i.e. when statingAgain is
true).
Or maybe it would be simpler to not change m_dest below, and keep the dest
as it was. AFAICS there are more bugs lurking here: what if there was no
uds_local_path ? Then we modified destinationState, but not m_dest, so they
don't match anymore.
src/core/copyjob.cpp (line 424)
<https://git.reviewboard.kde.org/r/128618/#comment66172>
path/localfile confusion.
This should be m_dest.setPath(m_dest.path() + filename).
This looks wrong for another reason: if we're here, isGlobalDest is true,
so m_dest == m_globalDest. So we're appending the filename to the full path in
m_dest ? Sounds like we'll have the filename twice.
Also, by changing dest from a dest dir to a dest file, you're changing
KIO::link into KIO::linkAs, no? I'm in fact confused as to whether you're still
using link, or linkAs.
- David Faure
On Aug. 9, 2016, 3:20 p.m., Chinmoy Ranjan Pradhan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128618/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2016, 3:20 p.m.)
>
>
> Review request for KDE Frameworks and David Faure.
>
>
> Repository: kio
>
>
> Description
> -------
>
> 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.
> This patch aims to fix this issue.
>
>
> Diffs
> -----
>
> src/core/copyjob.cpp 8d6ab05
>
> Diff: https://git.reviewboard.kde.org/r/128618/diff/
>
>
> Testing
> -------
>
> All tests pass.
>
>
> File Attachments
> ----------------
>
> error message
>
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/06/d4da6ff3-53d8-49d1-a826-0c8cf12d7aa0__symlink_folderview.png
>
>
> Thanks,
>
> Chinmoy Ranjan Pradhan
>
>
