D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

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