dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> meven wrote in kfileitem.cpp:787
> I did not mean to avoid android, just surfaced the comment in
> KMountPoint::currentMountPoints regarding its limitation to make it clear why
> we need an else block here in the first place.
OK, but I still read "can only mean" like a possibly incorrect assumption.
I suggest s/can only mean/for instance/
> kfileitem.cpp:771
> + // refresh cached mountpoints data
> + mountPoints = KMountPoint::currentMountPoints();
> + }
You forgot to update lastMountPointUpdate here.
And to avoid calling currentDateTime() twice (this is a relatively slow
method), make sure to put the result into a local var used in both places.
> kfileitem.cpp:1246
> + // avoid potential blocking stat on network mount
> + if (d->m_bSkipMimeTypeFromContent && d->isSlow()) {
> return false;
Did you mean || ?
Otherwise this changes the behaviour also on fast local paths (when
SkipMimeTypeFromContent is set).
Then again, D19887 <https://phabricator.kde.org/D19887> (which introduced this
if) was apparently about network mounts.
> kmountpoint.h:66
> + */
> + Ptr findByPathDirectly(const QString &path) const;
> +
I don't really like the name, it's non-obvious until reading the documentation.
Maybe it should be findByPath(path, KMountPoint::DontResolveSymlinks) ?
(with a 2-args findByPath overload and a KF6 TODO to merge the two)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D26407
To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
bruns
