Changeset View
Standalone View
src/core/copyjob.cpp
Show First 20 Lines • Show All 842 Lines • ▼ Show 20 Line(s) | 842 | } else if (m_currentSrcURL.isLocalFile() && KProtocolManager::canRenameFromFile(m_dest)) { | |||
---|---|---|---|---|---|
843 | startRenameJob(m_dest); | 843 | startRenameJob(m_dest); | ||
844 | return; | 844 | return; | ||
845 | } else if (m_dest.isLocalFile() && KProtocolManager::canRenameToFile(m_currentSrcURL)) { | 845 | } else if (m_dest.isLocalFile() && KProtocolManager::canRenameToFile(m_currentSrcURL)) { | ||
846 | startRenameJob(m_currentSrcURL); | 846 | startRenameJob(m_currentSrcURL); | ||
847 | return; | 847 | return; | ||
848 | } | 848 | } | ||
849 | } | 849 | } | ||
850 | 850 | | |||
851 | // if the file system doesn't support deleting, we do not even stat | 851 | // if the source file system doesn't support deleting, we do not even stat | ||
852 | if (m_mode == CopyJob::Move && !KProtocolManager::supportsDeleting(m_currentSrcURL)) { | 852 | if (m_mode == CopyJob::Move && !KProtocolManager::supportsDeleting(m_currentSrcURL)) { | ||
853 | QPointer<CopyJob> that = q; | 853 | QPointer<CopyJob> that = q; | ||
854 | emit q->warning(q, buildErrorString(ERR_CANNOT_DELETE, m_currentSrcURL.toDisplayString())); | 854 | emit q->warning(q, buildErrorString(ERR_CANNOT_DELETE, m_currentSrcURL.toDisplayString())); | ||
855 | if (that) { | 855 | if (that) { | ||
856 | statNextSrc(); // we could use a loop instead of a recursive call :) | 856 | statNextSrc(); // we could use a loop instead of a recursive call :) | ||
857 | } | 857 | } | ||
858 | return; | 858 | return; | ||
859 | } | 859 | } | ||
860 | 860 | | |||
861 | // if the destination file system doesn't support writing, do not stat | ||||
862 | if (m_currentDestURL.isLocalFile()) { | ||||
dfaure: Wrong mix of URLs and paths. How can this ever work? QFileInfo takes local paths, and you're… | |||||
863 | QFileInfo destInfo(m_currentDestURL.toLocalFile()); | ||||
Why the "copy or move" test? The only alternative is creating a symlink, which also requires being able to write, no? dfaure: Why the "copy or move" test? The only alternative is creating a symlink, which also requires… | |||||
864 | if (m_mode && (destInfo.isDir() && destInfo.isWritable())) { | ||||
broulik: Not sure `ERR_CYCLIC_COPY` is the correct error for this? | |||||
dfaure: It doesn't sound like it, no. This should rather be ERR_CANNOT_WRITE. | |||||
865 | q->setError(ERR_CANNOT_WRITE); | ||||
This used to be an error, now it gets degraded to a warning. This means applications performing the copy will think it actually worked, only the user got (maybe) a warning.... I think this should be an error. And there should be a unittest for it. JobTest::copyFolderWithUnaccessibleSubfolder shows how to make a folder non-writable (and still be able to clean it up at the end). A similar test should be added where the destination is the one that is unaccessible (or just unwritable). dfaure: This used to be an error, now it gets degraded to a warning. This means applications performing… | |||||
866 | q->setErrorText(buildErrorString(ERR_CANNOT_DELETE, m_currentDestURL.toDisplayString())); | ||||
867 | q->emitResult(); | ||||
that's not going to be a helpful error text on it's own. Code above calls buildErrorString davidedmundson: that's not going to be a helpful error text on it's own.
Code above calls buildErrorString | |||||
868 | return; | ||||
869 | } | ||||
870 | } | ||||
871 | | ||||
861 | m_bOnlyRenames = false; | 872 | m_bOnlyRenames = false; | ||
emitting a result and then continuing seems questionable, you'll end up with the job emitting another result later davidedmundson: emitting a result and then continuing seems questionable, you'll end up with the job emitting… | |||||
emitresult() emits the corresponding signal and then commits suicide, hence no such chance possible. shubham: ```
emitresult() emits the corresponding signal and then commits suicide, hence no such chance… | |||||
dfaure: Still, don't call statNextSrc() then. | |||||
862 | 873 | | |||
863 | // Testing for entry.count()>0 here is not good enough; KFileItem inserts | 874 | // Testing for entry.count()>0 here is not good enough; KFileItem inserts | ||
864 | // entries for UDS_USER and UDS_GROUP even on initially empty UDSEntries (#192185) | 875 | // entries for UDS_USER and UDS_GROUP even on initially empty UDSEntries (#192185) | ||
865 | if (entry.contains(KIO::UDSEntry::UDS_NAME)) { | 876 | if (entry.contains(KIO::UDSEntry::UDS_NAME)) { | ||
866 | qCDebug(KIO_COPYJOB_DEBUG) << "fast path! found info about" << m_currentSrcURL << "in KCoreDirLister"; | 877 | qCDebug(KIO_COPYJOB_DEBUG) << "fast path! found info about" << m_currentSrcURL << "in KCoreDirLister"; | ||
867 | // sourceStated(entry, m_currentSrcURL); // don't recurse, see #319747, use queued invokeMethod instead | 878 | // sourceStated(entry, m_currentSrcURL); // don't recurse, see #319747, use queued invokeMethod instead | ||
868 | QMetaObject::invokeMethod(q, "sourceStated", Qt::QueuedConnection, Q_ARG(KIO::UDSEntry, entry), Q_ARG(QUrl, m_currentSrcURL)); | 879 | QMetaObject::invokeMethod(q, "sourceStated", Qt::QueuedConnection, Q_ARG(KIO::UDSEntry, entry), Q_ARG(QUrl, m_currentSrcURL)); | ||
869 | return; | 880 | return; | ||
▲ Show 20 Lines • Show All 1419 Lines • Show Last 20 Lines |
Wrong mix of URLs and paths. How can this ever work? QFileInfo takes local paths, and you're giving it a full URL?
All this should be inside if (m_currentDestURL.isLocalFile()) and use m_currentDestURL.toLocalFile() as the path to give to QFileInfo.