Skip to content
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

Playlists #85

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Playlists #85

wants to merge 2 commits into from

Conversation

rocus
Copy link

@rocus rocus commented Jun 19, 2015

MPD can load playlists (located on the MPD server). Sonata ignores those playlists. This modification shows playlists in the library view. You can add them to the current queue. If the playlist contains mp3's you can find these mp3's in your current queue: they will be played in due time. If the playlist contains one or more URL's (of radio streams), they will also be added to the current queue (and played later).

Most MPD clients ignore playlists, gmpc does not. Sonata changed by the suggested modifications in library.py and main.py will work as gmpc as far as playlists are concerned.

The playlist as implemented sofar in sonata (see playlist tab) are stored in the client (whithout directory structure). This modification stores them at the MPD server and they can be structured in directories.

screenshot from 2015-06-19 13 35 06

screenshot from 2015-06-19 13 32 23

@multani
Copy link
Owner

multani commented Jun 21, 2015

OK, I think I finally understand what you want.

Basically, you want the playlist files to appear in the Filesystem view of the library, right? It's probably a bit redundant with the Playlist tab, and the justification gmpc does it and Sonata doesn't isn't really one. But why not, I'm not against it.

I had a look at your code - can you at least squash your 2 commits into one and remove the comments you put? (we don't need ####)

I'm not a huge fan of duplicating all the playlist detection code: MPD already tells us which items are playlists and which are files, but we need to make additional comparisons against the icon (really?) and then again with the end of the string (really?). That's a lot of duplicated effort, not really understandable code (why comparing pixbuf to add or not an item?) and fragile code (what if one day we need to support other playlists than .pls and .m3u)
That's the sad state of Sonata unfortunately.

I'm going to make some comments on the code, please have look.

Also, could you provide a better icon for the playlist items in the Library view? Something which looks like more a playlist than something which looks like "song currently played".

for item in items:
self.mpd.add(item)
for item in items: ######
if item.endswith('.pls') or item.endswith('.PLS') or \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be extracted into a dedicated function, since I guess we'll have to support something else than .pls and .m3u playlists one day.
Also, this could be simplified if you convert your "item" value into lower/upper case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 21.06.2015 20:42, Jonathan Ballet wrote:

In sonata/main.py
#85 (comment):

@@ -1234,8 +1234,12 @@ def on_add_item(self, _widget, play_after=False):
if self.current_tab == self.TAB_LIBRARY:
items = self.library.get_path_child_filenames(True)
self.mpd.command_list_ok_begin()

  •            for item in items:
    
  •                self.mpd.add(item)
    
  •            for item in items:     ######
    
  •                 if item.endswith('.pls') or item.endswith('.PLS') or \
    

This should be extracted into a dedicated function, since I guess
we'll have to support something else than |.pls| and |.m3u| playlists
one day.
Also, this could be simplified if you convert your "item" value into
lower/upper case.


Reply to this email directly or view it on GitHub
https://github.com/multani/sonata/pull/85/files#r32896050.

I can do that but like you said there must be a better way to discern
between mp3's and pls's.

I can not remember that I somewhere found code that looks at the
extension and says: this is not an mp3 and therefore should not be shown
in the fileview window. Do you?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no place which looks at the extension of the file to know if the file is a playlist or not, but MPD "knows" and already tells you if it is (and you already used it before)
But, using this information is probably non-trivial, so simply refactor these few lines of could would be enough, I guess.

@rocus
Copy link
Author

rocus commented Jun 22, 2015

On 21.06.2015 20:39, Jonathan Ballet wrote:

OK, I think I finally understand what you want.

Basically, you want the playlist files to appear in the Filesystem
view of the library, right? It's probably a bit redundant with the
Playlist tab, and the justification gmpc does it and Sonata doesn't
isn't really one. But why not, I'm not against it.

You are right that "gmpc does it" is not a good argument. I just don't
understand why almost all mpd clients ignore playlists. While it is
relatively easy to handle them like mp3's. That was the reason I started
to look at sonata: playlist are files (like mp3's), they only must be
loaded (and not added) to the current queue.

I also find it a better idea to have playlists at the server (in the mpd
tree) than save them at the client like sonata does. Also for streams:
streams in a playlist file at a centralized location seems logical to me
and better than streams at the client sonatarc file.

At several fora I have seen discussions about playlists and mpd clients.
It confused me in the beginning what the difference is between Sonata
and Gmpc. Till someone talked about serverside and clientside. I think
the gmpc way is better than the way sonata does it. I start to wonder if
mpd maybe later got the ability to load playlists files (as a later
feature). If MPD got the playlist load feature from the beginning, I
really don't understand why many clients don't implement playlists or
implement them in the client.

I tried a mpd plugin with codi (xbmc) and there too it ignores
playlists. also mpd clients on android ignore playlists. Why?

I had a look at your code - can you at least squash your 2 commits
into one and remove the comments you put? (we don't need |####|)

I don't know what you mean by squash. I could ofcourse edit those files
and commit them again. Push them to my github branch. That would be OK?

I'm not a huge fan of duplicating all the playlist detection code: MPD
already tells us which items are playlists and which are files, but we
need to make additional comparisons against the icon (really?) and
then again with the end of the string (really?). That's a lot of
duplicated effort, not really understandable code (why comparing
pixbuf to add or not an item?) and fragile code (what if one day we
need to support other playlists than |.pls| and |.m3u|)

You are absolutely right that the code is not very nice. I still
understand only 30% of the code of sonata and I could not find the
place/attributes by which sonata knows if an item is an mp3 or playlist
file. When I understand sonata better I will maybe find a nicer solution.

I'm going to make some comments on the code, please have look.

Also, could you provide a better icon for the playlist items in the
Library view? Something which looks like more a playlist than
something which looks like "song currently played".

I was planning to make in due time an icon but I ran into ploblems with
an icon cache (as I remenmber) so I just took an (unused?) sonata icon.
At the moment I think this icon does works, it looks like a mp3 but
suggests a bit more.
I tried to use some other icon (at the start) but did not succeed in
calling it: I thought that there maybe was a (python) cache or
something. Then I used this, unused, sonata icon.

Jonathan, you must understand that to me there are many new things here:

Sonata, not a trivial small program
Python new to me but worth the effort I think
Git, never worked with it (I used to program alone or in a small team)
gtk completely new to me and certainly not trivial

So bear with me if I can not handle it all as you would like it. I am
doing my best...

Reply to this email directly or view it on GitHub
#85 (comment).

@multani
Copy link
Owner

multani commented Jun 22, 2015

On 06/22/2015 05:25 PM, Rocus wrote:

I also find it a better idea to have playlists at the server (in the mpd
tree) than save them at the client like sonata does. Also for streams:
streams in a playlist file at a centralized location seems logical to me
and better than streams at the client sonatarc file.

At several fora I have seen discussions about playlists and mpd clients.
It confused me in the beginning what the difference is between Sonata
and Gmpc. Till someone talked about serverside and clientside. I think
the gmpc way is better than the way sonata does it. I start to wonder if
mpd maybe later got the ability to load playlists files (as a later
feature). If MPD got the playlist load feature from the beginning, I
really don't understand why many clients don't implement playlists or
implement them in the client.

There's no "server side" or "client side" playlists.
MPD provides a way to remotely control (create, update, delete)
playlists, without having to touch the filesystem, and that's the basis
of MPD: it's a server which provides clients a way to remotely control
playback of song files. As far as I know, at no point MPD ever changes
the music directory being served. And the clients surely are not
supposed to even access these files in the first place (although it's
possible).
Now, MPD provides this playlist feature because how do you even create
playlists if you don't have access to a filesystem to write the
playlists onto?
Furthermore, MPD becomes the place where all the playlists are
available, whereas you are using Sonata on a remote computer, a client
on your mobile phone when you are somewhere else, or a web interface if
you are in another country. Playlists are stored on the server, and you
can even choose in the MPD configuration where you want to store them.

Sonata, like all (most?) of the other MPD clients are supporting this
MPD feature, which is fine FWIW.
GMPC, besides being able to also load playlists from the filesystem,
does exactly the same as the other clients, and relies on MPD for all
the other load/save functionality related to playlists.

So, being able to load playlists from the filesystem, I'm still not sure
what your point is.

I had a look at your code - can you at least squash your 2 commits
into one and remove the comments you put? (we don't need |####|)

I don't know what you mean by squash. I could ofcourse edit those files
and commit them again. Push them to my github branch. That would be OK?

I meant, can you make one commit from your two commits. That's probably
moderately advanced Git usage though; using Git rebase --interactive
would be a good start, but this ticket is probably not the best place to
be a tutorial on Git.
You will probably find good documentation on how to do that if you look
into your search engine for "how to squash 2 commits with Git".

I'm not a huge fan of duplicating all the playlist detection code: MPD
already tells us which items are playlists and which are files, but we
need to make additional comparisons against the icon (really?) and
then again with the end of the string (really?). That's a lot of
duplicated effort, not really understandable code (why comparing
pixbuf to add or not an item?) and fragile code (what if one day we
need to support other playlists than |.pls| and |.m3u|)

You are absolutely right that the code is not very nice. I still
understand only 30% of the code of sonata and I could not find the
place/attributes by which sonata knows if an item is an mp3 or playlist
file. When I understand sonata better I will maybe find a nicer solution.

The place where MPD tells you if an item is a file or a playlist is this
place:
https://github.com/multani/sonata/pull/85/files#diff-7b9aa4ce036b0612cc050d5b1fd01519R548
(sonata/library.py, line 548).

This change is not trivial though. Making the function I asked you in
the diff would already be a good start.

I'm going to make some comments on the code, please have look.

Also, could you provide a better icon for the playlist items in the
Library view? Something which looks like more a playlist than
something which looks like "song currently played".

I was planning to make in due time an icon but I ran into ploblems with
an icon cache (as I remenmber) so I just took an (unused?) sonata icon.
At the moment I think this icon does works, it looks like a mp3 but
suggests a bit more.
I tried to use some other icon (at the start) but did not succeed in
calling it: I thought that there maybe was a (python) cache or
something. Then I used this, unused, sonata icon.

You can find the icon definition in the sonata/ui/icons.glade file, it
should be pretty self-explanatory. From there, it should be
straightforward to load the icon as the other icons.

Jonathan, you must understand that to me there are many new things here:

Sonata, not a trivial small program
Python new to me but worth the effort I think
Git, never worked with it (I used to program alone or in a small team)
gtk completely new to me and certainly not trivial

So bear with me if I can not handle it all as you would like it. I am
doing my best...

I appreciate your effort in improving Sonata and I certainly realise you
are doing your best. I spent time to make these comments with the hope
that you will improve your skills so next time you provide patches on
Sonata on another similar project, it will be easier for everybody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants