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

[David Faure]

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




src/extractors/taglibextractor.cpp (line 111)
<https://git.reviewboard.kde.org/r/129839/#comment68306>

    I would have initialized the int to -1, and used that as the invalid value (surely a disc number is never -1). It removes the risk of forgetting to set the bool to true in one code path.
    But no big deal, not a blocker issue.



src/properties.h (line 264)
<https://git.reviewboard.kde.org/r/129839/#comment68305>

    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)


- David Faure


On Feb. 5, 2017, 1:02 p.m., Matthieu Gallien wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129839/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2017, 1:02 p.m.)
> 
> 
> Review request for Baloo and KDE Frameworks.
> 
> 
> 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).
> 
> 
> Diffs
> -----
> 
>   autotests/taglibextractortest.cpp d46e49ea6a189d16459799100ec49480bed893c3 
>   src/extractors/taglibextractor.cpp 8fcad93ca4fc6572a412c1f729d1ef361dd7e8cf 
>   src/properties.h 1763b9bfa4a250231932e588edbd6bebc4af3f0a 
>   src/propertyinfo.cpp 97003ae70c683eb73e2ecd84899ae35d29edaefc 
> 
> Diff: https://git.reviewboard.kde.org/r/129839/diff/
> 
> 
> 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.
> 
> 
> Thanks,
> 
> Matthieu Gallien
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170205/dfaeadf6/attachment-0001.html>