meven planned changes to this revision. meven added a comment.
In D25010#556237 <https://phabricator.kde.org/D25010#556237>, @dfaure wrote: > If you change the actual values being sent to kioslaves, then this breaks the "wire protocol". I.e. after upgrading KF5 on a live system, kioslaves (started by kdeinit5 forking the "old" libkiocore) will receive numbers from newly started apps (which link to the "new" libkiocore), that they won't understand. Of course a simple `kdeinit5` in a terminal would fix that, but most users don't know that. (One idea that has been floating around is to detect changes to libKIO.so and other dependencies and auto-restart kdeinit when these libs are changed... But, hmm, then already-running apps will send old values to newly started kioslaves, problem again). Thank you for reminding me of this concern. > One solution is to make the enum actually use the old values (0, 2, 3) by doing careful mapping. That's what I thought we would do, but I see you went for a more flexible approach where apps can pick the exact groups of fields they want. > > So another solution would be to use a different metadata key and send the old key (with the old value, at least a close approximation of it) for compatibility purposes. That's the solution I would prefer : it would allow me to add a "TODO KF6 remove to the old metadata key", breaking the wire format should be fine then, so we end up with the best state for KF6. Next items: - Add compatibility to older KIO version on the wire - match the flags to statx flags to avoid paying for unwanted fields down to the syscall. INLINE COMMENTS > statjob.h:53 > + /// Filename, access, type, size, linkdest > + Basic = 0x1, > + /// uid, gid I am open to suggestion here : 1. Should it be more granular ? 2. Are names KIO-ish ? @kossebau : > On a first look, the names seems very short & generic to me, having some > other name element might avoid semantic collisions perhaps. No idea yet, not > looked further. > dfaure wrote in file.cpp:896 > Did you know you can just write `if (details & KIO::StatJob::Basic)`? > This is more C++-like, which is probably more future-proof if one day don't > use QFlags anymore for these types of things. > > Or did you use testFlag() for a specific reason? I used testFlag because i was documented, and I still have to familiar myself with C/C++. > dfaure wrote in listjobtest.cpp:44 > It's a bit weird to use a StatJob enum in the ListJob class. But then again, > it is about the stat() done by listing.... So this is OK, I guess. > Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How > does it match other similar flags? It was @kossebau concern as well. I am open to suggestion to improve on that, maybe this enum should be in KIO namespace ? In the meantime, it was what was made the most sense to me. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25010 To: meven, #frameworks, dfaure, kossebau Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns