Review Request 128618: Fix creating symlink in Folder View

[David Faure]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160812/8920e8e5/attachment-0001.html>