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
