D11360: provide sample rate in kHz

[Alexander Stippich]

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


  - use qlatin1string for comparison and do not display trailing zeros

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11360?vs=29621=29662

BRANCH
  sample_rate

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

AFFECTED FILES
  src/widgetfactory.cpp

To: astippich, #frameworks, #baloo
Cc: elvisangelaccio, ashaposhnikov, astippich, spoorun, nicolasfella, alexeymin

D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:157
> This is an example for what we're discussing. Why the join??? We might lose 
> info, when the album artist contains a comma.

Yeah, I've seen that as well. Looks unnecessary, since we could also just 
directly add it to a stringlist. This is something I would like to change (at 
least for the taglibextractor)

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks, astippich
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:331
> You mean other than lines 225, 233, 240?

Yes, but imho we shouldn't even enter the function for mimetypes not having ogg 
tags. Maybe that is overly careful

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks, astippich
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D10694: epubextractor: Handle multiple subjects better

[Alexander Stippich]

astippich added a comment.


  In D10694#221734 , @mgallien wrote:
  
  > In D10694#221719 , @michaelh 
wrote:
  >
  > > This is bad! I have learned baloo itself doesn't handle stringlists. 
Which in my view would be the natural way to handle token-like items like tags, 
keywords and subject(s). Until this is changed in baloo I'm putting this diff 
on hold and fro the time being work around these long subject-strings within 
baloo-widgets.
  >
  >
  > I believe this is quite the opposite. I am already getting strings list for 
some audio metadata. I would like to get your patch in given you make it works 
like already existing code and apart from the baloo-widgets code everybody 
should be fine with your modifications.
  >
  > > @mgallien: Did I understand you correctly, the refactoring helped you to 
understand it better? That would be an incentive for me to do more refactoring.
  >
  > This is this patch that made me read the code in baloo that store the 
properties returned by the extractors. This code is handling strings list quite 
fine and automatically when a property is added several times.
  
  
  Could you please point me to the code location? I would like to investigate 
how the code is used for D10803 .
  Also, since this discussion also applies for the taglibextractor: Is a string 
list preferred for new properties in KFileMetadata when multiple entries are 
possible? I think right now most of the properties are using a single 
concatenated string.

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, dfaure
Cc: astippich, #frameworks, ashaposhnikov, michaelh, spoorun, navarromorales, 
isidorov, nicolasfella, firef, andrebarros, alexeymin, emmanuelp

D10918: taglibextractor: Refactor for better readability

[Alexander Stippich]

astippich requested changes to this revision.
astippich added a comment.
This revision now requires changes to proceed.


  Looks good imho, just one small comment inline. of course, @mgallien as the 
maintainer must also give his ok.

INLINE COMMENTS

> taglibextractor.cpp:331
> +} else {
> +extractOgg(stream, mimeType, data);
>  }

I think we should check explicitly for the appropriate mimetypes here

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, mgallien, #baloo, #frameworks, astippich
Cc: astippich, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, bruns, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  @mgallien this should be safe to land now since KF 5.45 has been tagged, 
right?

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, bruns, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  ping. Can I push? I'm asking again because I don't want to mess up anything 
before the frameworks release, and this one contains string changes

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D12156: implement reading of rating tag

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin

D12156: implement reading of rating tag

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, michaelh.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  allows to read the rating stored in the metadata for idv2, vorbis and ape tags

REPOSITORY
  R286 KFileMetaData

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:7f9de32eedff: handle more tags in taglibextractor 
(authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D10803?vs=30956=31950#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30956=31950

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D12320: add ability to read embedded cover files

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, michaelh.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  read the embedded image of audio files as binary data

REPOSITORY
  R286 KFileMetaData

BRANCH
  cover

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

AFFECTED FILES
  autotests/samplefiles/no-meta/test.m4a
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12320: add ability to read embedded cover files

[Alexander Stippich]

astippich added a comment.


  This revision is only one possibility and is mainly there to start the 
discussion again on how to read the embedded picture data from e.g. audio 
files. Before this can land, Baloo has to be patched in order to cope with 
binary data.
  I think for Baloo there are two possibilities: (a) Put it in the database 
same as all other data or (b) ignore the binary data. In the latter case, 
applications would have to query the metadata separately in order to get the 
cover.
  For KFileMetaData I also see two possible implementation: Either in the 
taglibextractor, or in a completely new extractor that only reads the cover 
files (may be also useful for ebooks, but I don't know if such things exist 
there). If we agree on (a), I would opt to implement it in taglibextractor, if 
we chose (b) I think a new extractor makes sense such that applications would 
not have to read all the metadata again with the taglibextractor, but could 
specifically just read the picture data and get the rest from Baloo.
  
  Any other suggestions? Opinions?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D11874: change 24px view-media-artist icon

[Alexander Stippich]

astippich added a reviewer: andreask.
astippich added a comment.


  ping

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #vdg, andreask
Cc: #frameworks, michaelh, ngraham, bruns

D11820: Handle properties with multiple values

[Alexander Stippich]

astippich added a comment.


  Sorry for the late answer. Isn't this already handled in widgetfactory.cpp 
toString function (line 87)? During my testing with multiple values I've never 
had issues with baloo-widgets, only with baloo itself. If there are still 
changes required in baloo-widgets, I think they should happen inside the 
toString() function.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, #baloo, #frameworks
Cc: astippich, mgallien, elvisangelaccio, ashaposhnikov, spoorun, alexeymin

D11820: Handle properties with multiple values

[Alexander Stippich]

astippich added a comment.


  Seems to work just fine, see screenshot:
  F5808864: Screenshot_20180415_181027.png 

  
  I did a little bit more testing, and in case you didn't already know: It 
looks like baloo outputs properties with multiple values as a QVariantList 
(QList), and hence baloo-widgets with its test for QVariant::List 
should work just fine. I tested this with #elisa 
 and the values we get from baloo. But 
baloo doesn't work with lists as input. Luckily, QVariant::toStringList() works 
both with QStringList and QVariantList. I will work on a solution that accepts 
stringlists as an input for baloo. That should allow to solve the issues with 
all the property types in KFileMetaData. As it looks right now, there is a 
difference if one queries the properties via baloo (QVariantList) or directly 
via KFileMetaData (multiple entries with a single string).
  
  In D11820#246628 , @michaelh wrote:
  
  > > If there are still changes required in baloo-widgets, I think they should 
happen inside the toString() function.
  >
  > That could be done in `toString()`. The concatenation is only an 
intermediate step. The real plan is to use `TagWidget` for e.g. keywords, 
genres and so on, in that case I would have to move all the decision making 
back to where it is now.
  
  
  If it's only intermediary, then it's probably fine. Maybe add a comment.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, #baloo, #frameworks
Cc: bruns, astippich, mgallien, elvisangelaccio, ashaposhnikov, spoorun, 
alexeymin

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249951 , @michaelh wrote:
  
  > It's for elisa I guess, could you please elaborate how POPM/RATING is going 
to be used and why xattr are not applicable?
  
  
  It will be used as a fallback when there is no xattr rating set or available 
(e.g. on Windows) so that users who have rated their music with other players 
can still see their previous ratings. 
  It will also useful for managing ratings on other platforms when writing 
support is added.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

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


  - implement reading of half star ratings for mp3 and add tests

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=31987=32587

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  Those formulas didn't work with newly added tests with values from Winamp, 
Windows Media Player and MediaMonkey. I calculated a linear regression line and 
this one seems to work with some special case handling

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  code didn't get prettier, but that's the de-facto standard

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

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


  - simplify

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=32587=32591

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249223 , @mgallien wrote:
  
  > Thanks.
  >  Fix one issue and we should be good to go.
  >  Another nice thing is that we should have a portable way to have ratings 
in Elisa that could be read or write somewhere else.
  
  
  Writing the tags must be implemented first for that :)

INLINE COMMENTS

> mgallien wrote in taglibextractor.cpp:222-234
> You can and should be using all intermediate values. If I remember correctly, 
> in dolphin, you can set all values between 0 and 10. This is done by half 
> blue stars.
> Can you use modulo instead of embedded if ?

The thing here is that the most common implementation don't allow half star 
ratings. Only Mediamonkey does it and that diverges from the rest. Also, the 
values are not distributed equidistantly, but I'll look into a more flexible 
solution.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12320: add ability to read embedded cover files

[Alexander Stippich]

astippich added a comment.


  In D12320#249982 , @michaelh wrote:
  
  > -2
  >  https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure though.
  
  
  It is also disqualified by the fact that it is not in frameworks. I think a 
nice solution is to implement a separate "extractor" that is not an extractor 
plugin like taglib, epub, etc. but implemented like the xattr tags 
(usermetadata) as a separate, exported class. This way, baloo doesn't have to 
be changed in any way and still applications using kfilemetadata can query the 
cover files specifically.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249977 , @michaelh wrote:
  
  > In D12156#249971 , @astippich 
wrote:
  >
  > > In D12156#249951 , @michaelh 
wrote:
  > >
  > > > It's for elisa I guess, could you please elaborate how POPM/RATING is 
going to be used and why xattr are not applicable?
  > >
  > >
  > > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  > >  It will also useful for managing ratings on other platforms when writing 
support is added.
  >
  >
  > I that case I would suggest to use xattr only on systems that support it, 
and use POPM/RATING on those that don't. I afraid users will find it confusing 
to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not 
planned?
  >  Because extractors are called in sequence it would also be possible to 
create a dedicate taglibRatingExtractor (much easier to apply build 
conditionals).
  
  
  We still need the ability to read the tag, since users expect to see the 
rating in music players.
  For baloo-widgets I created D12157 

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  All of a sudden :)
  I've been quiet about this because I'd like to resolve the issues regarding 
strings/stringlists first, so if you have some spare time, please have a look 
at D11365 . I will rebase after this one 
landed

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 30955.
astippich marked an inline comment as done.
astippich added a comment.


  - fix formatting issues during rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30951=30955

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich marked 3 inline comments as done.
astippich added a comment.


  In D10803#237133 , @michaelh wrote:
  
  > Except for the white space stuff I'm still fine with this.
  
  
  Oops, sorry, messed that up during rebase.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:304
> Nitpick: These comments aren't really informative. The tagname is explicit 
> enough. 
> I don't mind if you leave them in.

You're right, I copied them from MP3s, but they make no sense for APE and OGG 
tags

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - rebase on top of master

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=28467=30950

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - remove duplicate genre property for mp3

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30955=30956

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - update value types

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30950=30951

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich planned changes to this revision.
astippich added a comment.


  In D11365#236274 , @michaelh wrote:
  
  > This patch should be split.
  >
  > 1. Test more properties
  > 2. Change return types of ...
  >
  >   Also 'fix errors' in the title is misleading because currently 
kfilemetadata works well.
  
  
  Sure, I can do that if it is not a problem that the new tests do not pass 
(temporarily). It is probably the best idea to create thorough tests for the 
way we'd like KFileMetaData/taglib to work, as they are lacking in several ways 
(and hence created the confusion I have had) . Then we can start fixing the 
errors.
  Can we reach a consent how it should behave in the end? e.g. should the 
result match the valueType in propertyinfo, should there be multiple properties 
with single strings or stringlists for multiple values? Any other concerns?
  @mgallien, what's your opinion here?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11874: change 24px view-media-artist icon

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: VDG.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  the 24px version of the view-media-artist icon is the only one that links to 
im-invisible-user, while lower resolutions link to im-user.
  change the 24px version accordingly

REPOSITORY
  R266 Breeze Icons

BRANCH
  media-artist

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

AFFECTED FILES
  icons-dark/actions/24/view-media-artist.svg
  icons/actions/24/view-media-artist.svg

To: astippich, #vdg
Cc: #frameworks, michaelh, ngraham

D11874: change 24px view-media-artist icon

[Alexander Stippich]

astippich added a comment.


  Before
  F5781105: Screenshot_20180401_222839.png 

  After
  F5781107: Screenshot_20180401_222817.png 


REPOSITORY
  R266 Breeze Icons

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

To: astippich, #vdg
Cc: #frameworks, michaelh, ngraham

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> mgallien wrote in taglibextractor.cpp:363
> Is it really needed to push directly a list ?
> You are ignoring the fact that the original developer intended add to be 
> called once for each value. I would prefer to keep doing that until we can be 
> absolutely sure that nothing breaks if we change that.

That was actually the point for this. In propertyinfo, it says that this 
property is a stringlist, but it was actually never one, just a simple string. 
For multiple values, there will be multiple entries in the property map with a 
single string.  This is not considered within Elisa at least. We could also go 
the other way and adjust the properties to match the behavior. 
@michaelh agreed that stringlists are easier to handle

> mgallien wrote in taglibextractor.cpp:389
> Are you sure we need this cast ? I do not think we will ever need to have 
> negative track numbers added. The original type is unsigned int and should 
> exactly convey the fact that we do not expect negative numbers.

I changed it so it matches its associated value type, I could also change the 
type in propertyinfo to uint

> mgallien wrote in taglibextractor.cpp:393
> idem

This one is a litte bit more tricky, as the year property could theoretically 
also be negative (B.C.). Not really relevant for music, but maybe for written 
documents, and still only very rarily :)

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  Thanks a lot!

REPOSITORY
  R266 Breeze Icons

BRANCH
  playlist_shuffle_repeat

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

To: astippich, #breeze, #vdg, andreaska, andreask, ngraham
Cc: bcooksley, andreask, ngraham, #frameworks, michaelh

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:f0a96f5f85cb: change 32px icons for playlist shuffle and 
repeat (authored by astippich).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10292?vs=29110=30868

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

AFFECTED FILES
  icons-dark/actions/32/media-playlist-normal.svg
  icons-dark/actions/32/media-playlist-repeat.svg
  icons-dark/actions/32/media-playlist-shuffle.svg
  icons-dark/actions/32/media-repeat-all.svg
  icons-dark/actions/32/media-repeat-none.svg
  icons/actions/32/media-playlist-normal.svg
  icons/actions/32/media-playlist-repeat.svg
  icons/actions/32/media-playlist-shuffle.svg
  icons/actions/32/media-repeat-all.svg
  icons/actions/32/media-repeat-none.svg

To: astippich, #breeze, #vdg, andreaska, andreask, ngraham
Cc: bcooksley, andreask, ngraham, #frameworks, michaelh

D11461: rename 64 px icons added for elisa

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:0f7c93f1b7e8: rename 64 px icons added for elisa 
(authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11461?vs=29851=30869#toc

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11461?vs=29851=30869

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

AFFECTED FILES
  icons-dark/actions/64/media-album-cover.svg
  icons-dark/actions/64/media-album-track.svg
  icons-dark/actions/64/media-default-album.svg
  icons-dark/actions/64/media-default-track.svg
  icons-dark/actions/64/view-media-playlist.svg
  icons/actions/64/media-album-cover.svg
  icons/actions/64/media-album-track.svg
  icons/actions/64/media-default-album.svg
  icons/actions/64/media-default-track.svg
  icons/actions/64/view-media-playlist.svg

To: astippich, #breeze, #vdg, andreask
Cc: #frameworks, michaelh, ngraham

D11461: rename 64 px icons added for elisa

[Alexander Stippich]

astippich added a comment.


  any feedback here? would be great to resolve this for frameworks 5.45

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreask
Cc: #frameworks, michaelh, ngraham

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  Can I land this? phabricator doesn't show it as accepted though

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska, andreask, ngraham
Cc: andreask, ngraham, #frameworks, michaelh

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  any feedback on this? I would like to land this and then get going with 
D10803  again

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  It's just testing each file for all available properties, one after another. 
This revision just adds missing bits to the existing test. Refactoring should 
be done separately imho

INLINE COMMENTS

> michaelh wrote in propertyinfo.cpp:98
> Looks unrelated to me.

It's not, composer should really be treated the same as e.g. artist or 
lyricist, as it can have multiple entries.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  Okay, I changed my mind, I rebased on top of master. Since the other revision 
needs more time and the new properties behave like the old ones, I'd like to 
end this long journey and land this revision if no-one objects :)

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, nicolasfella, alexeymin

D11457: decrease bitrate precision for display

[Alexander Stippich]

astippich added reviewers: Frameworks, Baloo.

REPOSITORY
  R824 Baloo Widgets

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

To: astippich, #frameworks, #baloo
Cc: ashaposhnikov, astippich, spoorun, nicolasfella, alexeymin

D11461: rename 64 px icons added for elisa

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: Breeze, VDG, andreask.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  see D10492 . The new 64px icons with the 
same name cause issues because the icons now look totally different with 
different sizes.
  rename the files so that explicit support must be added in Elisa. the 
view-media-playlist icon is actually the JuK icon, and should stay unique, so 
delete it again

REPOSITORY
  R266 Breeze Icons

BRANCH
  icons_move

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

AFFECTED FILES
  icons-dark/actions/64/media-album-cover.svg
  icons-dark/actions/64/media-album-track.svg
  icons-dark/actions/64/media-default-album.svg
  icons-dark/actions/64/media-default-track.svg
  icons-dark/actions/64/view-media-playlist.svg
  icons/actions/64/media-album-cover.svg
  icons/actions/64/media-album-track.svg
  icons/actions/64/media-default-album.svg
  icons/actions/64/media-default-track.svg
  icons/actions/64/view-media-playlist.svg

To: astippich, #breeze, #vdg, andreask
Cc: #frameworks, michaelh, ngraham

D11457: decrease bitrate precision for display

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R824:e9f00ca876a2: decrease bitrate precision for display 
(authored by astippich).

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11457?vs=29840=29849

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

AFFECTED FILES
  src/widgetfactory.cpp

To: astippich, #frameworks, #baloo, elvisangelaccio
Cc: ashaposhnikov, astippich, spoorun, nicolasfella, alexeymin

D10292: change 32px icons for playlist shuffle and repeat

[Alexander Stippich]

astippich added a comment.


  ping @andreask

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #breeze, #vdg, andreaska, andreask, ngraham
Cc: andreask, ngraham, #frameworks, michaelh

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  In D11365#227241 , @michaelh wrote:
  
  > Just in case you didn't use data-driven tests on purpose: 
https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html
  
  
  Thanks for the hint, that's new for me. Anyways, there are subtle differences 
for each file and there will be more in the future, so I think it is better to 
leave it unchanged.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added reviewers: mgallien, michaelh.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - remove lyrics and map encodedby to generator

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=28300=28467

BRANCH
  enhance_taglib

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, spoorun, 
nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  I don't know dolphin works, but given how KFileMetadata is designed, the new 
tags should be ignored until explicit support is added in dolphin.

INLINE COMMENTS

> michaelh wrote in properties.h:164
> ?

Oops, seems like a wrong copy & paste :)  will fix

> michaelh wrote in properties.h:168
> Will we break ABI with correct 'Language'?

I noticed the typo as well, but I think this is a matter of API compatibility, 
so I left it unchanged. Matthieu should know.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: michaelh, ngraham, #frameworks, kmorwinski

D16135: refactor tests for taglibextractor

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns, svuorela.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  refactors all tests so that they can be reused for
  files of different mimetype, but with the same metadata format

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_tests

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich added a comment.


  To be on the safe side here: I am allowed to modify private member functions 
regarding binary compatibility, right?

REPOSITORY
  R286 KFileMetaData

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

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

D16135: refactor tests for taglibextractor

[Alexander Stippich]

astippich added a comment.


  In D16135#341549 , @bruns wrote:
  
  > Can you split this up in two patches, the first one updating the test files 
and fixing the values in the tests, and the second one doing the refactoring?
  
  
  see D16162 . I will rebase afterwards

REPOSITORY
  R286 KFileMetaData

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

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

D16162: harmonize test data for vorbis comment tags

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  harmonize the test data for all files with vorbis comments
  so that the test can be converted to a data-driven test 
  in a following patch

REPOSITORY
  R286 KFileMetaData

BRANCH
  harmonize_vorbis

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Refactor the specific extraction funtions in taglibextractor
  so that they can be re-used for files of different mimetype,
  but with the same tag types for their metadata.
  No functional change intended.

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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

D16165: Don't crash on invalid exiv2 data

[Alexander Stippich]

astippich added a comment.


  I guess bugs
  352856 
  353848
  361259
  will also be fixed by this?

REPOSITORY
  R286 KFileMetaData

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

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

D16162: harmonize test data for vorbis comment tags

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:1aab09a628ae: harmonize test data for vorbis comment tags 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16162?vs=43485=43489

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich added a comment.


  In D16163#342057 , @mgallien wrote:
  
  > In D16163#342046 , @astippich 
wrote:
  >
  > > To be on the safe side here: I am allowed to modify private member 
functions regarding binary compatibility, right?
  >
  >
  > This page is a very good reference: 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
  
  
  Thanks, I already knew that side, but I tend to ask explicitly in case I 
misunderstood something. I really don't want to mess up frameworks :)
  
  In D16163#342061 , @bruns wrote:
  
  > In D16163#342046 , @astippich 
wrote:
  >
  > > To be on the safe side here: I am allowed to modify private member 
functions regarding binary compatibility, right?
  >
  >
  > Non-virtual methods do not affect the class layout or the vtable layout, so 
you are safe here for sure. `private` or not does not matter.
  
  
  Thanks for the explanation!

REPOSITORY
  R286 KFileMetaData

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

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

D16135: refactor tests for taglibextractor

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:2d6b448b69b0: refactor tests for taglibextractor 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16135?vs=43490=43525

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

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

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

D16135: refactor tests for taglibextractor

[Alexander Stippich]

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


  - rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16135?vs=43409=43490

BRANCH
  refactor_tests

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

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

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


  - use forward declaration

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16163?vs=43486=43500

BRANCH
  refactor_taglib

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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

D16196: add a description property to KFileMetaData

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  add the description property to KFileMetaData,
  as the Comment property is inaccurate in some cases,
  see D12114 . Currently unused.

REPOSITORY
  R286 KFileMetaData

BRANCH
  description_property

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

AFFECTED FILES
  src/properties.h
  src/propertyinfo.cpp

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[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
  provide a list of supported mimetypes similar to
  the extractor plugins and adjust test

REPOSITORY
  R286 KFileMetaData

BRANCH
  mimetypes_embedded_image

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

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

D11869: epubextractor: Add property ReleaseYear

[Alexander Stippich]

astippich accepted this revision.
astippich added a comment.
This revision is now accepted and ready to land.
Herald edited subscribers, added: Baloo, kde-frameworks-devel; removed: 
Frameworks.


  Any objections? Otherwise I will merge this on michaelh's behalf

REPOSITORY
  R286 KFileMetaData

BRANCH
  releaseyear (branched from master)

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

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

D11869: epubextractor: Add property ReleaseYear

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:15adb113399e: epubextractor: Add property ReleaseYear 
(authored by michaelh, committed by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11869?vs=31112=43685

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

AFFECTED FILES
  autotests/epubextractortest.cpp
  src/extractors/epubextractor.cpp

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

D16165: Don't crash on invalid exiv2 data

[Alexander Stippich]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  dont-crash-invalid-exiv (branched from master)

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

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> mgallien wrote in embeddedimagedata.h:62
> You can make it static because you are returning a static member. It means 
> that you should probably return by const reference (even if I do not remember 
> if it is always the best answer).

I had it this way before, but actually reverted it to make it consistent with 
the plugins, and this would also allow to potentially change the implementation 
later, wouldn't it? Anyway, your call.

REPOSITORY
  R286 KFileMetaData

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

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

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


  - move mimetype list to private

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16197?vs=43587=43601

BRANCH
  mimetypes_embedded_image

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

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

D16283: implement more tags for asf metadata

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibextractor.cpp:739
> this makes me always wonder why we don't do:
> 
>   // decltype(data.albumArtists) == QStringList
>   for (attribute : lstAsf) {
> QString t = TStringToQString(attribute.toString());
> data.albumArtists << contactsFromString(t);
>   }

Yeah, there is a lot of unneccesary stuff in there that must be cleaned up. But 
before I get to that, I would like to add tests for multiple entries, and 
before that we have to agree how the output of mutliple entries shall look 
like, which brings me to D12950  :)

> bruns wrote in taglibextractor.h:28
> Side note - I think `tfilestream.h` is no longer needed.

Is it okay if I push this directly?

REPOSITORY
  R286 KFileMetaData

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

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

D16283: implement more tags for asf metadata

[Alexander Stippich]

astippich updated this revision to Diff 43885.
astippich marked 3 inline comments as done.
astippich added a comment.


  - fix comment and simplify iterating
  - cleanup

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16283?vs=43822=43885

BRANCH
  extract_asf

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

AFFECTED FILES
  autotests/samplefiles/test.wma
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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

D16281: update epub test data and test for comment property

[Alexander Stippich]

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


  - fix test file

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16281?vs=43820=43887

BRANCH
  description_test_preparation

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

AFFECTED FILES
  autotests/epubextractortest.cpp
  autotests/samplefiles/test.epub

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

astippich added a comment.


  I've decided to name it similar to the extractor plugins

REPOSITORY
  R286 KFileMetaData

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

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

astippich updated this revision to Diff 43888.
astippich marked an inline comment as done.
astippich added a comment.


  - remove read prefix

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16197?vs=43819=43888

BRANCH
  mimetypes_embedded_image

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

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

D16196: add a description property to KFileMetaData

[Alexander Stippich]

astippich added a comment.


  ping. would be great to get this in before the string freeze

REPOSITORY
  R286 KFileMetaData

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

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

D16179: compare with QLatin1String and harmonize handling of all types

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:fd9ce3a4e1d2: compare with QLatin1String and harmonize 
handling of all types (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16179?vs=43546=43818

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp

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

D16179: compare with QLatin1String and harmonize handling of all types

[Alexander Stippich]

astippich added a comment.


  Ok. Thanks!

REPOSITORY
  R286 KFileMetaData

BRANCH
  harmonize_optimize

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

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

D16282: extract ape tags from ape and wavpack files

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  extend the existing support of ape tags to all types
  that use the ape tag system

REPOSITORY
  R286 KFileMetaData

BRANCH
  extract_ape_wavpack

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

AFFECTED FILES
  autotests/samplefiles/test.ape
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.wv
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

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

D16283: implement more tags for asf metadata

[Alexander Stippich]

astippich added a comment.


  for some reason the TagLib::ASF::File tag() function returns only an invalid 
pointer, so I decided to use the dynamic_cast here.

REPOSITORY
  R286 KFileMetaData

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

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

D16281: update epub test data and test for comment property

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  update the test data for epub and test for
  the comment property in the epub autotest

REPOSITORY
  R286 KFileMetaData

BRANCH
  description_test_preparation

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

AFFECTED FILES
  autotests/epubextractortest.cpp
  autotests/samplefiles/test.epub

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

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


  - use QVERIFY
  - use capital T
  - add since 5.52 tag and rephrase documentation

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16197?vs=43601=43819

BRANCH
  mimetypes_embedded_image

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

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

D16281: update epub test data and test for comment property

[Alexander Stippich]

astippich added a comment.


  In preparation of D12114 , which I plan 
to commandeer

REPOSITORY
  R286 KFileMetaData

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

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

D16283: implement more tags for asf metadata

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  add some more tags for asf audio files

REPOSITORY
  R286 KFileMetaData

BRANCH
  extract_asf

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

AFFECTED FILES
  autotests/samplefiles/test.wma
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

astippich marked an inline comment as done.
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in embeddedimagedata.cpp:73
> `supportedMimeTypes()` ?
> capital `T`, see `ImageType` and `QMimeType`

The "read" is supposed to mark that only read operations for these mimetypes 
are supported. I plan to add support for writing add some points, and the 
supported formats may differ.
Or maybe one could use extractionMimeTypes() ?

REPOSITORY
  R286 KFileMetaData

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

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

D16179: compare with QLatin1String and harmonize handling of all types

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  small optimization and harmonization for all metadata formats in 
taglibextractor

REPOSITORY
  R286 KFileMetaData

BRANCH
  harmonize_optimize

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp

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

D16486: implement support for reading ID3 tags from aiff and wav files

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  implement ID3 tags for aiff and wav files as supported by taglib

REPOSITORY
  R286 KFileMetaData

BRANCH
  extract_aiff_riff

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

AFFECTED FILES
  autotests/samplefiles/test.aif
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.wav
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:1e9705162343: provide a list of supported mimetypes for 
embeddedimagedata (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16197?vs=43888=44363

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

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

D16283: implement more tags for asf metadata

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:02e8cab069ff: implement more tags for asf metadata 
(authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D16283?vs=43885=44366#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16283?vs=43885=44366

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

AFFECTED FILES
  autotests/samplefiles/test.wma
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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

D16282: extract ape tags from ape and wavpack files

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:4582ba5f3d7a: extract ape tags from ape and wavpack files 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16282?vs=43821=44365

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

AFFECTED FILES
  autotests/samplefiles/test.ape
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.wv
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

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

D16281: update epub test data and test for comment property

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:d97b15679179: update epub test data and test for comment 
property (authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D16281?vs=43887=44364#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16281?vs=43887=44364

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

AFFECTED FILES
  autotests/epubextractortest.cpp
  autotests/samplefiles/test.epub

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

D16579: Musepack disk number field name is DISC.

[Alexander Stippich]

astippich added a comment.


  In D16579#352147 , @smithjd wrote:
  
  > In D16579#351910 , @astippich 
wrote:
  >
  > > The ape tag tests fail with this patch, but the test is actually wrong in 
that regard. It tests for an empty disc number, which I haven't noticed before.
  > >  I've found references to both DISCNUMBER and DISC, so the safest way is 
probably to check both.
  > >  So please query both tags like it is already done for the album artist 
and adjust the taglibextractortest.
  >
  >
  > DISCNUMBER doesn't seem to be a valid (or widely used) field for APEv2.
  >
  > More (Picard) information: https://picard.musicbrainz.org/docs/mappings
  >
  > Also it appears Picard, Puddletag and ffmpeg use 'Album Artist', not 
'ALBUMARTIST'. And the link I provided 
(https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping) also lists 'Album 
Artist' not 'ALBUMARTIST'. The unit test file has an 'ALBUMARTIST' field that 
should be changed to 'Album Artist'.
  
  
  Yes, maybe it's not widely used, but it is used. Kodi for example supports 
both reading from DISCNUMBER and DISC. If you use kid3 to edit the metadata of 
ape tags, the standard behavior is to actually to write to DISCNUMBER (and 
similar to ALBUMARTIST).
  One thing I've learned when I digged into metadata of audio files is that 
there is no standard, and KFileMetaData should handle as much cases as 
possible. Since it is easy to query both, please add it.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Alexander Stippich]

astippich added a comment.


  In D16490#352109 , @bruns wrote:
  
  > In D16490#351935 , @astippich 
wrote:
  >
  > > In D16490#351799 , @bruns 
wrote:
  > >
  > > > In D16490#351662 , 
@astippich wrote:
  > > >
  > > > > Only one minor thing: please also check that the mimetype is in the 
list of supported mimetypes
  > > >
  > > >
  > > > This can actually happen and is completely valid, due to mimetype 
inheritance.
  > > >
  > > > So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...
  > >
  > >
  > > Hmmm, I don't understand. When I change the code to return an empty 
stringlist of supported mimetypes for the xmlextractor, the tests still pass.
  > >  This should imho be covered by the tests.
  >
  >
  > This is one level above these tests. The surrounding code ensures the right 
extractor is called for each file, see 
`ExtractorCollection::fetchExtractors(...)`.
  
  
  Right, and if e.g. the list of supported mimetypes is empty, the 
corresponding extractor will never be selected because ExtractorCollection 
doesn't know that the mimetype is supported by this extractor.
  Hence we should ensure and test imho that the list of supported mimetypes 
provided to the ExtractorCollection is correct for this extractor. I'm not 
calling for testing that the right extractor is selected.
  
  > These are unit tests. The test itself is responsible to call an extractor 
with a suitable file and a matching mimetype.
  > 
  > What you are calling for are system tests.

REPOSITORY
  R286 KFileMetaData

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

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

D16486: implement support for reading ID3 tags from aiff and wav files

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:19f0237fdc98: implement support for reading ID3 tags from 
aiff and wav files (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16486?vs=44369=44583

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

AFFECTED FILES
  autotests/samplefiles/test.aif
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.wav
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

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

D16488: [KFileMetaData] Add helper for XML encoded Dublin Core metadata

[Alexander Stippich]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  xml_extractor

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

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

D16488: [KFileMetaData] Add helper for XML encoded Dublin Core metadata

[Alexander Stippich]

astippich added a comment.


  I guess you plan to convert the existing extractors to use this?
  The only concern I might have that now two iterations over the node are 
required when this is used in e.g. odfextractor, isn't it?
  Btw, I don't think it's necessary to prepend KFileMetaData in the diff title, 
since the repository is only for KFileMetaData anyway.

REPOSITORY
  R286 KFileMetaData

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

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

D16554: move supported mimetypes to static string list

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Do not create the list of supported mime types on demand
  and use a static stringlist instead.
  Order alphabetically where required

REPOSITORY
  R286 KFileMetaData

BRANCH
  mimetype_lists

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

AFFECTED FILES
  src/extractors/epubextractor.cpp
  src/extractors/epubextractor.h
  src/extractors/exiv2extractor.cpp
  src/extractors/exiv2extractor.h
  src/extractors/ffmpegextractor.cpp
  src/extractors/ffmpegextractor.h
  src/extractors/odfextractor.cpp
  src/extractors/odfextractor.h
  src/extractors/office2007extractor.cpp
  src/extractors/office2007extractor.h
  src/extractors/plaintextextractor.cpp
  src/extractors/plaintextextractor.h
  src/extractors/poextractor.cpp
  src/extractors/poextractor.h
  src/extractors/popplerextractor.cpp
  src/extractors/popplerextractor.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/writers/taglibwriter.cpp
  src/writers/taglibwriter.h

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

D16560: create a separate test file for embedded images test

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  if the test.jpg file is altered, e.g. additional
  metadata is added, the embedded images test fails.
  create a separate cover test file from the existing one
  for this test.

REPOSITORY
  R286 KFileMetaData

BRANCH
  cover_jpg

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  autotests/samplefiles/cover.jpg

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Alexander Stippich]

astippich added a comment.


  Only one minor thing: please also check that the mimetype is in the list of 
supported mimetypes

REPOSITORY
  R286 KFileMetaData

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

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

D16564: cleanup the test for embedded image extraction

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: Frameworks, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  cleanup the test by converting to a data-driven one

REPOSITORY
  R286 KFileMetaData

BRANCH
  embedded_test

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

AFFECTED FILES
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h

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

D15829: make units and prefixes of formatValue translatable

[Alexander Stippich]

astippich added a comment.


  ping

REPOSITORY
  R244 KCoreAddons

BRANCH
  translate_units

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

To: astippich, bruns, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16489: [KFileMetaData] Add extractor for generic XML and SVG

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> xmlextractor.cpp:24
> +
> +#include 
> +#include 

seems unused

> xmlextractor.cpp:27
> +#include 
> +#include 
> +

same

> xmlextractor.cpp:71
> +
> +return list;
> +}

Can we agree on using a static qstringlist if the mimetypes are fixed? I 
prepared a patch to convert all other extractors to this scheme when applicable 
in D16554 

Also, using an initializer list is faster, see 
https://www.angrycane.com.br/en/2018/06/19/speeding-up-cornercases/

REPOSITORY
  R286 KFileMetaData

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

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