D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
dfaure closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28901 To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
meven accepted this revision.
meven added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> dfaure wrote in global.h:323
> There are bugs in the current kio_file implementation if StatBasic isn't set.
>
> mode_t type = 0;
> if (details & KIO::StatBasic) {
> ... code that sets type ...
> }
> if (details & KIO::StatAcl) {
> appendACLAtoms(targetPath, entry, type); // oops type is 0
> }
>
> Hmm I thought I saw more, but now I don't see more (must have been fixed
> meanwhile). If you fix the bug I'm happy to remove the comment, LOL.
I believe it was `StatResolveSymlink`
D28947 for the StatAcl fix
REPOSITORY
R241 KIO
BRANCH
2020_optimize_recursive_size
REVISION DETAIL
https://phabricator.kde.org/D28901
To: dfaure, meven, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
dfaure updated this revision to Diff 80428. dfaure added a comment. Remove comment change above StatBasic REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28901?vs=80335=80428 BRANCH 2020_optimize_recursive_size REVISION DETAIL https://phabricator.kde.org/D28901 AFFECTED FILES src/core/global.h src/ioslaves/trash/kio_trash.cpp src/ioslaves/trash/kio_trash.h src/ioslaves/trash/trashimpl.cpp src/ioslaves/trash/trashimpl.h To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
dfaure added inline comments. INLINE COMMENTS > meven wrote in trashimpl.cpp:1105 > Maybe, check for `details & KIO::StatTime` > Down the rabbit hole... Well, if StatRecursiveSize is set, we have the info, so we might as well provide it. If it's not set, then this would mean we need to do the whole recursive thing also when StatTime is set, and it's part of StatDefaultDetails so I expect we'll do this often for nothing. I think we don't really want to do that, in practice we need both pieces of information, or none. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28901 To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
dfaure added inline comments.
INLINE COMMENTS
> meven wrote in global.h:323
> I don't think we should assume that, it defeats quite the purpose.
> For instance even if we the ioslave gets the filename or type for its needs
> it may just not include it in the UDSEntry if StatBasic was not passed.
There are bugs in the current kio_file implementation if StatBasic isn't set.
mode_t type = 0;
if (details & KIO::StatBasic) {
... code that sets type ...
}
if (details & KIO::StatAcl) {
appendACLAtoms(targetPath, entry, type); // oops type is 0
}
Hmm I thought I saw more, but now I don't see more (must have been fixed
meanwhile). If you fix the bug I'm happy to remove the comment, LOL.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D28901
To: dfaure, meven, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
meven requested changes to this revision. meven added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > global.h:323 > StatNoDetails = 0x0, > -/// Filename, access, type, size, linkdest > +/// Filename, access, type, size, linkdest -- necessary for all others > below > StatBasic = 0x1, I don't think we should assume that, it defeats quite the purpose. For instance even if we the ioslave gets the filename or type for its needs it may just not include it in the UDSEntry if StatBasic was not passed. > meven wrote in trashimpl.cpp:1105 > Maybe, check for `details & KIO::StatTime` > Down the rabbit hole... (I don't expect it) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28901 To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
meven accepted this revision. meven added a comment. This revision is now accepted and ready to land. So we are gonna need a patch to baloo-widget stat, with `KIO::StatRecursiveSize` A test would nice for trash:/ with `KIO::StatRecursiveSize` INLINE COMMENTS > trashimpl.cpp:1105 > - > -entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME, > latestModifiedDate / 1000); > -// access date is unreliable for the trash folder, use the modified date > instead Maybe, check for `details & KIO::StatTime` Down the rabbit hole... REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D28901 To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand
dfaure created this revision. dfaure added reviewers: meven, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY This indirectly fixes the testtrash unittest which noticed that the directory size cache was being updated when CopyJob stat's the destination trash:/. TEST PLAN bin/testtrash REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D28901 AFFECTED FILES src/core/global.h src/ioslaves/trash/kio_trash.cpp src/ioslaves/trash/kio_trash.h src/ioslaves/trash/trashimpl.cpp src/ioslaves/trash/trashimpl.h To: dfaure, meven, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
