D10918: taglibextractor: Refactor for better readability

2018-03-15 Thread Michael Heidelbach 
This revision was automatically updated to reflect the committed changes.
Closed by commit R286:a97308cf9ab8: taglibextractor: Refactor for better 
readability (authored by michaelh).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10918?vs=28268=29617

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

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

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

D10918: taglibextractor: Refactor for better readability

2018-03-15 Thread Alexander Stippich 
astippich accepted this revision.
astippich added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:331
> I think it is. At this point only `ogg`, `flac` and `opus` should be left.
> Also I only refactored and didn't change the logic, unless I made a mistake 
> that is.

you're right, it was the same way before and it already bugged me there ;)
but since this is a only refactoring, it's probably okay.
note that quite not all mimetypes are handled here compared with the mimetype 
list above (line 58)

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglibextractor (branched from master)

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

2018-03-15 Thread Michael Heidelbach 
michaelh added inline comments.

INLINE COMMENTS

> taglibextractor.cpp:219
>  
> -// Handling multiple tags in Ogg containers.
> -{

No `if` here

> astippich wrote in taglibextractor.cpp:331
> Yes, but imho we shouldn't even enter the function for mimetypes not having 
> ogg tags. Maybe that is overly careful

I think it is. At this point only `ogg`, `flac` and `opus` should be left.
Also I only refactored and didn't change the logic, unless I made a mistake 
that is.

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

2018-03-15 Thread 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

2018-03-15 Thread 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

D10918: taglibextractor: Refactor for better readability

2018-03-14 Thread Michael Heidelbach 
michaelh added inline comments.

INLINE COMMENTS

> taglibextractor.cpp:157
> +if (itAlbumArtists != allTags.end()) {
> +data.albumArtists = 
> itAlbumArtists->second.toStringList().toString(", ");
>  }

This is an example for what we're discussing. Why the join??? We might lose 
info, when the album artist contains a comma.

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

2018-03-14 Thread Michael Heidelbach 
michaelh added a comment.


  In D10918#225927 , @astippich 
wrote:
  
  > @mgallien as the maintainer must also give his ok.
  
  
  @mgallien: You're the maintainer of kfilemetadata? Great! I didn't know. I 
thought all of Vishesh's heritage was unmaintained.

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

2018-03-14 Thread Michael Heidelbach 
michaelh added inline comments.

INLINE COMMENTS

> astippich wrote in taglibextractor.cpp:331
> I think we should check explicitly for the appropriate mimetypes here

You mean other than lines 225, 233, 240?

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

2018-03-14 Thread 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

D10918: taglibextractor: Refactor for better readability

2018-03-13 Thread Michael Heidelbach 
michaelh added a comment.


  In D10918#224424 , @astippich 
wrote:
  
  > Btw, I'm not opposed anymore for merging before D10803 
, as I need some more time to think about 
the value types and probably also need to extend the tests. I will adapt to the 
changes afterwards.
  
  
  Good! (I always thought it would be easier to apply your patch on top of this 
one). There is no need for you to commandeer this patch anymore. You could 
review it instead ;-)

REPOSITORY
  R286 KFileMetaData

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

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

D10918: taglibextractor: Refactor for better readability

2018-03-13 Thread Alexander Stippich 
astippich added a comment.


  Btw, I'm not opposed anymore for merging before D10803 
, as I need some more time to think about 
the value types and probably also need to extend the tests. I will adapt to the 
changes afterwards.

REPOSITORY
  R286 KFileMetaData

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

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

D10918: taglibextractor: Refactor for better readability

2018-03-01 Thread Michael Heidelbach 
michaelh added a comment.


  In D10918#216439 , @astippich 
wrote:
  
  > If you don't mind, I will take this over when D10803 
 lands.
  
  
  That would be great.

REPOSITORY
  R286 KFileMetaData

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

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

D10918: taglibextractor: Refactor for better readability

2018-02-28 Thread Matthieu Gallien 
mgallien added a comment.


  Thanks for your work.
  I would prefer to wait for D10803  to 
land before doing any big changes like that.

REPOSITORY
  R286 KFileMetaData

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

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

D10918: taglibextractor: Refactor for better readability

2018-02-28 Thread Michael Heidelbach 
michaelh created this revision.
michaelh added reviewers: mgallien, Baloo, Frameworks.
michaelh added a project: Frameworks.
Restricted Application added a project: Baloo.
michaelh requested review of this revision.

REVISION SUMMARY
  - Use const where applicable

TEST PLAN
  make test

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglibextractor (branched from master)

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

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

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