D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200413/eadc9ba9/attachment-0001.html>