Differential D20967

[UserMetaData] Add method to query which attributes are set
ClosedPublic
Actions

Authored by bruns on May 2 2019, 5:58 PM.

Details

Reviewers
astippich
ngraham
Group Reviewers
Baloo
Frameworks
Commits
R286:bbb05dfe7f9f: [UserMetaData] Add method to query which attributes are set
Summary

Many files have no UserMetaData attributes set at all, or only a subset
of the available ones. Allow to bulk query which attributes are set to
short cut the one-by-one query.

Test Plan

ctest (on Linux)

Code paths for Mac and FreeBSD are fully implemented, but untested. The
Windows implementation is mostly a stub, but functional conforming with
the documentation.

Diff Detail

Repository
R286 KFileMetaData
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12631
Build 12649: arc lint + arc unit
bruns created this revision.May 2 2019, 5:58 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMay 2 2019, 5:58 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bruns requested review of this revision.May 2 2019, 5:58 PM
arrowd added a subscriber: arrowd.Jun 10 2019, 7:40 AM
arrowd added inline comments.
src/xattr_p.h
204 ↗(On Diff #57390)
error: use of undeclared identifier 'attributeName'
231 ↗(On Diff #57390)

Ditto.

Also, size should be r, probably, see below.

234 ↗(On Diff #57390)
error: use of undeclared identifier 'r'
bruns updated this revision to Diff 59500.Jun 10 2019, 10:53 AM

fix BSD code

bruns marked 2 inline comments as done.Jun 10 2019, 10:54 AM
bruns added inline comments.
src/xattr_p.h
231 ↗(On Diff #57390)

Why ditto?

bruns updated this revision to Diff 59502.Jun 10 2019, 10:55 AM

update since

arrowd added a comment.Jun 10 2019, 11:02 AM

It compiles now, but the test fails:

********* Start testing of UserMetaDataWriterTest *********
Config: Using QtTest library 5.12.2, Qt 5.12.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by Clang 8.0.0 (tags/RELEASE_800/final 356365))
PASS   : UserMetaDataWriterTest::initTestCase()
FAIL!  : UserMetaDataWriterTest::test() 'md.queryAttributes(UserMetaData::Attribute::All) & UserMetaData::Attribute::Tags' returned FALSE. ()
   Loc: [/home/arr/projects/kfilemetadata/autotests/usermetadatawritertest.cpp(55)]
PASS   : UserMetaDataWriterTest::cleanupTestCase()
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 1ms
********* Finished testing of UserMetaDataWriterTest *********

I also had to do this to enable the test:

diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index 49824ba..994c7bc 100644
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -220,7 +220,7 @@ endif()
 #
 # UserMetaData
 #
-if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+if(UNIX)
     kde_enable_exceptions()
     ecm_add_test(usermetadatawritertest.cpp ../src/usermetadata.cpp
         TEST_NAME "usermetadatawritertest"
src/xattr_p.h
231 ↗(On Diff #57390)

Right, no need to change there.

bruns added a comment.Jun 10 2019, 11:10 AM

Apparently, the usermetadatawritertest was not executed on *BSD before, can you check if the test succeeds on the old code?

Please submit a PR with the Linux -> UNIX change, and make this one a dependency of it.

bruns added inline comments.Jun 10 2019, 11:14 AM
src/xattr_p.h
206 ↗(On Diff #57390)

@arrowd - please add a qDebug() << "extattr_list size:" << size; here

232 ↗(On Diff #57390)

@arrowd - please add a qDebug() << "extattr_list r:" << r << data; here

arrowd added a comment.Jun 10 2019, 11:22 AM
In D20967#477314, @bruns wrote:

Apparently, the usermetadatawritertest was not executed on *BSD before, can you check if the test succeeds on the old code?

Yep, it works on master.

The output with qDebug()s:

PASS   : UserMetaDataWriterTest::initTestCase()
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list r: 14 "\ruser.xdg.tags"
FAIL!  : UserMetaDataWriterTest::test() 'md.queryAttributes(UserMetaData::Attribute::All) & UserMetaData::Attribute::Tags' returned FALSE. ()
   Loc: [/home/arr/projects/kfilemetadata/autotests/usermetadatawritertest.cpp(55)]
bruns added a comment.EditedJun 10 2019, 11:33 AM
In D20967#477318, @arrowd wrote:
PASS   : UserMetaDataWriterTest::initTestCase()
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list r: 14 "\ruser.xdg.tags"
FAIL!  : UserMetaDataWriterTest::test() 'md.queryAttributes(UserMetaData::Attribute::All) & UserMetaData::Attribute::Tags' returned FALSE. ()
   Loc: [/home/arr/projects/kfilemetadata/autotests/usermetadatawritertest.cpp(55)]

Unfortunately, the returned format is different:
https://www.freebsd.org/cgi/man.cgi?query=extattr_list_file&sektion=2&apropos=0&manpath=FreeBSD+10.0-RELEASE

Each list entry consists of a single byte containing the length of the attribute name, followed by the attribute name. The attribute name is not terminated by ASCII 0 (nul).

bruns updated this revision to Diff 59505.Jun 10 2019, 11:47 AM

add and use split helper for BSD

arrowd added a comment.Jun 10 2019, 11:52 AM

Now the test passes!

src/xattr_p.h
194 ↗(On Diff #57390)
variable 'pos' is uninitialized
bruns updated this revision to Diff 59509.Jun 10 2019, 12:01 PM

initialize pos

bruns marked 6 inline comments as done.Jun 10 2019, 12:02 PM
astippich accepted this revision.Jun 10 2019, 9:15 PM

for the linux part

This revision is now accepted and ready to land.Jun 10 2019, 9:15 PM
bruns added a comment.Jun 14 2019, 12:05 AM

FreeBSD and WIndows folks had their chance to speak up - Windows will just have to wait for the CI ...
FreeBSD has been reviewed, @arrowd - Thanks!

Closed by commit R286:bbb05dfe7f9f: [UserMetaData] Add method to query which attributes are set (authored by bruns). · Explain WhyJun 14 2019, 12:10 AM
This revision was automatically updated to reflect the committed changes.
bruns added a comment.Jun 15 2019, 9:26 PM

Windows - thanks for ignoring every review request!

bcooksley added a comment.Jun 15 2019, 9:37 PM

Please note that there are very few people involved with Windows builds (despite the number of projects that make use of them in general) so people may not always have the ability to respond to every review request.
That is why the CI system monitors the state of code and can catch issues as they happen.

bruns added a comment.Jun 15 2019, 10:18 PM
In D20967#480338, @bcooksley wrote:

Please note that there are very few people involved with Windows builds (despite the number of projects that make use of them in general) so people may not always have the ability to respond to every review request.
That is why the CI system monitors the state of code and can catch issues as they happen.

If there are OS specific changes, and they are added to the subscribers list, they damn should. The CI catches some errors, but not everything.

Also, this code is not covered by the unit tests on Windows, so even when the CI shows green this just means it compiles.

Showing Only Differences

This revision modifies 3 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

PathPackages
Msrc/xattr_p.h (126 lines)

Diff 59500

View Options

src/xattr_p.h

Loading...