D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option

[David Faure]

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

INLINE COMMENTS

> kfileitem.cpp:362
> +
> +    if (!item.m_bInitCalled && m_bInitCalled) {
> +        item.init();

There is one thing I've been wondering back and forth about: the alternative way of doing this, which is to call init() unconditionally and testing the bool first thing in there.
The benefit is that it would reduce the code size overall, and greatly simplify the top of this method (two calls to init, done).
The downside is that for the reader, a call to init() might in fact do nothing (and it reads like it's doing something, until going there to check). So it's maybeInit(), urgh.

Ah, in another code base (QMimeType) I wrote ensureLoaded(). This could be ensureInitialized() ?

Sorry for not suggesting this earlier. Do you agree that it would make the code better?

> kfileitem.cpp:609
>  
> -    d->m_entry.replace(KIO::UDSEntry::UDS_LOCAL_PATH, path);
> +    entry().replace(KIO::UDSEntry::UDS_LOCAL_PATH, path);
>  }

No no, this won't work. entry() returns a copy, not a reference.

> kfileitem.cpp:663
>      // Extract the local path from the KIO::UDSEntry
>      return m_entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH);
>  }

here entry() would work, no?

> kfileitem.cpp:1054
>  
> -    QStringList names = d->m_entry.stringValue(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES).split(QLatin1Char(','), QString::SkipEmptyParts);
> +    QStringList names = entry().stringValue(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES).split(QLatin1Char(','), QString::SkipEmptyParts);
>  

It's more fragile this way here, because the reader of this code won't see that d->m_bLink is initialized indirectly via entry() calling init().

This is why I had only suggested to use entry() in a few places, not in those other places which need init() for other reasons anyway.

By fragile it means, it works today, but any further work/refactoring of this code is likely to break.

Same problem in linkDest().

REPOSITORY
  R241 KIO

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

To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190329/d0aa5c9c/attachment.htm>