D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:444 > I'm a bit confused. My explanation here points to kio_desktop and kio_remote > (and was apparently clear), and the API docs for UDS_LOCAL_PATH say > > /// A local file path if the ioslave display files sitti

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

dfaure added inline comments. INLINE COMMENTS > ahmadsamir wrote in copyjob.cpp:444 > Excellent points; I was scratching my head trying to figure out what use > UDS_LOCAL_PATH is when, while testing with qDebug(), it was _always_ empty, > the only time it wasn't empty was in testtrash unit test

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

This revision was automatically updated to reflect the committed changes. Closed by commit R241:1cac602d9966: [CopyJob] Check free space for remote urls before copying and other improvements (authored by ahmadsamir). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir added a comment. @dfaure please don't forget to flesh out the UDS_LOCAL_PATH docs https://phabricator.kde.org/D29485#inline-169199 REPOSITORY R241 KIO BRANCH l-freespace-remote-2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D29485 To: ahmadsamir, #fra

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:477 > I'm assuming the TODO was about "What if I'm using a NFS mount and the > connection breaks at the time of KDiskFreeSpaceInfo, i.e. what should we do > about error handling". > > But I think the current code -

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

dfaure accepted this revision. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > ahmadsamir wrote in copyjob.cpp:477 > I meant connection to the remote ulr/host; however e.g Dolphin already > reports those connection errors in the status bar when it

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir updated this revision to Diff 82334. ahmadsamir added a comment. Remove comment REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29485?vs=82305&id=82334 BRANCH l-freespace-remote-2 (branched from master) REVISION DETAIL https://phabricator.kde.or

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:477 > This comment is now completely out of context. It used to be about NFS/SMB, > now this information has gone and one is left wondering why kind of > connections we're talking about (connection to the kioslave??

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Sorry, I have one last comment about a comment :) INLINE COMMENTS > copyjob.cpp:477 > +// Check available free space for remote urls > +// TODO: find a way

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir updated this revision to Diff 82305. ahmadsamir marked an inline comment as done. ahmadsamir added a comment. "existingDest" is a better name for the var relating to m_asMethod REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29485?vs=82285&id=82305 B

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir marked an inline comment as done. ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:430 > This is the same as "actualDest" too, so its definition could be moved > further up and shared with this too. > > (Not that the name is perfect --- when copying ~/f

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

dfaure added inline comments. INLINE COMMENTS > copyjob.cpp:430 > +if (!m_privilegeExecutionEnabled && !isWritable) { > +const QUrl dest = m_asMethod ? > m_dest.adjusted(QUrl::RemoveFilename) : m_dest; > +q->setError(ERR_WRITE_ACCESS_DENIED); This is

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir updated this revision to Diff 82285. ahmadsamir marked 2 inline comments as done. ahmadsamir added a comment. Address comments REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29485?vs=82113&id=82285 BRANCH l-freespace-remote-2 (branched from master

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir marked 2 inline comments as done. ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:430 > Here you kept a comment that said "we want to check", but the check already > happened. > I'd say just remove the two lines of comments. > The code is clearer withou

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Good idea overall. INLINE COMMENTS > copyjob.cpp:430 > +if (!m_privilegeExecutionEnabled && !isWritable) { > +// In copy-as mode, we want to check the

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ngraham added a comment. In D29485#664977 , @ahmadsamir wrote: > I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using dolphin, the paste action is disabled if the dir isn't owned by me. See D7563

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir added a comment. I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using dolphin, the paste action is disabled if the dir isn't owned by me. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29485 To: ahmadsamir, #frameworks, dfaure, meven, si

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, meven, sitter. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY Use KIO::FileSystemFreeSpaceJob to check free space for remote urls. Thanks to sitter for pin-p