1093219 - [Music] m3u parser for playlist

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Hubert Figuiere [:hub]]

We need to write a m3u parser (read and write) for the playlist support.

Comment 1

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

Current work in progress. Basic parsing.

Comment 2

[David Flanagan [:djf]]

Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

I've left a lot of comments on github. This needs more work, and more thought about how it is going to integrate with the rest of the music app. For example, is there actually any need to parse the EXTINF comments in an m3u file, or can we just ignore those?

Comment 3

[Hubert Figuiere [:hub]]

Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

David,

I think I addressed all the issues. Including simplifying the model to only keep what we need.

Notable change I always write the #EXTM3U magic at the beginning.

Tests have been updated.

Thank you kindly.

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

This is a lot better than it was. The parse() function is broken, however, and I've left various other comments on github.

So one more round of changes, then we should start thinking about how to integrate this with the metadata parsing code and how to handle playlists in the app once they've been parsed. How will be query the mediadb for a list of all the playlists it knows about, for example.

RTL fixes should probably come first, though.

Comment 5

[Hubert Figuiere [:hub]]

Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

So I updated the PR with all the comments. Added some more test.

I'll wait for confirmation of Gaia-try, but we should be alright with the tests in CI.

(I'll squash the commits into one when ready to land)

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

The code looks okay except that the test playlist file is missing from the PR. I'm guessing you just forgot to 'git add' that new file.

Does it make sense to land this playlist data structure and parser without hooking it up to the metadata parser? If you want to get it landed (once you add the missing test file) that is okay with me.

It might make more sense, though, to integrate it with the metadata parser before landing so it is not just sitting unused in the music app.

Comment 7

[Hubert Figuiere [:hub]]

As I commented on github, the file is not needed as the mock for the device storage API feed the bytes to the reader. Maybe this makes a hole in the test suite.

As for landing it, since we are positive to have the feature this cycle, it will be used. Therefore I don't see the need to hold it back. Surely there will be changes down the line as we use it in the app.

Comment 8

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia/commit/1fff49c664f905f11a86426a9835e6df6b58e825