Populate UDS_CREATION_TIME on Linux if Qt and kernel versions support it
Needs ReviewPublic

Authored by ngraham on Aug 20 2017, 1:28 AM.

Details

Summary

FEATURE: 286689

Populate UDS_CREATION_TIME to make Dolphin display file creation times, if the Linux Kernel and Qt version support this.

If one or both of the above are false, there is no effect and UDS_CREATION_TIME is not populated.

Test Plan

Tested these changes with Dolphin in KDE Neon:

  • Without Qt 5.10 and/or Kernel 4.11: no regressions; UDS_CREATION_TIME is not populated and no file creation dates are displayed in Dolphin.
  • With Qt 5.10 and Kernel 4.11+: TBD (soon)

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D7423
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Aug 20 2017, 1:28 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 20 2017, 1:28 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
ngraham edited the summary of this revision. (Show Details)Aug 20 2017, 1:30 AM

The commit log is inconsistent with the actual patch. It says "lack of a Qt version that supports this", but the patch doesn't use QFile at all, it uses statx. Either way, the patch can be tested, no?

ngraham added a comment.EditedAug 20 2017, 8:14 PM

This patch doesn't actually directly use statx(); it uses the existing approach, which goes through QFile, which is used to populate the statbuf object that we query. QT has gained support for populating the statbuf object using statx() in Linux where available in current git master, but that change hasn't landed in an actual released QT version yet.

If you can point me to a way to test this, I'll be happy to.

I can't comment on the content of the patch, but about the commit message, in order to close a bug, use the syntax:

BUG: nnnnnn

while in order to add the commit as comment of a bug without closing it, use

CCBUG: nnnnnn

Both keyword should be in a separate line.

Also, it's Qt, not QT.

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if QT version and Linux kernel support it (no effect if they don't) to Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't).Aug 20 2017, 8:32 PM
ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) to Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) BUG: 286689.Aug 20 2017, 8:34 PM

Noted! Thanks, Luigi. I'd like to add BUG: 286689 to the commit message on a separate line. Does the title of this revision become the commit message? If so, how do I add another line? The text field doesn't seem to support multi-line strings.

The title of the revision becomes the first line; all the rest (summary, test, reviewers) become the rest of the content, a blank separator line is added automatically. Please add the BUG: line in the summary, not in the title.
Also, as the title becomes the first line, it would be nice to follow the usual git rules of length.

Ah, OK, I see.
Well, still, you could test the patch by compiling qtbase dev (or by applying whichever Qt patch is necessary).

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) BUG: 286689 to Populate UDS_CREATION_TIME on Linux if Qt version and kernel support it.Aug 20 2017, 9:32 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Great, thanks guys. Thanks for bearing with me here. I'll see if I can test with the unreleased qtbase.

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and kernel support it to Populate UDS_CREATION_TIME on Linux if Qt and kernel versions support it.Aug 20 2017, 9:46 PM

I haven't forgotten about this, but life has intervened and I'm not going to have time to work on it for a few days, maybe even a couple of weeks. If anyone else wants to run with it and take over, I won't be in the least bit offended. If not, I'll resume work at some point in the near to medium term future.

ngraham updated this revision to Diff 21871.Nov 4 2017, 4:42 PM

Improve comment for clarity

ngraham edited the summary of this revision. (Show Details)Nov 4 2017, 6:35 PM
ngraham edited the test plan for this revision. (Show Details)
dfaure added a comment.Sat, Dec 2, 3:50 PM

I don't get it. Which parts of this commit depend upon Qt 5.10? I only see buff.birthTime_ being used.

Unless I'm misunderstanding everything, birthTime_ is only set to a nonzero value as of https://github.com/qt/qtbase/commit/d3393ce25833c0afd7f0fa6b85fd6f3bd7ad520a#diff-e061e747264146cf47a86b371e512348R363, which only made it into Qt 5.10 and was not backported to 5.9.

dfaure added inline comments.Sat, Dec 2, 4:11 PM
src/ioslaves/file/file.cpp
783

... but we fill buff using lstat(), not using QFile, so it doesn't matter what QFile does.

dfaure added a comment.Sat, Dec 2, 4:24 PM

This patch doesn't actually directly use statx(); it uses the existing approach, which goes through QFile, which is used to populate the statbuf object that we query.

Can you show me where this code uses QFile to populate the statbuf object? One of us is missing something here.

One of us is missing something here.

I'm sure it's me. :-)
If this approach doesn't work, would you be able to offer some pointers regarding a better method of implementing it?

dfaure added a comment.Sat, Dec 2, 7:20 PM

Well, this hasn't been tested, has it?

AFAIU this requires calling statx(..., AT_SYMLINK_NOFOLLOW, ...) instead of lstat, on systems where statx is available.