D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven abandoned this revision.
meven marked 5 inline comments as done.
meven added a comment.


  Moved to https://invent.kde.org/frameworks/kio/-/merge_requests/174

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: cfeck, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> kmountpoint.cpp:444
> +// for /dir/link/dir/test will return result for 
> /destLink/dir/test
> +return findByPath(fileinfo.symLinkTarget() + 
> path.mid(cursor));
> +}

Need to pass the flag here.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: cfeck, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven planned changes to this revision.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: cfeck, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Christoph Feck]

cfeck added a comment.


  Are more changes planned?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: cfeck, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  The approach makes sense to me.

INLINE COMMENTS

> kmountpoint.cpp:420
> +#else
> +const QString realname = path;
>  #endif

You should rename this variable, it's no longer the result of 
canonicalFilePath() (also called "realpath" in glibc).

> kmountpoint.h:45
> + * making this function non-blocking, or try to return the real final 
> mount point potentially blocking
> + * if a network filesystem is unresponsive
> + */

Missing @since

> kmountpoint.h:68
>   */
> -Ptr findByPath(const QString ) const;
> +Ptr findByPath(const QString , FindByPathFlag flag = 
> RealMountPoint) const;
>  

This is BIC, you changed the signature of an existing exported method.

You need to overload it, instead.
i.e. add a method with two args, and a comment like

  // TODO KF6 merge with the above method using RealMountPoint as default value

(and a @since flag of course)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added a comment.


  We might want to use Solid instead since it is capable of sending signals 
when mnttab is updated, rather than like here having a mount point list updated 
every 5 seconds.
  With this code we might have issues with potential run condition : when there 
is a mount within the 5 seconds cache refresh window and this function is 
called, it will return incorrect data by default mount point for / .
  If this filesystem becomes unresponsive right away, this will cause a freeze 
because the slowness state would be incorrect.
  
  And a great thing I learned more about at Fosdem, there is a new upcoming 
Linux API io_uring https://lwn.net/Articles/810414/ that will allow us to make 
asynchronous statx calls with timeout (linux 5.6+), making dealing with 
unresponsive file system that much easier.
  
  In the meantime this works.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 75216.
meven added a comment.


  Add a enum parameter to findByPath to choose its behavior regarding returning 
first slow path

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73836=75216

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added a dependent revision: D26916: Allow to display in the kicker 
appliation context menu the recently accessed directories.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added a dependent revision: D26915: Allow to display in the task context 
menu the recently accessed directories.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  > Really? that's not what all callers might want.
  >  e.g. for \"free disk space\" calculation we want the real final mountpoint 
for that path.
  >  If you need something else, it should be a different method, or indeed a 
flag
  
  Good point, I need to take care of this.
  Although the other use case "return the final mountpoint" may be blocking.
  I am back at square "add a function or enum".

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73836.
meven marked 3 inline comments as done.
meven added a comment.


  Fix comment issues and improve syntax used

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73831=73836

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kmountpoint.cpp:436
> +if (result && !result->probablySlow()) {
> +// check recursively its eventual parent dir transitive symlinks
> +int cursor = path.lastIndexOf(QLatin1Char('/'));

is the word "for" missing in this sentence? (as in "check A for B?")

Even then I fail to parse it...

> kmountpoint.cpp:439
> +while (cursor > 0) {
> +const QFileInfo fileinfo = QFileInfo(path.mid(0, cursor));
> +if (fileinfo.isSymLink()) {

Ctor syntax is simpler than assignment syntax (which is in fact a copy ctor). 
And mid(0,N) is left(N).

  const QFileInfo fileInfo(path.left(cursor));

> kmountpoint.h:53
> + * Resolves symlinks before looking for mount point
> + * Returns the first mountpoint that isProbablySlow
> + *

Really? that's not what all callers might want.

e.g. for "free disk space" calculation we want the real final mountpoint for 
that path.

If you need something else, it should be a different method, or indeed a flag

> kmountpoint.h:56
> + * @param path the path to check, must be absolute
> + * @param a MountPointFlag enum value
>   * @return the mount point of the given file

where's this param? leftover docu?

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven marked an inline comment as done.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in kmountpoint.cpp:443
> Why reuse and assign, compared to just `const QFileInfo fileinfo(parentPath)`?
> 
> (Same for parentPath -- I prefer C++ over C)

I expected reviewers to tell to have declaration outside of loops...

> dfaure wrote in kmountpoint.cpp:447
> Wouldn't .mid() be faster than split+join? Just wondering. I see why you need 
> to split, just wondering about join..

I am pretty sure a mid operation would be faster.
It was just that algorithmic-wise it was clearer and faster to put together for 
me.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73831.
meven marked 3 inline comments as done.
meven added a comment.


  Use mid/lastIndexOf instead of split/join, use const assign in loop

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73819=73831

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kmountpoint.cpp:443
> +parentPath = splitted.join(QDir::separator());
> +fileinfo = QFileInfo(parentPath);
> +if (fileinfo.isSymLink()) {

Why reuse and assign, compared to just `const QFileInfo fileinfo(parentPath)`?

(Same for parentPath -- I prefer C++ over C)

> kmountpoint.cpp:447
> +// for /dir/link/dir/test will return result for 
> /destLink/dir/test
> +return findByPath(fileinfo.symLinkTarget() + 
> QDir::separator() + poped.join(QDir::separator()));
> +}

Wouldn't .mid() be faster than split+join? Just wondering. I see why you need 
to split, just wondering about join..

> kmountpoint.cpp:449
> +}
> +poped.append(splitted.takeLast());
> +}

Doesn't this reverse the order?

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in kmountpoint.cpp:438
> Hmm, what if the symlink *is* the very last component, like your previous 
> iteration tried to handle?

I made the incorrect assumption, I had already checked it

> dfaure wrote in kmountpoint.cpp:445
> I don't want to be a pain, but this is still wrong
> 
> If /home/dfaure is a symlink to /opt/dfaure, and then /opt/dfaure/tmp is a 
> symlink to /tmp, then the canonical path (and therefore the mount point) for 
> /home/dfaure/tmp is in fact /tmp.
> 
> But this is going to call findByPath(/opt/dfaure) (the symlink target of the 
> first symlink found in the path), and stop there, assuming that everything at 
> that target is part of the same mountpoint, which isn't necessarily the case.
> 
> I guess this should be findByPath(symLinkTarget() + remainder_of_path)
> 
> One possible objection is a case like /a/b/c/d/e/f/g where g is a symlink to 
> h (in the same directory), because then both levels of recursion will stat a, 
> b, c, d, e, f. But maybe this is unavoidable. I don't know how clever 
> canonicalPath() implementations are to optimize such things, while still 
> allowing for /a/b/c/d/e to be a symlink to something totally different, like 
> /x/y, where /x itself might be a symlink (!!).
> OK so maybe the recursion and redoing the stat's for all levels is correct.

On the contrary, thanks this is again a great review.
Sorry for missing cases.

> OK so maybe the recursion and redoing the stat's for all levels is correct.

I think so, that's what canonicalFilePath does anyway.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73819.
meven marked 3 inline comments as done.
meven added a comment.


  Make the recursive canonical work

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73690=73819

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kmountpoint.cpp:438
> +QStringList splitted = path.split(QDir::separator());
> +splitted.pop_back();
> +QString parentPath;

Hmm, what if the symlink *is* the very last component, like your previous 
iteration tried to handle?

> kmountpoint.cpp:445
> +if (fileinfo.isSymLink()) {
> +return findByPath(fileinfo.symLinkTarget());
> +}

I don't want to be a pain, but this is still wrong

If /home/dfaure is a symlink to /opt/dfaure, and then /opt/dfaure/tmp is a 
symlink to /tmp, then the canonical path (and therefore the mount point) for 
/home/dfaure/tmp is in fact /tmp.

But this is going to call findByPath(/opt/dfaure) (the symlink target of the 
first symlink found in the path), and stop there, assuming that everything at 
that target is part of the same mountpoint, which isn't necessarily the case.

I guess this should be findByPath(symLinkTarget() + remainder_of_path)

One possible objection is a case like /a/b/c/d/e/f/g where g is a symlink to h 
(in the same directory), because then both levels of recursion will stat a, b, 
c, d, e, f. But maybe this is unavoidable. I don't know how clever 
canonicalPath() implementations are to optimize such things, while still 
allowing for /a/b/c/d/e to be a symlink to something totally different, like 
/x/y, where /x itself might be a symlink (!!).
OK so maybe the recursion and redoing the stat's for all levels is correct.

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73690.
meven added a comment.


  Fix poping .length() > 0 ordering

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73687=73690

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73687.
meven marked an inline comment as done.
meven added a comment.


  Check parent dir transitive symlinks, aka manual canonicalPath

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73524=73687

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

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 meant this and this what KMountPoint does, better be precise here.

I see. Sorry, I hadn't realized that there is a Q_OS_ANDROID check in 
kmountpoint itself.

> kmountpoint.cpp:437
> +QFileInfo fileinfo(path);
> +if (fileinfo.isSymLink()) {
> +return findByPath(fileinfo.symLinkTarget());

Compared to canonicalPath, this only works if the very last component is a 
symlink.

If I make /home/dfaure a symlink to /opt/dfaure (because I have more disk space 
there), then findByPath("/home/dfaure/Documents/foo.odt") used to return the 
mountpoint for /opt (which was correct) while now it will return the mount 
point for /home.
isSymlink() will say false because foo.odt isn't a symlink.

I don't see a perfect solution to this. The best we can do IMHO is make things 
work for local symlinked directories and for remote mounts, but we can't fully 
avoid slow calls in the case of symlinks-to-remote-mounts.

So I would just call canonicalPath here, once we find out that the orig path 
isn't a slow mount. Yes it might point to a slow mount, but IMHO that's the 
user asking for (performance) trouble :)

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven added a comment.


  I am thinking about adding some api to check status of nfs, sftp or smb 
mounts by checking their hostname network reachability, to complement isSlow(), 
and perhaps then running a stat of the mount with a timeout.
  This would be kept in a cache with a short duration.
  We still need this because we will lack icons for network files and folders 
as long as we skip mimetype checking solely on fs type.
  And this would allow to make this check default and remove 
m_bSkipMimeTypeFromContent at this point.

INLINE COMMENTS

> dfaure wrote in kfileitem.cpp:787
> OK, but I still read "can only mean" like a possibly incorrect assumption.
> 
> I suggest s/can only mean/for instance/

I meant this and this what KMountPoint does, better be precise here.

> dfaure wrote in kfileitem.cpp:1246
> Did you mean || ?
> 
> Otherwise this changes the behaviour also on fast local paths (when 
> SkipMimeTypeFromContent is set).
> 
> Then again, D19887  (which introduced 
> this if) was apparently about network mounts.

I meant to change the behavior : this isSlow check would be triggered only when 
m_bSkipMimeTypeFromContent is set  or I will create regression in Dolphin for 
instance where icons for folders in network mount would disappear.

> dfaure wrote in kmountpoint.h:66
> 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)

Nice suggestion !

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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73524.
meven marked 4 inline comments as done.
meven added a comment.


  Address review: Make KMountPoint::Ptr KMountPoint::List::findByPath 
non-blocking, checking recursively intermediate symlinks

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73282=73524

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

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  (which introduced this 
if) was apparently about network mounts.

> kmountpoint.h:66
> + */
> +Ptr findByPathDirectly(const QString ) 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

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven updated this revision to Diff 73282.
meven marked 2 inline comments as done.
meven added a comment.


  Move static variables to KFileItemPrivate::getMountPoints

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26407?vs=73261=73282

BRANCH
  arcpatch-D26407

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Méven Car]

meven retitled this revision from "KFileItem: Improve isSlow to not block when 
a network mount is unresponsive, make SkipMimeTypeFromContent skip only on 
network fs" to "KFileItem: Improve isSlow to not block when a network mount is 
unresponsive, make SkipMimeTypeFromContent skip only on slow fs".

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