Fix creating symlink in Folder View
Review Request #128618 - Created Aug. 6, 2016 and submitted
|Chinmoy Ranjan Pradhan|
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.
All tests pass.
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:
and add tests for any other code path you see in your code.
Should have a much more descriptive name, say m_statingLinkDestDir
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.
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.
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.
Looks great. Two minor fixes and it's good to go.
I just pushed more autotests with 4d43c8f, please rebase and check they pass with your changes too, but they should.
OK, I understand and I like this approach.
In "as" mode we don't need to stat the actual dest URL, better stat the parent directory which is more likely to exist.
It's faster (one stat() instead of two) and simpler.
How do you know that the dest file doesn't exist? Maybe it does exist...
AFAICS it all works just fine without setting destinationState, anyway. It's only used if !m_asMethod.
How can we be sure that sLocalPath ends with a '/' ? Surely the kioslave doesn't have to provide it in UDS_LOCAL_PATH, so we'd better add one here (if it doesn't already end with a slash => needs an if()).
Well, unittests in kio_desktop would certainly be a good idea, but at least in this version most of the change is tested by the kio unittest so I'm more confident.