Merge lp:~grawity/beat-box/mpris into lp:beat-box

[Mantas Mikulėnas]

This removes MPRIS dependency on libindicate/libdbusmenu, which are hard to come by on non-Ubuntu systems, and fixes a few other issues (primarily broken PropertiesChanged signals).

[Kurt Smolderen (kurt.smolderen)]

Mantas,

Thanks for submitting some ideas to improve the code. However, some questions...
- When removing precompiler checks such as '#if HAVE_DBUS', you should make sure to also update the CMakeList.txt file to force the dependency on dbus. THe debus dependency remains required for MPRIS, even for non-Ubuntu users
- Playlists are still managed by libunity (if available at compile time). We are planning to suppoty these via MPRIS as well (although I think there are some limitations, but I'm going to post a comment on that in the original bug report)
- You should make sure Beatbox also triggers playPauseChanged when MprisPlayer.Stop() is called. Without doing so, Beatbox will stop playing music but for instance the Gnome3 media indicator applet still thinks music is playing...

What I want to propose: I've been working on this issue as well. Basic playlist support is implemented and I will probably push my revision this evening... I will incorpotate the addition of the track_id in the metadata if this is required by MPRIS...

Please let me know your thoughts on this and thanks for your input!

Kurt

[lp:~grawity/beat-box/mpris]

676. By Mantas Mikulėnas on 2012-07-07

MPRIS: Update PlaybackStatus on playback_stopped signal

[Mantas Mikulėnas (grawity)]

Thanks for your feedback.

> - When removing precompiler checks such as '#if HAVE_DBUS', you should make
> sure to also update the CMakeList.txt file to force the dependency on dbus.
> THe debus dependency remains required for MPRIS, even for non-Ubuntu users

HAVE_DBUS was never present in the first place, since BeatBox already depends on GLib and Gio and uses their internal DBus support (gdbus).

My patches only remove HAVE_DBUSMENU (libdbusmenu, a different library) from code that never required libdbusmenu.

> - You should make sure Beatbox also triggers playPauseChanged when
> MprisPlayer.Stop() is called. Without doing so, Beatbox will stop playing
> music but for instance the Gnome3 media indicator applet still thinks music is
> playing...

Should be fixed in revision 676.

(I wonder why playPauseChanged is part of LibraryWindow, but playback_stopped is in LibraryManager.)

> What I want to propose: I've been working on this issue as well. Basic
> playlist support is implemented and I will probably push my revision this
> evening... I will incorpotate the addition of the track_id in the metadata if
> this is required by MPRIS...

Good to hear that.

Yes, mpris:trackid is required, although mpristester[1] is probably the only program that will complain about its absence – unless you're also implementing the TrackList interface, but I'm not sure if there are any programs that actually make use of it.

[1]: https://github.com/randomguy3/mpristester/

[Scott Ringwelski (sgringwe)]

Mantas, thanks for the input you provided with the branch. It was (all?) used in Kurt's branch and was very helpful. Kurt's branch was just merged.

Thanks again!

review: Disapprove

[Mantas Mikulėnas (grawity)]

Thanks for the review.

There's one comment I still have, regarding build dependencies. One of Kurt's commits adds "dbusmenu-glib-0.4" to CMakeLists as a required dependency, noting that "DBUS is required dependency". However, as far as I know, the "dbusmenu" library is NOT required here as it only deals with the global Unity menubar and is practically useless on other distributions than Ubuntu. The support for DBus itself is already provided by "glib-2.0", and was sufficient for my branch on Arch Linux.

[Mantas Mikulėnas (grawity)]

Hmm, I had hoped that my rev 674 would get picked up too. Without it, BeatBox doesn't send out change signals at all.

[Scott Ringwelski (sgringwe)]

Thanks for your comments.

I added your rev 674 change. Out of curiousity, why is null preferred?
I also removed dbusmenu-glib-0.4 as removing that dependency worked on my system too.

Both are in rev 682.

On a sidenote, do you know anything about MprisPlaylist support? Kurt started it but couldn't quite get it 100% so it is currently commented out. If we could get MPRIS.vala to have full playlist support, we could take all media player stuff out of UnityIntegration.vala and avoid this whole mess.

[Mantas Mikulėnas (grawity)]

If I understand the documentation for GLib.DBusConnection.emit_signal() [1] correctly, the first argument is the busname to send signals *to* – if it is null, the signal is sent to all subscribers, but with the old code, the signals were sent only to BeatBox itself. (They still appear in dbus-monitor, but are not received by normal subscribers such as the GNOME Shell extension.)

Without this fix, GNOME would never notice any changes when I paused BeatBox or switched to a different song. (I'm not sure how Unity dealt with the previous behavior -- it might be polling the player for changes?)

[1]: http://valadoc.org/#!api=gio-2.0/GLib.DBusConnection.emit_signal

I haven't yet worked at all with the TrackList interface of MPRIS (and I don't know if any MPRIS clients make use of it). I will take a look at the commented-out implementation of Playlists, though it might be beyond my knowledge of BeatBox internals.

Unmerged revisions

676. By Mantas Mikulėnas on 2012-07-07

MPRIS: Update PlaybackStatus on playback_stopped signal

675. By Mantas Mikulėnas on 2012-07-06

MPRIS: Add mpris:trackid metadata, reduce code duplication

674. By Mantas Mikulėnas on 2012-07-06

MPRIS: Emit signals to all listeners

Signals sent to our own bus name never reach other programs.

673. By Mantas Mikulėnas on 2012-07-06

MPRIS: Fix range for xesam:userRating (must be 0.0~1.0)

672. By Mantas Mikulėnas on 2012-07-06

MPRIS: Export required MinimumRate, MaximumRate properties

671. By Mantas Mikulėnas on 2012-07-06

MPRIS: Fix xesam:genre metadata name

670. By Mantas Mikulėnas on 2012-07-06

MPRIS: Enable regardless of libindicate presence

=== modified file 'src/Core/LibraryWindow.vala'
--- src/Core/LibraryWindow.vala	2012-07-06 22:00:14 +0000
+++ src/Core/LibraryWindow.vala	2012-07-07 17:12:18 +0000
@@ -114,9 +114,6 @@
 #endif
 #endif
 		
-
-#if HAVE_INDICATE
-#if HAVE_DBUSMENU
 		message("Initializing MPRIS and sound menu\n");
 		var mpris = new BeatBox.MPRIS(lm, this);
 		mpris.initialize();
@@ -126,8 +123,6 @@
 		// that there isn't, the player will reshow when user tries to open
 		// it (app-uniqueness).
 		this.delete_event.connect(on_delete);
-#endif
-#endif
 
 		dragging_from_music = false;
 

=== modified file 'src/DBus/MPRIS/MPRIS.vala'
--- src/DBus/MPRIS/MPRIS.vala	2012-07-04 00:35:02 +0000
+++ src/DBus/MPRIS/MPRIS.vala	2012-07-07 17:12:18 +0000
@@ -18,8 +18,6 @@
  * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
  * Boston, MA 02111-1307, USA.
  */
-#if HAVE_INDICATE
-#if HAVE_DBUSMENU
 using Gee;
  
 public class BeatBox.MPRIS : GLib.Object {
@@ -48,12 +46,14 @@
 		if(owner_id == 0) {
 			warning("Could not initialize MPRIS session.\n");
 		}
+#if HAVE_INDICATE
+#if HAVE_DBUSMENU
 		else {
 			bool have_unity = false;
 			#if HAVE_UNITY
 			have_unity = true;
 			#endif
-			
+
 			// If Unity is running, let the implementation in UnityIntegration
 			// handle the sound menu since it is cleaner and has playlists.
 			// Otherwise, use this as a backup.
@@ -65,6 +65,8 @@
 				debug("It's up to unity to take care for sound menu integration");
 			}
 		}
+#endif
+#endif
 	}
 	
 	private void on_bus_acquired(DBusConnection connection, string name) {
@@ -219,8 +221,31 @@
 		
 		BeatBox.Beatbox.library_manager.media_played.connect(lm_media_played);
 		BeatBox.Beatbox.library_manager.medias_updated.connect(medias_updated);
+		BeatBox.Beatbox.library_manager.playback_stopped.connect(playback_stopped);
 		BeatBox.Beatbox.library_window.playPauseChanged.connect(playing_changed);
 	}
+
+	// MPRIS requires a mpris:trackid metadata item.
+	private GLib.ObjectPath get_trackid(BeatBox.Media s) {
+		string id = "/net/launchpad/beatbox/Track" + s.rowid.to_string();
+		return new GLib.ObjectPath(id);
+	}
+
+	private void fill_metadata(BeatBox.Media s) {
+		string[] artistArray = {};
+		artistArray += s.artist;
+		string[] genreArray = {};
+		genreArray += s.genre;
+
+		_metadata.insert("mpris:trackid", get_trackid(s));
+		_metadata.insert("xesam:artist", artistArray);
+		_metadata.insert("xesam:album", s.album);
+		_metadata.insert("xesam:title", s.title);
+		_metadata.insert("xesam:genre", genreArray);
+		_metadata.insert("mpris:artUrl", File.new_for_path(s.getAlbumArtPath()).get_uri());
+		_metadata.insert("mpris:length", BeatBox.Beatbox.library_manager.player.getDuration()/1000);
+		_metadata.insert("xesam:userRating", s.rating / 5.0);
+	}
 	
 	void medias_updated(Gee.LinkedList<int> ids) {
 		if(BeatBox.Beatbox.library_manager.media_info.media == null)
@@ -237,6 +262,10 @@
 	private void playing_changed() {
 		trigger_metadata_update();
 	}
+
+	private void playback_stopped(int wasPlaying) {
+		trigger_metadata_update();
+	}
 	
 	private void trigger_metadata_update() {
 		if(update_metadata_source != 0)
@@ -254,19 +283,7 @@
 	}
 	
 	void lm_media_played(BeatBox.Media s, BeatBox.Media? old) {
-		string[] artistArray = {};
-		artistArray += s.artist;
-		string[] genreArray = {};
-		genreArray += s.genre;
-		
-		_metadata.insert("xesam:artist", artistArray);
-		_metadata.insert("xesam:album", s.album);
-		_metadata.insert("xesam:title", s.title);
-		_metadata.insert("sesam:genre", genreArray);
-		_metadata.insert("mpris:artUrl", File.new_for_path(s.getAlbumArtPath()).get_uri());
-		_metadata.insert("mpris:length", BeatBox.Beatbox.library_manager.player.getDuration()/1000);
-		_metadata.insert("xesam:userRating", s.rating);
-		
+		fill_metadata(s);
 		trigger_metadata_update();
 	}
 	
@@ -286,7 +303,7 @@
 		changed_properties = null;
 		
 		try {
-			conn.emit_signal("org.mpris.MediaPlayer2.beatbox",
+			conn.emit_signal(null,
 			                 "/org/mpris/MediaPlayer2", 
 			                 "org.freedesktop.DBus.Properties", 
 			                 "PropertiesChanged", 
@@ -398,19 +415,7 @@
 			if(s == null)
 				return _metadata;
 			
-			string[] artistArray = {};
-			artistArray += s.artist;
-			string[] genreArray = {};
-			genreArray += s.genre;
-			
-			_metadata.insert("xesam:artist", artistArray);
-			_metadata.insert("xesam:album", s.album);
-			_metadata.insert("xesam:title", s.title);
-			_metadata.insert("sesam:genre", genreArray);
-			_metadata.insert("mpris:artUrl", File.new_for_path(s.getAlbumArtPath()).get_uri());
-			_metadata.insert("mpris:length", BeatBox.Beatbox.library_manager.player.getDuration()/1000);
-			_metadata.insert("xesam:userRating", s.rating);
-			
+			fill_metadata(s);
 			return _metadata;
 		}
 	}
@@ -430,7 +435,7 @@
 		}
 	}
 	
-	/*public double MinimumRate {
+	public double MinimumRate {
 		get {
 			return (double)1.0;
 		}
@@ -440,7 +445,7 @@
 		get {
 			return (double)1.0;
 		}
-	}*/
+	}
 
 	public bool CanGoNext {
 		get {
@@ -625,5 +630,3 @@
 	}
 	
 }*/
-#endif
-#endif