aadsm / jsmediatags

Media Tags Reader (ID3, MP4, FLAC)
Other
748 stars 128 forks source link

Parse chapters (CHAP/CTOC) frames #56

Closed nils closed 7 years ago

nils commented 7 years ago

Hi António,

I've implemented chapter parsing according to http://id3.org/id3v2-chapters-1.0.

This has also been requested in issue #42.

I'm open for improvements based on your review :-)

Best Regards, Nils

aadsm commented 7 years ago

Thanks for putting this up!

My only comment about the approach is related to the creation of a mutual dependency between ID3TagReader and ID3FrameReader.

My recommendation is to move the _readFrames function to ID3FrameReader (with all its frame reading dependencies like _readFrameHeader (but not _expandShortcutTags though)) so they can be reused there without having to create a new data reader and ID3TagReader.

What do you think?

On the logistic side:

nils commented 7 years ago

Thanks for your review!

I'll spend some time next weekend to implement the changes you recommended.

nils commented 7 years ago

I think everything should be done like you described, could you please take another look?

nils commented 7 years ago

I just added the unit tests and commented on your specific remarks.

aadsm commented 7 years ago

Awesome! I've just merged it.

aadsm commented 7 years ago

New version v2.6.0 just published on npm.

aadsm commented 7 years ago

Thank you so much for contributing to this project. Chapter support is a feature that's going to be really useful for tons of people!

Btw, are you looking for job in the front end world? I'm happy to refer at my current workplace.