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
