Merge lp:~kurt.smolderen/beat-box/beat-box into lp:beat-box

[Scott Ringwelski]

Kurt's sound menu changes

[Scott Ringwelski (sgringwe)]

Hey Kurt, it's easier to just use merges rather than lots of patches.

Thanks for the work! Some things for you to work on further:

1. What if user clicks close while music is playing but DOESN'T have soundmenu to reshow it? It would be really bad for the window to be "lost" meaning it is running but not accessible and has to be killed.
2. What kind of performance regression is had by checking for sound menu or unity at startup? Also, I'm not positive, but I'm pretty sure those checks are dead code meaning HAVE_INDICATE is set during compile time anyways...
3. Your rev 623 change for the raised signal should also be implemented in MPRIS.vala, which is the fallback file for when a user does not have unity but still has MPRIS services (UnityIntegration.vala uses libunity, and libunity is just a higher level api for MPRIS).
4. What if the user is in the middle of importing their music library (or other similar operations) and clicks close?

[Scott Ringwelski (sgringwe)]

review: Needs Fixing

[Kurt Smolderen (kurt.smolderen)]

Hey Scot,

Some feedback inline...

Regards
Kurt

On 20-06-12 23:50, Scott Ringwelski wrote:
> Hey Kurt, it's easier to just use merges rather than lots of patches.
>
> Thanks for the work! Some things for you to work on further:
>
> 1. What if user clicks close while music is playing but DOESN'T have soundmenu to reshow it? It would be really bad for the window to be "lost" meaning it is running but not accessible and has to be killed.
If soundmenu is not running, the hook which hides the main window
instead of closing it is not set (so it still points to destroy in this
case...)
> 2. What kind of performance regression is had by checking for sound menu or unity at startup? Also, I'm not positive, but I'm pretty sure those checks are dead code meaning HAVE_INDICATE is set during compile time anyways...
About 0.035 seconds on my system (Core i3 2.2GHz, 4GB ram).
HAVE_INDICATE is set during compile time indeed but that's ok. If the
indicate library is not found, we do not want any code relying on that
library to be compiled. The reason why this change is needed (according
to me at least) is because in the MPRIS class the creation of the Sound
menu integration object was let to the UnityIntegration object is
HAVE_UNITY was set. So when running Gnome Classic, unity library is
present but unity itself is not running and hence it is not optimal...
> 3. Your rev 623 change for the raised signal should also be implemented in MPRIS.vala, which is the fallback file for when a user does not have unity but still has MPRIS services (UnityIntegration.vala uses libunity, and libunity is just a higher level api for MPRIS).
This was correctly implemented in the MPRIS class...
> 4. What if the user is in the middle of importing their music library (or other similar operations) and clicks close?
At this point, the same would happen as when this patch is not
integrated I think as the Gtk Window will just handle the 'click on
close' event as it would do without this patch...

[Scott Ringwelski (sgringwe)]

Hmmm looks like I should have looked at the code a little more before I asked those questions. You are absolutely right.

As for #4, you are right on that too, but I was hoping to see if you would be willing to implement that :). I can do that though.

I really like your changes, and I am not worried about 0.035 seconds for the added stability/robustness. There are ways to gain that back and much more in better ways.

I'm going to go ahead and merge this. I wasn't sure about the sound menu hide/show ux, but I am ok with it now. Plus I know it will make lots of users happy.

If you want to help with the project more, please let me know :)

review: Approve

[lp:~kurt.smolderen/beat-box/beat-box]

622. By Scott Ringwelski on 2012-06-21

Merged Kurt Smolderen's sound menu/unity changes.

[Kurt Smolderen (kurt.smolderen)]

#4: OK, didn't interpret the question/ remark that way :-), but it's
not a problem to have a look at it, because you have a point there are
some other situations for which it's preferable the application to be
hidden instead of closed (importing library, ripping CD,...). I'll have
a look at it...

What would be the expected behaviour? I would suggest:

  * Check whether a 'non-interruptible' task is going on
  * If it is
not: close the program
  * If it is:

  * Finish the task
  * Check
again whether a non-interruptible task is going on
  * etc...

BTW: for
your interest: I'm currently working on a solution for
https://bugs.launchpad.net/beat-box/+bug/1014554. So yes, I'm kind of
interested in supporting this project. But as my Vala knowledge is
pretty limited at this stage, I'm cvurrently picking up the 'easy to
fix' bugs to get into the language and into the program structure... But
I do hope to send you a few more merge requests :-)

Regards Kurt

On
21.06.2012 04:50, Scott Ringwelski wrote:

> Review: Approve
>
> Hmmm
looks like I should have looked at the code a little more before I asked
those questions. You are absolutely right.
>
> As for #4, you are right
on that too, but I was hoping to see if you would be willing to
implement that :). I can do that though.
>
> I really like your
changes, and I am not worried about 0.035 seconds for the added
stability/robustness. There are ways to gain that back and much more in
better ways.
>
> I'm going to go ahead and merge this. I wasn't sure
about the sound menu hide/show ux, but I am ok with it now. Plus I know
it will make lots of users happy.
>
> If you want to help with the
project more, please let me know :)
> --
https://code.launchpad.net/~kurt.smolderen/beat-box/beat-box/+merge/111311
You are the owner of lp:~kurt.smolderen/beat-box/beat-box.

[Scott Ringwelski (sgringwe)]

That's roughly what I was thinking. Really, you can just check for
lm.doing_file_operations the same way you do for lm.playing,and then add a
signal handler to file_operations_finished to potentially close if it is
hidden (maybe do same with playback_stopped?)

Remember though that merges should be specific to the feature you are
working on.

For https://bugs.launchpad.net/beat-box/+bug/1014554 look for key_pressed
in library window

Thanks!

On Thu, Jun 21, 2012 at 4:40 AM, Kurt Smolderen <email address hidden>wrote:

>
>
> #4: OK, didn't interpret the question/ remark that way :-), but it's
> not a problem to have a look at it, because you have a point there are
> some other situations for which it's preferable the application to be
> hidden instead of closed (importing library, ripping CD,...). I'll have
> a look at it...
>
> What would be the expected behaviour? I would suggest:
>
>
> * Check whether a 'non-interruptible' task is going on
> * If it is
> not: close the program
> * If it is:
>
> * Finish the task
> * Check
> again whether a non-interruptible task is going on
> * etc...
>
> BTW: for
> your interest: I'm currently working on a solution for
> https://bugs.launchpad.net/beat-box/+bug/1014554. So yes, I'm kind of
> interested in supporting this project. But as my Vala knowledge is
> pretty limited at this stage, I'm cvurrently picking up the 'easy to
> fix' bugs to get into the language and into the program structure... But
> I do hope to send you a few more merge requests :-)
>
> Regards Kurt
>
> On
> 21.06.2012 04:50, Scott Ringwelski wrote:
>
> > Review: Approve
> >
> > Hmmm
> looks like I should have looked at the code a little more before I asked
> those questions. You are absolutely right.
> >
> > As for #4, you are right
> on that too, but I was hoping to see if you would be willing to
> implement that :). I can do that though.
> >
> > I really like your
> changes, and I am not worried about 0.035 seconds for the added
> stability/robustness. There are ways to gain that back and much more in
> better ways.
> >
> > I'm going to go ahead and merge this. I wasn't sure
> about the sound menu hide/show ux, but I am ok with it now. Plus I know
> it will make lots of users happy.
> >
> > If you want to help with the
> project more, please let me know :)
> > --
> https://code.launchpad.net/~kurt.smolderen/beat-box/beat-box/+merge/111311
> You are the owner of lp:~kurt.smolderen/beat-box/beat-box.
>
>
> --
> https://code.launchpad.net/~kurt.smolderen/beat-box/beat-box/+merge/111311
> You proposed lp:~kurt.smolderen/beat-box/beat-box for merging.
>

[Kurt Smolderen (kurt.smolderen)]

Changes for #4 have been pushed to my branch. Once all file operations
are finished, Beatbox is closed. I did not implement 'close on
playback_stopped' as this would require the user to completely restart
beatbox once his queue is empty...

Regards
Kurt

On 21-06-12 14:28, Scott Ringwelski wrote:
> That's roughly what I was thinking. Really, you can just check for
> lm.doing_file_operations the same way you do for lm.playing,and then add a
> signal handler to file_operations_finished to potentially close if it is
> hidden (maybe do same with playback_stopped?)
>
> Remember though that merges should be specific to the feature you are
> working on.
>
> For https://bugs.launchpad.net/beat-box/+bug/1014554 look for key_pressed
> in library window
>
> Thanks!
>
> On Thu, Jun 21, 2012 at 4:40 AM, Kurt Smolderen<email address hidden>wrote:
>
>>
>> #4: OK, didn't interpret the question/ remark that way :-), but it's
>> not a problem to have a look at it, because you have a point there are
>> some other situations for which it's preferable the application to be
>> hidden instead of closed (importing library, ripping CD,...). I'll have
>> a look at it...
>>
>> What would be the expected behaviour? I would suggest:
>>
>>
>> * Check whether a 'non-interruptible' task is going on
>> * If it is
>> not: close the program
>> * If it is:
>>
>> * Finish the task
>> * Check
>> again whether a non-interruptible task is going on
>> * etc...
>>
>> BTW: for
>> your interest: I'm currently working on a solution for
>> https://bugs.launchpad.net/beat-box/+bug/1014554. So yes, I'm kind of
>> interested in supporting this project. But as my Vala knowledge is
>> pretty limited at this stage, I'm cvurrently picking up the 'easy to
>> fix' bugs to get into the language and into the program structure... But
>> I do hope to send you a few more merge requests :-)
>>
>> Regards Kurt
>>
>> On
>> 21.06.2012 04:50, Scott Ringwelski wrote:
>>
>>> Review: Approve
>>>
>>> Hmmm
>> looks like I should have looked at the code a little more before I asked
>> those questions. You are absolutely right.
>>> As for #4, you are right
>> on that too, but I was hoping to see if you would be willing to
>> implement that :). I can do that though.
>>> I really like your
>> changes, and I am not worried about 0.035 seconds for the added
>> stability/robustness. There are ways to gain that back and much more in
>> better ways.
>>> I'm going to go ahead and merge this. I wasn't sure
>> about the sound menu hide/show ux, but I am ok with it now. Plus I know
>> it will make lots of users happy.
>>> If you want to help with the
>> project more, please let me know :)
>>> --
>> https://code.launchpad.net/~kurt.smolderen/beat-box/beat-box/+merge/111311
>> You are the owner of lp:~kurt.smolderen/beat-box/beat-box.
>>
>>
>> --
>> https://code.launchpad.net/~kurt.smolderen/beat-box/beat-box/+merge/111311
>> You proposed lp:~kurt.smolderen/beat-box/beat-box for merging.
>>

=== modified file 'src/Core/LibraryWindow.vala'
--- src/Core/LibraryWindow.vala	2012-06-19 22:50:13 +0000
+++ src/Core/LibraryWindow.vala	2012-06-20 21:41:18 +0000
@@ -24,6 +24,9 @@
 using Gee;
 using Notify;
 
+const string PROCESS_INDICATOR_SOUND = "indicator-sound-service";
+const string PROCESS_UNITY = "unity-panel-service";
+
 public class BeatBox.LibraryWindow : LibraryWindowInterface, Gtk.Window {
 
 	// signals
@@ -88,6 +91,10 @@
 	private Gtk.MenuItem fileRescanMusicFolder;
 	private Gtk.MenuItem editEqualizer;
 	private ImageMenuItem editPreferences;
+	
+	// Unity and sound menu integration flags
+	public bool enableUnityIntegration { get; private set; }
+	public bool enableSoundmenuIntegration { get; private set; }
 
 	public Notify.Notification notification { get; private set; }
 
@@ -106,21 +113,31 @@
 		
 		//various objects
 		mkl = new MediaKeyListener(lm, this);
+		
+		// Check for Unity and sound menu availability
+		enableUnityIntegration = is_process_running(PROCESS_UNITY);
+		enableSoundmenuIntegration = is_process_running(PROCESS_INDICATOR_SOUND);
 
 #if HAVE_INDICATE
 #if HAVE_DBUSMENU
-		message("Initializing MPRIS and sound menu\n");
-		var mpris = new BeatBox.MPRIS(lm, this);
-		mpris.initialize();
+		if (enableSoundmenuIntegration) {
+			message("Initializing MPRIS and sound menu\n");
+			var mpris = new BeatBox.MPRIS(lm, this);
+			mpris.initialize();
+			this.delete_event.connect(on_delete);
+		}
 #endif
 #endif
 
 #if HAVE_UNITY
 #if HAVE_DBUSMENU
-		message("Initializing Unity integration\n");
-		unity = new BeatBox.UnityIntegration(lm, this);
-		if(!unity.initialize())
-			warning("Unity integration failed\n");
+		if (enableUnityIntegration) {
+			message("Initializing Unity integration\n");
+			unity = new BeatBox.UnityIntegration(lm, this);
+			if(!unity.initialize()) {
+				warning("Unity integration failed\n");
+			}
+		}
 #endif
 #endif
 
@@ -996,7 +1013,16 @@
 		w.focus(DirectionType.UP);
 	}
 
-	 void on_quit() {
+	bool on_delete() {
+		bool hide_on_close = false;
+		if (lm.playing) {
+			this.hide();
+			hide_on_close = true;
+		}
+		return hide_on_close;
+	}
+
+	void on_quit() {
 		lm.settings.setLastMediaPosition((int)((double)lm.player.getPosition()/1000000000));
 		if(lm.media_active) {
 			lm.media_info.media.resume_pos = (int)((double)lm.player.getPosition()/1000000000);
@@ -1421,5 +1447,21 @@
 		now_playing.hide_video_mode();
 		showSongInfo.set_image(Icons.INFO.render_image (IconSize.MENU, viewSelector.get_style_context()));
 	}
+	
+	bool is_process_running(string process_name) {
+		bool is_running = false;
+		string cmd = "pidof " + process_name;
+		string pid;
+		try {
+			Process.spawn_command_line_sync(cmd, out pid);
+			debug("PID of %s:  %s", process_name, pid);
+			if (pid.length > 0) {
+				is_running = true;
+			}
+		} catch (SpawnError e) {
+			debug("Unable to spawn process: %s", e.message);
+		}
+		return is_running;
+	}
 }
 

=== modified file 'src/DBus/MPRIS/MPRIS.vala'
--- src/DBus/MPRIS/MPRIS.vala	2012-06-19 22:50:13 +0000
+++ src/DBus/MPRIS/MPRIS.vala	2012-06-20 21:41:18 +0000
@@ -49,18 +49,16 @@
 			warning("Could not initialize MPRIS session.\n");
 		}
 		else {
-			bool have_unity = false;
-#if HAVE_UNITY
-			have_unity = true;
-#endif
-			
-			// If we have libunity, let the implementation in UnityIntegration
+			// 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.
-			if(!have_unity) {
+			if(lw.enableSoundmenuIntegration && !lw.enableUnityIntegration) {
 				var soundMenu = new SoundMenuIntegration(lm, lw);
 				soundMenu.initialize();
 			}
+			else {
+				debug("It's up to unity to take care for sound menu integration");
+			}
 		}
 	}
 	

=== modified file 'src/DBus/UnityIntegration.vala'
--- src/DBus/UnityIntegration.vala	2012-06-01 22:54:35 +0000
+++ src/DBus/UnityIntegration.vala	2012-06-20 21:41:18 +0000
@@ -94,6 +94,7 @@
 		player.previous.connect(lw.previousClicked);
 		player.play_pause.connect(lw.playClicked);
 		player.activate_playlist.connect(playlist_activated);
+		player.raise.connect(lw.present);
 		
 		lw.playPauseChanged.connect(playing_changed);