dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
The use case for this needs to be documented in the constructor. I am not
sure I am in favour of this because it breaks mimetype determination from
contents, which is part of the shared mime info spec.
But actually if that's the only thing that this breaks, and there's a good
use case for it, then the parameter should be SkipMimetypeDetermination. Tell
the user of the API what they'll miss, not what internal implementation detail
this changes.
Everyone wants to skip a syscall, but they need to realize what this means in
terms of feature loss.
INLINE COMMENTS
> kfileitem.cpp:183
> + */
> + bool m_bSkipStat: 1;
> };
move it so it's with the other bits above
> kfileitem.cpp:192
> // stat() local files if needed
> // TODO: delay this until requested
> if (m_fileMode == KFileItem::Unknown || m_permissions ==
> KFileItem::Unknown || m_entry.count() == 0) {
depending on the use case, wouldn't it be enough to implement this TODO instead?
> kfileitem.cpp:751
> + if (scheme.startsWith(QLatin1String("http")) || scheme ==
> QLatin1String("mailto"))
> + d->m_mimeType =
> db.mimeTypeForName(QLatin1String("application/octet-stream"));
> +
missing "else" after this line?
> kfileitem.cpp:1529
> + d->m_mimeType =
> db.mimeTypeForName(QLatin1String("application/octet-stream"));
> +
> + d->m_mimeType = db.mimeTypeForFile(url.path(),
> QMimeDatabase::MatchMode::MatchExtension);
same here -- duplicated code => move to helper function
> kfileitem.h:118
>
> + KFileItem(const QUrl &url, bool skipStat);
> +
booleans parameters are bad, please make it an enum.
https://wiki.qt.io/API_Design_Principles#The_Boolean_Parameter_Trap
+ docu missing including @since tag.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D19887
To: hoffmannrobert, dfaure, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
