Web lists-archives.com

Re: Review Request 129839: KFileMetaData: add a new property DiscNumber for audio files from multi-disc albums




This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129839/

On February 5th, 2017, 1:09 p.m. UTC, David Faure wrote:

src/properties.h (Diff revision 2)
264
    LastProperty = PropertyCount-1
264
    LastProperty = PropertyCount-1,

Count is supposed to be the count, and Last to be the last. Any reason why you're not moving DiscNumber to be before PropertyCount, as intended by this code? (remove the "= ..." value)

On February 5th, 2017, 1:35 p.m. UTC, Matthieu Gallien wrote:

To me, this seems to be part of public API of kfilemetadata and I thought that changin the value of the enum could be a breach of the binary compatibility. The only code I have found that uses this, is an automatic test of kfilemetadata that checks that the properties are declared correctly inside the library.

Fair point. Although not really a breakage, just a change of value of a public enum, to accomodate the change. An hypothetic application using LastProperty or PropertyCount, that hasn't been recompiled yet would still see the old value (and therefore ignore the new enum value), while after recompiling it would see the new enum value. Seems fine to me. I think it's valid to adjust what is clearly dependent on the number of enum values (but now I realize why Qt doesn't have this kind of stuff anywhere). Maybe add a KF6 TODO to remove Last and Count from the public API. The implementation (and autotest) can use a locally defined value instead.


- David


On February 5th, 2017, 1:32 p.m. UTC, Matthieu Gallien wrote:

Review request for Baloo.
By Matthieu Gallien.

Updated Feb. 5, 2017, 1:32 p.m.

Repository: kfilemetadata

Description

the new property is appended at the end of the existing enums such that binary compatibility is kept. The special values at the end of the enums are currently only used by automatic tests of KFileMetaData. There should be no harm by this commit.

At time of this commit, lxr.kde.org shows no user of the special values at the end of the enum. My patch should not cause any problems.

One interesting question is how to manage caching of new properties in Baloo when one modify KFileMetaData. I currently have no idea.

There should be more patches to add new properties after this review if needed (some music related properties are still missing).

Testing

Automatic tests are still all OK on my setup.

I have modified my music player to makes use of it and it works.

I have not yet any clear idea how to makes Baloo reindex the files to cache new properties.

Diffs

  • autotests/taglibextractortest.cpp (d46e49ea6a189d16459799100ec49480bed893c3)
  • src/extractors/taglibextractor.cpp (8fcad93ca4fc6572a412c1f729d1ef361dd7e8cf)
  • src/properties.h (1763b9bfa4a250231932e588edbd6bebc4af3f0a)
  • src/propertyinfo.cpp (97003ae70c683eb73e2ecd84899ae35d29edaefc)

View Diff