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

[David Faure]

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 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 ~/file.txt to ~/file2.txt, the actual destination is ~/file2.txt.
Or copying ~/dir1 as ~/dir2, then the actual destination is ~/dir2. So even a name like destDir isn't perfect...)

existingDest maybe. ~ exists. ~/dir2 not yet.

> copyjob.cpp:448
>                  const QString fileName = m_dest.fileName();
> +                // The statJob that returned "entry" has taken into account m_asMethod
>                  m_dest = QUrl::fromLocalFile(sLocalPath);

It has, but "taking into account" is ambiguous. I first thought it meant "we stat'ed the final dest", while in fact it means we stat'ed the parent existing dest. Maybe no comment is better than an ambiguous comment :-)

> ahmadsamir wrote in copyjob.cpp:479
> s/below/below, because return is called right after it, so that statCurrentSrc is called from the lambda/
> 
> does that sound better?

Yes. But comments should add information, not just describe what the code already says :-)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29485

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200508/bb1ae1a0/attachment.htm>