D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:982ebe58b3c8: Rewrite taglib writer to use property 
interface (authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18601?vs=55125&id=55426#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18601?vs=55125&id=55426

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  rewrite_taglib2

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich updated this revision to Diff 55125.
astippich added a comment.


  - check that file is valid
  - rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18601?vs=50704&id=55125

BRANCH
  rewrite_taglib2

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwriter.cpp:70
> > Taglib does not "handle this nicely". For APE and Xiph, it just accepts 
> > *any* unknown key and uses it verbatim, while for MP4 and ASF it rejects 
> > any unknown key.
> 
> And so do APE::Tag::setItem and OGG::XiphComment::addField ...? btw, also 
> perfectly legal, APE and Xiph allow writing arbitrary tags, while the others 
> do not.
> 
> > The setProperties() is also quite inconsistent, for APE and ASF it only 
> > removes items which have an empty value, while for Xiph, the properties are 
> > completely replaced.
> 
> Xiph explicitly allows multiple entries per key, which need to be removed 
> when writing.
> 
> > As soon as you add support for a property where APE and Xiph key naming 
> > differs, or is only supported by one, you will require two functions anyway.
> 
> TagLib automatically translates different keys from APE to "common names", 
> e.g. DISC->DISCNUMBER etc.
> 
> I would really like to hand off manual tag handling to TagLib as much as 
> possible. The library solely responsible for reading tags usually knows 
> better how to handle the tags than we do (with a few exceptions to the rule 
> of course).

By using the type specific function you signal you are aware of the differences 
between the two, and supply the appropriate data.

RATING is **not handled** by the properties interface, it just works by 
coincidence, not by design.

In case Taglib properly handles a tag, I am not against using it, as said 
several times. This is the case for DISC, but not for RATING.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:70
> Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* 
> unknown key and uses it verbatim, while for MP4 and ASF it rejects any 
> unknown key.
> 
> The setProperties() is also quite inconsistent, for APE and ASF it only 
> removes items which have an empty value, while for Xiph, the properties are 
> completely replaced.
> 
> As soon as you add support for a property where APE and Xiph key naming 
> differs, or is only supported by one, you will require two functions anyway.
> 
> Using `APE::Tag::setItem(...)` is as efficient as manipulating the key/value 
> in the Taglib::PropertyMap first and setting it by `setProperties(...)`. 
> Likewise for Xiph.
> 
> If you want to squeeze out the last bit of efficiency, you would skip the 
> `setProperties(...)` completely when no property is changed by 
> `writeGenericProperties()`. This happens e.g. if you only change the 
> rating.
> 
> If you want to avoid duplicate code, move the 
> `properties()`/`setProperties()` into `writeGenericProperties()`, that saves 
> 12*2 lines and adds 2.

> Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* 
> unknown key and uses it verbatim, while for MP4 and ASF it rejects any 
> unknown key.

And so do APE::Tag::setItem and OGG::XiphComment::addField ...? btw, also 
perfectly legal, APE and Xiph allow writing arbitrary tags, while the others do 
not.

> The setProperties() is also quite inconsistent, for APE and ASF it only 
> removes items which have an empty value, while for Xiph, the properties are 
> completely replaced.

Xiph explicitly allows multiple entries per key, which need to be removed when 
writing.

> As soon as you add support for a property where APE and Xiph key naming 
> differs, or is only supported by one, you will require two functions anyway.

TagLib automatically translates different keys from APE to "common names", e.g. 
DISC->DISCNUMBER etc.

I would really like to hand off manual tag handling to TagLib as much as 
possible. The library solely responsible for reading tags usually knows better 
how to handle the tags than we do (with a few exceptions to the rule of course).

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwriter.cpp:70
> No, I cannot use the the PropertyMap for ASF/MP4, those atoms/attributes are 
> unsupported in the PropertyMap and need to be handled separately. I would 
> have done so if it is possible.
> I really do not like to write unnecessary, duplicated code when TagLib 
> handles this nicely for us. I've only chosen the rating property to be 
> implemented first, but there are more to come. The code will be 100% the same 
> for APE and OGG with the PropertyMap, and also more efficient as we query the 
> PropertyMap anyway.

Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* 
unknown key and uses it verbatim, while for MP4 and ASF it rejects any unknown 
key.

The setProperties() is also quite inconsistent, for APE and ASF it only removes 
items which have an empty value, while for Xiph, the properties are completely 
replaced.

As soon as you add support for a property where APE and Xiph key naming 
differs, or is only supported by one, you will require two functions anyway.

Using `APE::Tag::setItem(...)` is as efficient as manipulating the key/value in 
the Taglib::PropertyMap first and setting it by `setProperties(...)`. Likewise 
for Xiph.

If you want to squeeze out the last bit of efficiency, you would skip the 
`setProperties(...)` completely when no property is changed by 
`writeGenericProperties()`. This happens e.g. if you only change the rating.

If you want to avoid duplicate code, move the `properties()`/`setProperties()` 
into `writeGenericProperties()`, that saves 12*2 lines and adds 2.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:70
> I think for **reading** it as generic as possible is fine, but for writing 
> being a little bit more explicit does not hurt.
> 
> My proposal for Ape and Ogg is to split the writing for these to two 
> different functions. Yes, it **is** possible to use the propertymap for both, 
> as both use the same scale and the same tag name, but the implementations on 
> the taglib side are completely different.
> 
> Writing the "rating" for XiphComment (Ogg) and  Ape in distinct functions 
> (see my comment in D18604 ) hardly adds 
> any code, but gets the Ape and Ogg code paths in line with the other file 
> formats. You don't write the ASF/MP4 rating using the property interface 
> although it would be possible, and IMHO thats the right thing to do,  also 
> for Ape and Ogg.
> 
> As soon Ape and Ogg are split, you no longer rely on the PropertyMap for the 
> rating, and you won't have to use `properties()`/`setProperties()` twice.

No, I cannot use the the PropertyMap for ASF/MP4, those atoms/attributes are 
unsupported in the PropertyMap and need to be handled separately. I would have 
done so if it is possible.
I really do not like to write unnecessary, duplicated code when TagLib handles 
this nicely for us. I've only chosen the rating property to be implemented 
first, but there are more to come. The code will be 100% the same for APE and 
OGG with the PropertyMap, and also more efficient as we query the PropertyMap 
anyway.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwriter.cpp:70
> Not sure I understand. I would like to avoid doing `file->properties();` and 
> `file->setProperties();` twice for those formats for which only the 
> PropertyMap is sufficient, e.g. Ape and Ogg, to avoid doing any unnecessary 
> work.  The beauty of Ape and Ogg in the TagLib implementation is that they 
> solely work with the PropertyMap and do not require any special handling. See 
> D18826 , Ape and Ogg do not have any 
> extra codepath at all. For writing tags, we have to be a little bit more 
> careful, though. If writing Rating information is added to 
> writeGenericProperties, for example Id3v2's "TXXX" tags will be polluted with 
> these values.

I think for **reading** it as generic as possible is fine, but for writing 
being a little bit more explicit does not hurt.

My proposal for Ape and Ogg is to split the writing for these to two different 
functions. Yes, it **is** possible to use the propertymap for both, as both use 
the same scale and the same tag name, but the implementations on the taglib 
side are completely different.

Writing the "rating" for XiphComment (Ogg) and  Ape in distinct functions (see 
my comment in D18604 ) hardly adds any 
code, but gets the Ape and Ogg code paths in line with the other file formats. 
You don't write the ASF/MP4 rating using the property interface although it 
would be possible, and IMHO thats the right thing to do,  also for Ape and Ogg.

As soon Ape and Ogg are split, you no longer rely on the PropertyMap for the 
rating, and you won't have to use `properties()`/`setProperties()` twice.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:70
> So **iff** the rating is updated at the same time as another property, taglib 
> has to adjust some in-memory structure twice. Obviously, for most formats 
> this poses no problem, but for Ape/Vorbis it does?
> 
> Also, this no longer applies when you use the specific tag types for Ape/Ogg.
> 
> Writing to disk only happens when calling `file->save()`...

Not sure I understand. I would like to avoid doing `file->properties();` and 
`file->setProperties();` twice for those formats for which only the PropertyMap 
is sufficient, e.g. Ape and Ogg, to avoid doing any unnecessary work.  The 
beauty of Ape and Ogg in the TagLib implementation is that they solely work 
with the PropertyMap and do not require any special handling. See D18826 
, Ape and Ogg do not have any extra 
codepath at all. For writing tags, we have to be a little bit more careful, 
though. If writing Rating information is added to writeGenericProperties, for 
example Id3v2's "TXXX" tags will be polluted with these values.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwriter.cpp:70
> That will then require to load and write the property map twice when 
> properties only specific to some tagging formats need to be written, see e.g. 
> Ape and Vorbis tags in D18604 . I would 
> like to avoid this.

So **iff** the rating is updated at the same time as another property, taglib 
has to adjust some in-memory structure twice. Obviously, for most formats this 
poses no problem, but for Ape/Vorbis it does?

Also, this no longer applies when you use the specific tag types for Ape/Ogg.

Writing to disk only happens when calling `file->save()`...

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added a comment.


  ping

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:70
> When you make this
> `void writeGenericProperties(Taglib::File *file, const PropertyMap 
> &newProperties)`, you can do `file->properties(); {/* merge */}; 
> file->setProperties(...);` here, saving most of the duplicate code below.
> 
> dito for the specializations in D18604 , 
> just pass in `Taglib::File*`, and call `auto tags = 
> dynamic_cast(file->tag());` there.

That will then require to load and write the property map twice when properties 
only specific to some tagging formats need to be written, see e.g. Ape and 
Vorbis tags in D18604 . I would like to 
avoid this.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> taglibwriter.cpp:70
>  
> -if (properties.contains(Property::Title)) {
> -title = 
> QStringToTString(properties.value(Property::Title).toString());
> -tags->setTitle(title);
> +void writeGenericProperties(TagLib::PropertyMap &oldProperties, const 
> PropertyMap &newProperties)
> +{

When you make this
`void writeGenericProperties(Taglib::File *file, const PropertyMap 
&newProperties)`, you can do `file->properties(); {/* merge */}; 
file->setProperties(...);` here, saving most of the duplicate code below.

dito for the specializations in D18604 , 
just pass in `Taglib::File*`, and call `auto tags = 
dynamic_cast(file->tag());` there.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added a comment.


  In D18601#402905 , @bruns wrote:
  
  > I think this becomes better structured when you:
  >
  > 1. Create one function per file type, with parameters (Taglib::Filestream, 
KFM::PropertyMap)
  > 2. From this function, call a generic `updateProperties(oldProperties, 
newProperties) -> mergedProperties`
  > 3. Call the type specific function from TaglibWriter::write(...)
  >
  >   Especially when taking the changes for writing the rating into account, 
this would make the code easier to read - handling of different types just once 
(not once for reading and once for writing), and no upcasting/dynamic_cast of 
Taglib::File*.  It also saves the heap allocation of the concrete TagLib::File 
implementation.
  
  
  Done. It requires a little bit more of boilerplate code, but I have not 
strong preference.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich updated this revision to Diff 50704.
astippich added a comment.


  - rewrite for better readability and to avoid heap allocation

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18601?vs=50497&id=50704

BRANCH
  rewrite_taglib2

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added a comment.


  I think this becomes better structured when you:
  
  1. Create one function per file type, with parameters (Taglib::Filestream, 
KFM::PropertyMap)
  2. From this function, call a generic `updateProperties(oldProperties, 
newProperties) -> mergedProperties`
  3. Call the type specific function from TaglibWriter::write(...)
  
  Especially when taking the changes for writing the rating into account, this 
would make the code easier to read - handling of different types just once (not 
once for reading and once for writing), and no upcasting/dynamic_cast of 
Taglib::File*.  It also saves the heap allocation of the concrete TagLib::File 
implementation.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> taglibwriter.cpp:123
>  
> -TagLib::Tag* tags = file.tag();
> +TagLib::File *file = openFile(&stream, mimeType);
> +if (!file) {

use a smart pointer (unique_ptr, QScopedPtr) here, and get rid of the delete 
below.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Nathaniel Graham]

ngraham added reviewers: broulik, cfeck.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added a dependent revision: D18603: Implement more tags for taglib 
writer.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: 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 writer to use taglib's
  unified property interface, which is more extensible.

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  rewrite_taglib

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams