-
-
Notifications
You must be signed in to change notification settings - Fork 51
Add support for loading/saving m3u playlists #813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
zeebok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my testing, I saved a playlist from Music but it failed to open. I am going to look into it more because I am not sure if the issue is how it is saving or how it is loading.
Co-authored-by: Ryan Kornheisl <ryan@skarva.tech>
Co-authored-by: Ryo Nakano <ryonakaknock3@gmail.com>
Co-authored-by: Ryo Nakano <ryonakaknock3@gmail.com>
Hmmm.... It worked in my testing. I will test more cases/see if i can get issues |
Co-authored-by: Danielle Foré <danielle@elementary.io>
Co-authored-by: Danielle Foré <danielle@elementary.io>
|
My access to a PC will be very limited in the coming weeks, i will not be able to take care of this for a couple weeks |
Co-authored-by: Ryo Nakano <ryonakaknock3@gmail.com>
Co-authored-by: Ryo Nakano <ryonakaknock3@gmail.com>
|
Vague thought but once this is merged, technically we could bin the whole "save/restore queue in dconf" and simply save/load from a m3u file in the data folder That would avoid two separate code blocks for the same thing (less moving parts), and skip dumping shit of dubious length in dconf maybe also not relying on dconf but straight up loading a file could also speed up recovery. Saving the playlist only on app closing (or DE session disconnecting if the apps are allowed grace time) would also reduce disk writes and cpu usage |
|
@danirabbit this has now conflicts because of the new QueueView branch being merged, despite sitting ready since months. It feels like another slap in the face. |
| }); | ||
| } | ||
|
|
||
| public void action_save_m3u_playlist () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be an action in the playback manager, not in part of the UI
|
@teamcons apologies. I hadn't kept up with this since when I was last reviewing it you had said you didn't have time to work on it and another reviewer had taken over since. I think regardless this shouldn't be in a part of the UI. So resolving my next review comment would also be avoiding conflicts anyways |
you couldve flagged this last summer, about 5 month ago. Im gonna spend the time on apps where it isnt pointless |
|
@teamcons My notifications are extremely full. I reviewed Leonhard's branch because it was a very easy review to do and it happened to be in my notifications when I had a free moment. It's nothing personal against you, just a matter of circumstance. I'm sorry that you feel demotivated, but I fear the expectations you've placed on me to prioritize reviewing your branches are not something I can live up to. |


Fixes #515
And a 11 years old bug: https://bugs.launchpad.net/elementaryos/+bug/1308876
Ctrl+S allows saving the playlist, but i am not sure how to expose it in the UI. I included it so half the support isnt in Needs Design Hell.
the implementation skip extended M3U flags. Meaning Extm3u playlists will still work.
URL will be skipped, so the coverage is alas not complete
when looping through the files and upon finding a m3u file in the list, Application.loop_through_files just call the support method that knows how to read m3u, and replace it with an array of all the files in it
it is then up to playback to check what in there actually exists
Saving M3U pretty much just loops through the queue to put it all in a file.