D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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 { > +

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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

D10918: taglibextractor: Refactor for better readability

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,

D10918: taglibextractor: Refactor for better readability

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