Inconsistency between actual playlists and Unity's menu bar

[Clementine-Issue-Importer]

From biboudis on November 15, 2013 03:43:55

What steps will reproduce the problem? 1. Create new playlists

2. Close them
3. Close clementine
4. Reopen it
5. The last playlist named by the default name "Playlist" alongside some id concatenated to it, will appear at the menu bar of unity's taskbar.

The problem is that the "Playlist XX" that appears at the menu bar is empty and appears there as if it exists, while the playlist tab bar (playlisttabbar.h) is hidden (due to the fact that a playlist always resides at the bar). The arbitrary id XX that appears is the unique id that is returned from this line in this PlaylistManager::New(...) in playlistmanager.cpp:

int id = playlistbackend->CreatePlaylist(name, special_type);

and then it bubbles up at the Mpris2::GetPlaylists in mpris2.cpp when GetAllPlaylists is invoked.

I am using Clementine 1.2-62-ge929b4f.

Attachment: Screenshot from 2013-11-15 04:27:06.png

Original issue: http://code.google.com/p/clementine-player/issues/detail?id=3962

[Clementine-Issue-Importer]

From arnaud.bienner on November 15, 2013 01:22:05

I'm not sure to understand the actual problem: since release 1.2 we name playlist by adding a unique id at the end (so we don't end up with multiple playlists named "Playlist"). The tab-bar here is hidden, but this playlist is still named "Playlist 9". Maybe you expect the id to go back to "1" when all playlists are closed?

In your case, the playlist name isn't visible in Clementine because there is only one playlist opened, so the playlist tabbar is closed (this is the current behavior, but there was a proposal to change this: issue 1420 ), but this playlist is named "Playlist 9" anyway.

Labels: -Priority-Medium Priority-Low

[Clementine-Issue-Importer]

From biboudis on November 15, 2013 03:18:06

It seems counter intuitive to me to be able to select a playlist from the menu near that seems that it does not exist at the main panel. My proposed solution is to add the playlist at unity's menu bar only if the playlist has some songs in it. https://code.google.com/r/biboudis-clementine/source/detail?r=9caf0d9e5e2a98607fc9551215c83aac00842d3a

[Clementine-Issue-Importer]

From arnaud.bienner on November 15, 2013 10:09:07

I'm not sure... I understand it might be not intuitive to have "Playlist 9" written in the main panel while no "Playlist 9" is visible in the main panel. But to end up with a playlist called "Playlist 9", that means you were aware of the tabbar capabilities. If so, you might understand more easily that playlists are named with numbers. Otherwise, only "Playlist" is listed, which is less disturbing. So I don't know if it's worth to change this.

Maybe in the Unity integration part otherwise? To not show playlists list if there only one playlist?

About your patch (thanks for providing a patch btw :)) I'm afraid it might ignore empty playlists when you have multiple playlists, and I don't think it's good idea - I'm afraid it will look "buggy" to the user to not have all playlists listed here, while he can see them in Clementine. Even worse, for some streaming songs (e.g. Grooveshark songs) we don't have the "length" metadata info, so your patch will consider a playlist with only these kind of song as empty I guess (if I understood correctly what "GetTotalLength" does: returns the total length/time of the playlist, and not its size/number of tracks).

[Clementine-Issue-Importer]

From biboudis on November 16, 2013 06:33:17

Firstly, I could be more elaborate on the GetTotalLength, but I couldn't find something of use in the QAbstractListModel class to return size, length or count (and I didn't want to insert a new method at the API with 0 knowledge of QT). Secondly, I can see your point and I agree.

This was a good opportunity for me to play with Clementine's code a little bit, it may prove useful to some other code fix in the future now that I understand the architecture a little bit.

You can close the ticket. :-)

[Clementine-Issue-Importer]

From arnaud.bienner on November 16, 2013 10:49:40

While playing with the sound panel to check this behavior, I noticed the Playlists list isn't updated everytime or as soon as we close/delete a playlist. As a result, if we select a playlist which doesn't exist anymore from the sound panel, Clementine crashes :(

[Clementine-Issue-Importer]

From arnaud.bienner on November 16, 2013 10:54:29

I fixed this problem in revision 51d9080a81ef . Now I'm wondering why the sound panel isn't updated when it should be.

[Clementine-Issue-Importer]

From biboudis on November 17, 2013 10:13:26

Yes, indeed. It was the transition from the third step to the next one at my reproduction steps. I needed to close/reopen Clementine for the playlist list to refresh. I'll try to provide a proposal for fix (if I ever find out how you can for the menu bar to update and rebind the playlist menu item).

[Clementine-Issue-Importer]

From biboudis on November 17, 2013 12:06:02

I started doing the attached diff, but then I found out that I couldn't find a way to force a re-binding with GetPlaylists, only changes via the corresponding fields. http://specifications.freedesktop.org/mpris-spec/latest/Playlists_Interface.html#Method:GetPlaylists

Attachment: patch.diff

[Clementine-Issue-Importer]

From arnaud.bienner on November 17, 2013 13:59:15

I quickly checked this spec too and didn't find a way to refresh the playlists list :( As there is no "PlaylistsListChanged" signal, I'm not sure we can do anything about this.

At least we don't we don't crash anymore now :)

[Clementine-Issue-Importer]

From biboudis on November 17, 2013 14:53:14

If you send the command:

qdbus org.mpris.MediaPlayer2.clementine /org/mpris/MediaPlayer2 org.freedesktop.DBus.Properties.Get org.mpris.MediaPlayer2.Playlists PlaylistCount

It correctly reports the number of playlists open. If you pay attention closely however, the single playlist that the menu bar mentions is the 2nd from right to left. :P

[Clementine-Issue-Importer]

From biboudis on November 17, 2013 18:07:55

I think I fixed it. If the app posts PropertiesChanged on the playlist interface, the sound menu seems that updates itself. I suspected it from here: https://bugs.launchpad.net/indicator-sound/+bug/707042 My master contains the fix https://code.google.com/r/biboudis-clementine/ I have also included the diff between my HEAD and your upstream/master.

At least it seems that it is towards the right direction... :-)

Attachment: patch-3962.diff

[Clementine-Issue-Importer]

From arnaud.bienner on November 18, 2013 05:05:25

That looks super good! :) I will check/try your patch this evening, but after quickly reviewing it, it sounds promising :) Just few (minor things):

diff --git a/src/core/mpris2.cpp b/src/core/mpris2.cpp index d0a1ba9..f86e340 100644 --- a/src/core/mpris2.cpp +++ b/src/core/mpris2.cpp @@ -104,6 +104,8 @@ Mpris2::Mpris2(Application* app, Mpris1* mpris1, QObject* parent) connect(app_->playlistmanager(), SIGNAL(PlaylistManagerInitialized()), SLOT(PlaylistManagerInitialized())); connect(app->playlistmanager(), SIGNAL(CurrentSongChanged(Song)), SLOT(CurrentSongChanged(Song))); connect(app->playlistmanager(), SIGNAL(PlaylistChanged(Playlist)), SLOT(PlaylistChanged(Playlist_)));

• I don't think we need this extra line
• connect(app_->playlistmanager(), SIGNAL(CurrentChanged(Playlist)), SLOT(PlaylistCollectionChanged(Playlist_))); }

+void Mpris2::PlaylistCollectionChanged(Playlist* playlist){

• I don't think we need this extra line
• qLog(Debug) <<"Playlist collection changed (" Adding a space between << and "Play... would be nicer.

• <<playlist->id()

• <<")"; ditto

+++ b/src/core/mpris2.h @@ -191,6 +191,7 @@ signals:

// Playlist void PlaylistChanged(const MprisPlaylist& playlist);

• Don't need this extra line.

  private slots: void ArtLoaded(const Song& song, const QString& art_uri); @@ -202,10 +203,12 @@ private slots: void ShuffleModeChanged(); void RepeatModeChanged(); void PlaylistChanged(Playlist* playlist);

• void PlaylistCollectionChanged(Playlist* playlist = 0); Why having a default value for playlist? This way, someone can omit to pass a valid pointer, and as you're doing "playlist->id()" in PlaylistCollectionChanged, this doesn't like a bad idea. Unless this is a reason for this I missed, you should either drop this param or remove the "= 0".

diff --git a/src/playlist/playlistmanager.cpp b/src/playlist/playlistmanager.cpp index 0e8d383..5f27001 100644 --- a/src/playlist/playlistmanager.cpp +++ b/src/playlist/playlistmanager.cpp @@ -122,7 +122,7 @@ Playlist* PlaylistManager::AddPlaylist(int id, const QString& name, playlists_[id] = Data(ret, name);

emit PlaylistAdded(id, name, favorite);

Please remove this useless leading space added.

Labels: PatchAttached

[Clementine-Issue-Importer]

From biboudis on November 18, 2013 08:30:44

I think that it still needs some work for a rich sound menu bar (in terms of playlist management) but it is a good start. Thank you for your comments.

// just edited out typos, from my previous msg. :-)

Attachment: patch-3962.diff

[Clementine-Issue-Importer]

From arnaud.bienner on November 18, 2013 15:17:44

This issue was closed by revision 2647fe4bbc36 .

Status: Fixed

[Clementine-Issue-Importer]

From arnaud.bienner on November 18, 2013 15:21:12

Thanks a lot for the patch! :)

I don't know how we can improve the sound menu bar: as we try to be compliant with MPRIS interface, we can't really choose how information will be shown to user. But if you have some ideas about how to improve things, let us know.

[Clementine-Issue-Importer]

From biboudis on November 18, 2013 19:39:13

It still needs some work. After a while, playlist changes aren't reflected as I discovered today. However messages are transferred between the sound panel and clementine. Below is the output taken by the command dbus-monitor --profile.

as we see we post the PlaylistCount:

method return sender=:1.38 -> dest=:1.9 reply_serial=886 signal sender=:1.121 -> dest=(null destination) serial=223 path=/org/mpris/MediaPlayer2; interface=org.freedesktop.DBus.Properties; member=PropertiesChanged string "org.mpris.MediaPlayer2.Playlists" array [ dict entry( string "PlaylistCount" variant string "" ) ] array [ ]

which triggers from 1.45 the GetPlaylists method call (1.45 is com.canonical.indicator.sound):

method call sender=:1.45 -> dest=org.mpris.MediaPlayer2.clementine serial=425 path=/org/mpris/MediaPlayer2; interface=org.mpris.MediaPlayer2.Playlists; member=GetPlaylists uint32 0 uint32 100 string "Alphabetical" boolean false

method return sender=:1.121 -> dest=:1.45 reply_serial=425 array [ struct { object path "/org/mpris/MediaPlayer2/Playlists/29" string "Playlist 29" string "" } struct { object path "/org/mpris/MediaPlayer2/Playlists/30" string "Playlist 30" string "" } ]

In this particular time 2 playlists were returned (as you see from the structs for ids 29, 30 above) but the list wasn't updated. :(

[Clementine-Issue-Importer]

From biboudis on November 19, 2013 04:58:45

I found out that the sound menu includes the newly created playlists If and only if the new ones are different than the ones that have previously created. And I found this!!!! https://bugs.launchpad.net/indicator-sound/+bug/997693 Aha, such a simple and small quirk, so multidimensional behaviour. :D

[Clementine-Issue-Importer]

From arnaud.bienner on November 19, 2013 06:08:22

If our methods are called and returned the correct result, there is nothing we can do about it :( Indeed, it looks like a bug in the sound menu.