Differential D10918

taglibextractor: Refactor for better readability
ClosedPublic
Actions

Authored by michaelh on Feb 28 2018, 2:39 PM.

Details

Reviewers
mgallien
astippich
Group Reviewers
Baloo
Frameworks
Commits
R286:a97308cf9ab8: taglibextractor: Refactor for better readability
Summary
  • Use const where applicable
Test Plan

make test

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
michaelh created this revision.Feb 28 2018, 2:39 PM
Restricted Application added a project: Baloo. · View Herald TranscriptFeb 28 2018, 2:39 PM
michaelh requested review of this revision.Feb 28 2018, 2:39 PM
mgallien added a comment.Feb 28 2018, 9:22 PM

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

michaelh added a comment.EditedMar 1 2018, 10:53 AM
In D10918#215909, @mgallien wrote:

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

I did this mostly for myself because I could not understand the code otherwise and published it because I thought it might be useful. I don't have the time to merge in the changes of D10803
Shall I abandon it?

astippich added a subscriber: astippich.Mar 1 2018, 7:05 PM
In D10918#216120, @michaelh wrote:
In D10918#215909, @mgallien wrote:

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

I did this mostly for myself because I could not understand the code otherwise and published it because I thought it might be useful. I don't have the time to merge in the changes of D10803
Shall I abandon it?

If you don't mind, I will take this over when D10803 lands.

michaelh added a comment.Mar 2 2018, 6:43 AM
In D10918#216439, @astippich wrote:

If you don't mind, I will take this over when D10803 lands.

That would be great.

astippich added a comment.Mar 13 2018, 6:46 AM

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.

michaelh added a comment.Mar 13 2018, 9:51 AM
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 ;-)

astippich requested changes to this revision.Mar 14 2018, 8:57 PM

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

src/extractors/taglibextractor.cpp
331

I think we should check explicitly for the appropriate mimetypes here

This revision now requires changes to proceed.Mar 14 2018, 8:57 PM
michaelh added inline comments.Mar 14 2018, 9:52 PM
src/extractors/taglibextractor.cpp
331

You mean other than lines 225, 233, 240?

michaelh added a comment.Mar 14 2018, 9:59 PM
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.

michaelh added inline comments.Mar 14 2018, 10:04 PM
src/extractors/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.

astippich added inline comments.Mar 15 2018, 6:52 AM
src/extractors/taglibextractor.cpp
331

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

astippich added inline comments.Mar 15 2018, 7:03 AM
src/extractors/taglibextractor.cpp
157

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)

michaelh added inline comments.Mar 15 2018, 2:15 PM
src/extractors/taglibextractor.cpp
219

No if here

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.

astippich accepted this revision.Mar 15 2018, 4:35 PM
astippich added inline comments.
src/extractors/taglibextractor.cpp
331

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)

This revision is now accepted and ready to land.Mar 15 2018, 4:35 PM
Closed by commit R286:a97308cf9ab8: taglibextractor: Refactor for better readability (authored by michaelh). · Explain WhyMar 15 2018, 5:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 29617

View Options

src/extractors/taglibextractor.h

Loading...
View Options

src/extractors/taglibextractor.cpp

Loading...