add ability to load playlists from file browser
ClosedPublic

Authored by astippich on Jul 15 2018, 9:11 AM.

Details

Summary

adds handling of m3u files to the file browser and allows to load the playlist from within the file browser

Test Plan

loading of playlist via Files tab works

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich requested review of this revision.Jul 15 2018, 9:11 AM
astippich created this revision.

See my inline comments.

src/models/filebrowsermodel.cpp
74

and the role name should be isPlaylist

src/models/filebrowsermodel.h
39

If I understand your diff correctly, this should be IsPlayListRole.

One more comment I had forgotten.

src/qml/FileBrowserDelegate.qml
46

This change and later in the same file introduce too much special and different code paths where another solution could avoid that.
Could you consider adding support for playlist in replaceAndPlay and enqueue ? This way, the user can always have the same actions and we avoid having too many code paths in the qml code.

astippich added inline comments.Aug 25 2018, 12:40 PM
src/qml/FileBrowserDelegate.qml
46

What solution do you have in mind? Would you like to get rid of the "isPlaylist" variable altogether?
Note that there is currently no possibility to only append the tracks from the loaded playlist to our play queue, and the metadata view must also be disabled for playlists.

The problem with the playlist is that it is just a simple file url like every music file, so the alternative would be to check the mimetype in replaceAndPlay() etc. and call the loadPlayList function there. If you prefer that, I can change it.

mgallien added inline comments.Aug 29 2018, 6:12 AM
src/qml/FileBrowserDelegate.qml
46

I believe that handling the playlist like any other files has the advantage of simplifying the qml files and should not be too hard to do in c++.

astippich updated this revision to Diff 41859.Sep 17 2018, 5:50 PM
  • rename enums, handle loading of playlist differently

In order to avoid polluting the mediaplaylist with a QMimeDataBase, I decided to move the handling to the proxy model. Let me know what you think.

mgallien requested changes to this revision.Sep 26 2018, 7:07 PM

In order to avoid polluting the mediaplaylist with a QMimeDataBase, I decided to move the handling to the proxy model. Let me know what you think.

Sorry for the delay. I am fine with your proposed solution. Thanks

Please fix my comments and this can soon land on master.

src/models/filebrowsermodel.cpp
73–74

idem

src/models/filebrowsermodel.h
38–39

Please push this refactoring to a later patch.

src/models/filebrowserproxymodel.cpp
129

Please remove this debug print

130

I believe that it would be more robust to only go to the playlist code if the mimetype matches the one of m3u playlists. Currently Elisa only supports them so that would be the best way to ensure that it will never fail.

This revision now requires changes to proceed.Sep 26 2018, 7:07 PM
astippich updated this revision to Diff 43042.Oct 7 2018, 2:33 PM
  • rebase
  • implement feedback
astippich marked 7 inline comments as done.Oct 7 2018, 2:35 PM
astippich added inline comments.
src/models/filebrowserproxymodel.cpp
129

oops, sorry

130

This is the mimetype of m3u playlists.

mgallien added inline comments.Oct 7 2018, 3:06 PM
src/models/filebrowserproxymodel.cpp
130

Sorry, I did not knew that. I trust you.

mgallien accepted this revision.Oct 9 2018, 3:14 PM

Thanks for your work.

This revision is now accepted and ready to land.Oct 9 2018, 3:14 PM
This revision was automatically updated to reflect the committed changes.
astippich marked an inline comment as done.