D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
This revision was automatically updated to reflect the committed changes. Closed by commit R241:0ba76d410a7c: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option (authored by hoffmannrobert, committed by dfaure). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19887?vs=55033=55063 REVISION DETAIL https://phabricator.kde.org/D19887 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert marked 3 inline comments as done. hoffmannrobert added a comment. Yes, please, and https://phabricator.kde.org/D19784 too. REPOSITORY R241 KIO BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks! Looks nice now. I don't see you in the list of developer accounts, so I guess you need someone to push this for you? REPOSITORY R241 KIO BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert marked 4 inline comments as done. hoffmannrobert added inline comments. INLINE COMMENTS > dfaure wrote in kfileitem.cpp:362 > 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? Yes, I agree, but here you still need the two ifs, and the init() still is needed because of refresh(). > dfaure wrote in kfileitem.cpp:609 > No no, this won't work. entry() returns a copy, not a reference. Ah, sorry, yes. > dfaure wrote in kfileitem.cpp:663 > here entry() would work, no? No, it's KFileItemPrivate. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert updated this revision to Diff 55033. hoffmannrobert marked 3 inline comments as done. hoffmannrobert added a comment. - Add ensureInitialized(), entry() changes REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19887?vs=55025=55033 BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
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
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert marked 4 inline comments as done. hoffmannrobert added inline comments. INLINE COMMENTS > dfaure wrote in kfileitem.cpp:361 > What about the other way around? I think this needs the symmetrical test to > call item.init() if needed > (and the corresponding unittest, write it first) You're right, fixed, unittest added. > dfaure wrote in kfileitem.cpp:730 > This use of d->m_entry needs a call to init(), no? It does, now there via entry(). And ACL() needs init(), too, it's in hasExtendedACL() there. > dfaure wrote in kfileitem.cpp:766 > This kind of method (which only uses d->m_entry in one place) could be > simplified by just doing > > return entry().stringValue(); > > Then the init() would happen inside entry(). > > This would work in user() just above, too. And in defaultACL(), setLocalPath(), linkDest(), hasExtendedACL() and overlays(). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert updated this revision to Diff 55025. hoffmannrobert added a comment. - Fix cmp() #3, add test for cmp()/init(), simplifications REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19887?vs=54934=55025 BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileitem.cpp:86
> * Computes the text and mode from the UDSEntry
> * Called by constructor, but can be called again later
> * Nothing does that anymore though (I guess some old KonqFileItem did)
Remove this comment, no longer true (only keep the first line, remove the other
3)
> kfileitem.cpp:361
> {
> +if (item.m_bInitCalled && !m_bInitCalled) {
> +init();
What about the other way around? I think this needs the symmetrical test to
call item.init() if needed
(and the corresponding unittest, write it first)
> kfileitem.cpp:730
> // Extract it from the KIO::UDSEntry
> const QString fieldVal =
> d->m_entry.stringValue(KIO::UDSEntry::UDS_DEFAULT_ACL_STRING);
> if (!fieldVal.isEmpty()) {
This use of d->m_entry needs a call to init(), no?
> kfileitem.cpp:766
>
> +if (!d->m_bInitCalled) {
> +d->init();
This kind of method (which only uses d->m_entry in one place) could be
simplified by just doing
return entry().stringValue();
Then the init() would happen inside entry().
This would work in user() just above, too.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D19887
To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert updated this revision to Diff 54934. hoffmannrobert marked 4 inline comments as done. hoffmannrobert added a comment. - Fix cmp() #2 REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19887?vs=54932=54934 BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 AFFECTED FILES src/core/kfileitem.cpp src/core/kfileitem.h To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert added a comment. KFileItemTest::testCmp() failed - fixed. JobTest also fails, but this fails in master, too. Something with FileCopyJob. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert updated this revision to Diff 54932. hoffmannrobert added a comment. - Fix cmp(), SkipMimeTypeFromContent rename, change isDir() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19887?vs=54862=54932 BRANCH add_skipStat REVISION DETAIL https://phabricator.kde.org/D19887 AFFECTED FILES src/core/kfileitem.cpp src/core/kfileitem.h To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks.
Did you also run the kio unittests to ensure they still pass?
INLINE COMMENTS
> kfileitem.cpp:203
> // stat() local files if needed
> // TODO: delay this until requested
> if (m_fileMode == KFileItem::Unknown || m_permissions ==
> KFileItem::Unknown || m_entry.count() == 0) {
Remove the TODO :-)
> kfileitem.cpp:491
> +QMimeDatabase db;
> +if (m_bSkipMimeTypeDetermination) {
> +const QString scheme = url.scheme();
So, we don't really skip it. We just use a "fast and less precise" mode.
Sounds like this should be called DetermineMimeTypeFromExtension (though that's
incorrect for http),
or FastMimeTypeDetermination (but people then think it's a good idea to call
this in all cases...)
or SkipMimeTypeFromContent -- maybe better?
> kfileitem.cpp:1019
>
> -if (isLocalUrl && !delaySlowOperations && isDir()) {
> +if (isLocalUrl && !delaySlowOperations &&
> !d->m_bSkipMimeTypeDetermination && isDir()) {
> if (isDirectoryMounted(url)) {
It reads like a bit of a hack here, because reading .directory is unrelated to
mimetype determination.
But I think your idea is that isDir() is what we don't want to call right?
Another way to do this, then, would be to change isDir() to have an early
`return false` if mimetype determination is skipped?
> kfileitem.cpp:1605
> QMimeDatabase db;
> -if (isDir()) {
> +if (!d->m_bSkipMimeTypeDetermination && isDir()) {
> d->m_mimeType =
> db.mimeTypeForName(QStringLiteral("inode/directory"));
my suggestion would simplify this here too
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D19887
To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert marked 5 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19887: KFileItem: call stat() on demand, add SkipMimeTypeDetermination option
hoffmannrobert retitled this revision from "Proposal for KFileItem to skip stat()" to "KFileItem: call stat() on demand, add SkipMimeTypeDetermination option". hoffmannrobert edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19887 To: hoffmannrobert, dfaure, #frameworks, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns
