D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
smithjd added a comment. 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? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
This revision was automatically updated to reflect the committed changes. Closed by commit R286:649555ee3182: Rewrite the taglib extractor to use the generic PropertyMap interface (authored by astippich). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D18826?vs=53587=53599#toc REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=53587=53599 REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES autotests/taglibextractortest.cpp src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
bruns accepted this revision. bruns added a comment. This revision is now accepted and ready to land. Thanks, looks good to me now. REPOSITORY R286 KFileMetaData BRANCH arcpatch-D18826 REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich updated this revision to Diff 53587. astippich marked an inline comment as done. astippich added a comment. - spelling fixes REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=53585=53587 BRANCH arcpatch-D18826 REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich marked 3 inline comments as done. astippich added inline comments. INLINE COMMENTS > bruns wrote in taglibextractor.cpp:364 > This intermediate list is not required, you can directly call result->add() > for each attribute in lstASF. 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. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich updated this revision to Diff 53585. astippich added a comment. - update REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=52890=53585 BRANCH arcpatch-D18826 REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> taglibextractor.cpp:364
> +lstASF = asfTags->attribute("Author");
> +QStringList authors;
> for (const auto& attribute : qAsConst(lstASF)) {
This intermediate list is not required, you can directly call result->add() for
each attribute in lstASF.
> taglibextractor.cpp:376
> lstASF = asfTags->attribute("WM/Writer");
> +QStringList lyricists;
> for (const auto& attribute : qAsConst(lstASF)) {
This intermediate list is not required, you can directly call `result->add()`
for each attribute in lstASF.
> taglibextractor.cpp:392
> +if (!lstASF.isEmpty()) {
> +const auto attribute = lstASF.front();
> +result->add(Property::Publisher,
> TStringToQString(attribute.toString()).trimmed());
Why only the first element?
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18826
To: astippich, ngraham, bruns, mgallien
Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov,
michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich added a comment. ping REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibextractor.cpp:208
> Can you also use the same style as above, i.e. `const auto` and `for( : )`?
Oh, damn copy
> smithjd wrote in taglibextractor.cpp:195
> lstASF = asfTags->attribute("WM/Writer");
> ...
>
> The existing code does this.
I wanted to avoid the extra querying, but apparently I already did the same for
other special cases, so I did as you suggested
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18826
To: astippich, ngraham, bruns, mgallien
Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh,
astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich updated this revision to Diff 52890. astippich marked 8 inline comments as done. astippich added a comment. - rebase on master - implement review comments - cleanup includes REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=52679=52890 BRANCH rewrite_taglib_extractor REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
bruns added inline comments.
INLINE COMMENTS
> taglibextractor.cpp:184
> +if (savedProperties.contains("COMPOSER")) {
> +const auto composersString =
> TStringToQString(savedProperties["COMPOSER"].toString(";")).trimmed();
> +const auto composers = contactsFromString(composersString);
doubled space
> taglibextractor.cpp:208
> +QStringList conductors = contactsFromString(conductorString);
> +foreach(const QString& arr, conductors) {
> +result->add(Property::Conductor, arr);
Can you also use the same style as above, i.e. `const auto` and `for( : )`?
> taglibextractor.cpp:215
> +QStringList arrangers = contactsFromString(arrangerString);
> +foreach(const QString& arr, arrangers) {
> +result->add(Property::Arranger, arr);
dito, and below ...
> taglibextractor.cpp:285
> +/* Special handling because TagLib::PropertyMap matches "TPUB" to "LABEL"
> + * Insert manually for Publisher */
> +lstID3v2 = Id3Tags->frameListMap()["TPUB"];
`*/` on a separate line
> taglibextractor.cpp:300
> + a 5 stars rating to a range of 0-255 for MP3.
> + Match it to baloo rating with a range of 0 - 10 */
> +lstID3v2 = Id3Tags->frameListMap()["POPM"];
`*/` on separate line, leading `*` on other lines
> taglibextractor.cpp:300
> + a 5 stars rating to a range of 0-255 for MP3.
> + Match it to baloo rating with a range of 0 - 10 */
> +lstID3v2 = Id3Tags->frameListMap()["POPM"];
s/Match/Map/
> taglibextractor.cpp:330
> + with a range of 0 to 100 (stored in steps of 10) and make it
> compatible
> + with baloo rating with a range from 0 to 10 */
> +TagLib::MP4::ItemListMap::Iterator itRating = allTags.find("rate");
dito
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18826
To: astippich, ngraham, bruns, mgallien
Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh,
astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
smithjd added inline comments.
INLINE COMMENTS
> astippich wrote in 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.
lstASF = asfTags->attribute("WM/Writer");
...
The existing code does this.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18826
To: astippich, ngraham, bruns, mgallien
Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh,
astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich updated this revision to Diff 52679. astippich added a comment. - remove now unnecessary include REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=52675=52679 BRANCH rewrite_taglib_extractor REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich marked an inline comment as done. astippich added inline comments. INLINE COMMENTS > smithjd wrote in taglibextractor.cpp:195 > Can this be moved into the asf-specific extractions? 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. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich updated this revision to Diff 52675. astippich added a comment. - return early if map is empty REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18826?vs=51128=52675 BRANCH rewrite_taglib_extractor REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
smithjd added inline comments.
INLINE COMMENTS
> taglibextractor.cpp:109
> +{
> +if (savedProperties.contains("TITLE")) {
> +result->add(Property::Title,
> TStringToQString(savedProperties["TITLE"].toString()).trimmed());
This could return early if the property map is empty.
> taglibextractor.cpp:195
> }
> -
> -itApe = lstApe.find("PERFORMER");
> -if (itApe != lstApe.end()) {
> -if (!data.performer.isEmpty()) {
> -data.performer += ", ";
> +/* Lyricist is called "WRITER" for wma/asf files */
> +if (savedProperties.contains("WRITER")) {
Can this be moved into the asf-specific extractions?
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18826
To: astippich, ngraham, bruns, mgallien
Cc: smithjd, kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh,
astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich added a comment. ping. I know this is quite a large diff, but it fixes a potential crash REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich edited the test plan for this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18826 To: astippich, ngraham, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18826: Rewrite the taglib extractor to use the generic PropertyMap interface
astippich created this revision. astippich added reviewers: ngraham, bruns, mgallien. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. astippich requested review of this revision. REVISION 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 REPOSITORY R286 KFileMetaData BRANCH rewrite_taglib_extractor REVISION DETAIL https://phabricator.kde.org/D18826 AFFECTED FILES src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, ngraham, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
