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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  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).
  
  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.

INLINE COMMENTS

> file.cpp:895
>      assert(entry.count() == 0); // by contract :-)
> -    switch (details) {
> -    case 0:
> +    short entries = 0;
> +    if (details.testFlag(KIO::StatJob::Basic)) {

int is actually faster on most CPUs, and reserve() will convert it to an int anyway, so it might as well be an int from the beginning.

> file.cpp:896
> +    short entries = 0;
> +    if (details.testFlag(KIO::StatJob::Basic)) {
>          // filename, access, type, size, linkdest

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?

> listjobtest.cpp:44
>          job->setUiDelegate(nullptr);
> -        job->addMetaData(QStringLiteral("details"), QStringLiteral("2")); // Default is 2 which means all details. 0 means just a few essential fields (KIO::UDSEntry::UDS_NAME, KIO::UDSEntry::UDS_FILE_TYPE and KIO::UDSEntry::UDS_LINK_DEST if it is a symbolic link. Not provided otherwise.
> +        job->addMetaData(QStringLiteral("details"), QString::number(KIO::StatJob::StatDefaultDetails));
>  

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.

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/20191029/98ad1053/attachment-0001.html>