dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> trashimpl.cpp:1098
> +
> + // Find lateest modification date
> + if (res.mtime > latestModifiedDate) {
typo: latest
> trashsizecache.cpp:144
> + if (trashFileInfo.exists() &&
> trashFileInfo.lastModified().toMSecsSinceEpoch() > max_mtime) {
> + max_mtime =
> trashFileInfo.lastModified().toMSecsSinceEpoch();
> + }
This calls lastModified().toMSecsSinceEpoch() twice.
How about a lambda for (at least)
if (lastModTime > max_mtime) {
max_mtime = lastModTime;
}
and then you can call that lambda with lastModified().toMSecsSinceEpoch()?
I was thinking about suggesting such a lambda anyway since that logic exists 4
times in this code.
In fact, checking that the trashinfo file exists is something that should be
done in all code paths, no? So there's a lot more code that can be factored
out, I would think.
> trashsizecache.cpp:168
> if (!usableCache) {
> + // orphaned directories
> const qulonglong size =
> DiscSpaceUtil::sizeOfPath(file.absoluteFilePath());
orphaned sounds like "no .trashinfo file pointing to that dir".
But that's not the case here. It's just items that have been added to the trash
without being added to the cache file (many implementations do that, the cache
is optional).
So the directory is not listed in the cache, or is listed with a too old mtime
i.e. it wasn't updated when adding another item into it.
> trashsizecache.cpp:171
> sum += size;
> - add(fileId, size);
> + long mtime =
> QFileInfo(file.absolutePath()).lastModified().toMSecsSinceEpoch() ;
> + if (mtime > max_mtime) {
Not technically correct, this is the mtime of the directory, while there could
be much more recent items inside that directory.
But I'm wondering how much effort we want to put into finding the latest mtime,
this is starting to sound like a very expensive operation.
So I'm actually find with this known bug, maybe with a comment about how this
is a heuristic -- in a fallback for bad implementations.
When kio_trash takes care of the trashing, we should never end up in this code
path (right?)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D24773
To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
