Details

Reviewers

ngraham
bruns
mgallien

Commits

R286:649555ee3182: Rewrite the taglib extractor to use the generic PropertyMap interface

Summary

Rewrite the taglib extractor to use taglib's
PropertyMap. Since this largely unifies the handling of the
different tag formats, but not quite, a lot of code is removed.
The resulting code is also faster. Additionally, this avoids the
usage of a FileRef object, which fixes a potential crash due to
a known bug in taglib.

BUG: 403902

Test Plan

all tests pass

Diff Detail

Repository

R286 KFileMetaData

Branch

rewrite_taglib_extractor

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 8001
Build 8019: arc lint + arc unit

astippich created this revision.Feb 7 2019, 8:05 PM

Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptFeb 7 2019, 8:05 PM

Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript

astippich requested review of this revision.Feb 7 2019, 8:05 PM

Harbormaster completed remote builds in B8001: Diff 51128.Feb 7 2019, 8:06 PM

astippich edited the test plan for this revision. (Show Details)Feb 7 2019, 8:06 PM

ping. I know this is quite a large diff, but it fixes a potential crash

astippich mentioned this in D19109: [Extractor] Add metadata to extractors.Feb 19 2019, 8:57 PM

astippich mentioned this in D16579: Remove support for non-standard APE tag field names from the test files.Feb 23 2019, 3:26 PM

smithjd added a subscriber: smithjd.Feb 25 2019, 4:00 AM

smithjd added inline comments.

src/extractors/taglibextractor.cpp
109	This could return early if the property map is empty.
195	Can this be moved into the asf-specific extractions?

return early if map is empty

Harbormaster completed remote builds in B8900: Diff 52675.Feb 26 2019, 8:30 PM

astippich marked an inline comment as done.Feb 26 2019, 8:35 PM

astippich added inline comments.

src/extractors/taglibextractor.cpp
195	I wanted to do so first, but that will require to also put the PropertyMap into the extractAsfTags method, which I think is not worth it.

remove now unnecessary include

Harbormaster completed remote builds in B8904: Diff 52679.Feb 26 2019, 8:36 PM

smithjd added inline comments.Feb 27 2019, 1:09 AM

src/extractors/taglibextractor.cpp
195	lstASF = asfTags->attribute("WM/Writer"); ... The existing code does this.

bruns added inline comments.Feb 28 2019, 2:34 AM

src/extractors/taglibextractor.cpp
279	doubled space
281	Can you also use the same style as above, i.e. `const auto` and `for( : )`?
281	dito, and below ...
283	`*/` on a separate line
298	`/` on separate line, leading `` on other lines
298	s/Match/Map/
328	dito

rebase on master
implement review comments
cleanup includes

Harbormaster completed remote builds in B9017: Diff 52890.Mar 1 2019, 10:35 AM

astippich added inline comments.Mar 1 2019, 10:35 AM

src/extractors/taglibextractor.cpp
195	I wanted to avoid the extra querying, but apparently I already did the same for other special cases, so I did as you suggested
281	Oh, damn copy&paste

astippich mentioned this in D18601: Rewrite taglib writer to use property interface.Mar 1 2019, 11:20 AM

astippich mentioned this in D19442: [Kfilemetada] Make it compile without foreach.Mar 1 2019, 12:27 PM

ping

bruns requested changes to this revision.Mar 8 2019, 4:56 PM

bruns added inline comments.

src/extractors/taglibextractor.cpp
358	This intermediate list is not required, you can directly call `result->add()` for each attribute in lstASF.
358	This intermediate list is not required, you can directly call result->add() for each attribute in lstASF.
371	Why only the first element?

This revision now requires changes to proceed.Mar 8 2019, 4:56 PM

update

Harbormaster completed remote builds in B9430: Diff 53585.Mar 10 2019, 10:00 AM

astippich marked 3 inline comments as done.Mar 10 2019, 10:04 AM

astippich added inline comments.

src/extractors/taglibextractor.cpp
358	After further investigation there is no need to look for more than one entry, since there cannot be duplicated keys. This is also what TagLib does internally. So I changed it to be more inline with the generic extraction.

spelling fixes

Harbormaster completed remote builds in B9432: Diff 53587.Mar 10 2019, 10:08 AM

Thanks, looks good to me now.

This revision is now accepted and ready to land.Mar 10 2019, 2:42 PM

bruns mentioned this in D19655: [kfilemetadata] Make it compile without foreach.Mar 10 2019, 2:51 PM

Closed by commit R286:649555ee3182: Rewrite the taglib extractor to use the generic PropertyMap interface (authored by astippich). · Explain WhyMar 10 2019, 3:02 PM

This revision was automatically updated to reflect the committed changes.

This test fails:

FAIL! : TagLibExtractorTest::testMp4(mp4) Compared values are not the same

Actual   (resultMp4.properties().value(Property::Rating).toInt()): 0
Expected (8)                                                     : 8
Loc: [/home/enderw/Code/kfilemetadata/autotests/taglibextractortest.cpp(381)]

Is this a new failure?

			Path	Packages
M			src/extractors/taglibextractor.h (44 lines)
M			src/extractors/taglibextractor.cpp (1133 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	51128	6e449d4		Feb 7 2019, 8:05 PM	★	★
Diff 2	52675	6e449d4	- return early if map is empty	Feb 26 2019, 8:30 PM	★	★
Diff 3	52679	6e449d4	- remove now unnecessary include	Feb 26 2019, 8:36 PM	★	★
Diff 4	52890	1aa7f91	- rebase on master	Mar 1 2019, 10:35 AM	★	★
Diff 5	53585	1aa7f91	- update	Mar 10 2019, 10:00 AM	★	★
Diff 6	53587	1aa7f91	- spelling fixes	Mar 10 2019, 10:08 AM	★	★
Diff 7	53599	2585edd	R286:649555ee31820af01869c7bfe8c1e96e5a9abb37	Mar 10 2019, 3:02 PM	★	★

Rewrite the taglib extractor to use the generic PropertyMap interface
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 51128

src/extractors/taglibextractor.h

src/extractors/taglibextractor.cpp

Rewrite the taglib extractor to use the generic PropertyMap interfaceClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 51128

src/extractors/taglibextractor.h

src/extractors/taglibextractor.cpp

Rewrite the taglib extractor to use the generic PropertyMap interface
ClosedPublic
Actions

Revision Contents
Changeset List