Merge lp:~ted/indicator-sound/synchronous-notification into lp:indicator-sound/14.10

[Ted Gould]

Show notifications on volume change

Unrevert the notifications. Requires fixes comments on the following bugs:

[silo] https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1378618
[?] https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1378920
[done] https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1378961
[done] https://bugs.launchpad.net/ubuntu/+source/indicator-sound/+bug/1379287
[done] https://bugs.launchpad.net/indicator-sound/+bug/1378564

[lp:~ted/indicator-sound/synchronous-notification]

455. By Ted Gould on 2014-10-08

Make volume string translatable

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:455
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/153/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/20/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/153/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

Also this bug on desktop:

https://bugs.launchpad.net/ubuntu/+source/indicator-sound/+bug/1379287

[Michał Sawicz (saviq)]

See https://code.launchpad.net/~macslow/indicator-sound/synchronous-notification/+merge/229061/comments/583086 for an issue I found with previous implementation.

Also, please land together with lp:~macslow/unity8/synchronous-notification and lp:~macslow/unity-notifications/synchronous-notification

[lp:~ted/indicator-sound/synchronous-notification]

456. By Ted Gould on 2014-10-09

No audio ping if we're doing multimedia, and no notification if we're not on phone

457. By Ted Gould on 2014-10-09

Adding a notifications test case

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:457
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/154/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/21/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/154/rebuild

review: Approve (continuous-integration)

[lp:~ted/indicator-sound/synchronous-notification]

458. By Ted Gould on 2014-10-10

Make sure to clear the sound hint

459. By Ted Gould on 2014-10-10

Catch the error that show can throw and present a warning

460. By Ted Gould on 2014-10-10

Make sure to use the passed parameter for the volume.

461. By Ted Gould on 2014-10-10

Didn't make initial string translated

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:460
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/155/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/22/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/155/rebuild

review: Approve (continuous-integration)

[lp:~ted/indicator-sound/synchronous-notification]

462. By Ted Gould on 2014-10-10

Ensure we send an integer to the notification service

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:462
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/156/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-amd64-ci/23
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/23
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/23/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/156/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Short & sweet, I like it.

A couple of changes that would be nice-to-have:

1. The new set_volume() notification code overlaps a lot with IndicatorSound.Service's existing activate_scroll_action() method, but with some differences, e.g. icon naming of "notification-audio-volume-off" vs "audio-volume-muted". Should these blocks be using the same icon names? If so, should the similar code be extracted into its own method where it can be shared?

2. Trivial change, "/usr/share/sounds/ubuntu/stereo/message.ogg" should be a symbolic somewhere, rather than hardcoded in the middle of the source file. Yawn.

[Charles Kerr (charlesk)]

review: Approve

Unmerged revisions

462. By Ted Gould on 2014-10-10

Ensure we send an integer to the notification service

461. By Ted Gould on 2014-10-10

Didn't make initial string translated

460. By Ted Gould on 2014-10-10

Make sure to use the passed parameter for the volume.

459. By Ted Gould on 2014-10-10

Catch the error that show can throw and present a warning

458. By Ted Gould on 2014-10-10

Make sure to clear the sound hint

457. By Ted Gould on 2014-10-09

Adding a notifications test case

456. By Ted Gould on 2014-10-09

No audio ping if we're doing multimedia, and no notification if we're not on phone

455. By Ted Gould on 2014-10-08

Make volume string translatable

454. By Ted Gould on 2014-10-08

Reenable notifications

453. By Ted Gould on 2014-10-08

Update to trunk

=== modified file 'src/volume-control.vala'
--- src/volume-control.vala	2014-10-08 18:17:29 +0000
+++ src/volume-control.vala	2014-10-10 03:15:57 +0000
@@ -19,6 +19,7 @@
  */
 
 using PulseAudio;
+using Notify;
 using Gee;
 
 [CCode(cname="pa_cvolume_set", cheader_filename = "pulse/volume.h")]
@@ -67,6 +68,7 @@
 	private uint _accountservice_volume_timer = 0;
 	private bool _send_next_local_volume = false;
 	private double _account_service_volume = 0.0;
+	private Notify.Notification _notification;
 
 	public signal void volume_changed (double v);
 	public signal void mic_volume_changed (double v);
@@ -84,6 +86,13 @@
 
 		_mute_cancellable = new Cancellable ();
 		_volume_cancellable = new Cancellable ();
+
+		Notify.init ("Volume");
+		_notification = new Notify.Notification(_("Volume"), "", "audio-volume-muted");
+		_notification.set_hint ("value", 0);
+		_notification.set_hint ("x-canonical-private-synchronous", "true");
+		_notification.set_hint ("x-canonical-non-shaped-icon", "true");
+
 		setup_accountsservice.begin ();
 
 		this.reconnect_to_pulse ();
@@ -568,8 +577,34 @@
 
 	public void set_volume (double volume)
 	{
-		if (set_volume_internal (volume))
+		/* Using this to detect whether we're on the phone or not */
+		if (_pulse_use_stream_restore) {
+			if (volume == 0.0)
+				_notification.update (_("Volume"), "", "audio-volume-muted");
+			if (volume > 0.0 && volume <= 0.33)
+				_notification.update (_("Volume"), "", "audio-volume-low");
+			if (volume > 0.33 && volume <= 0.66)
+				_notification.update (_("Volume"), "", "audio-volume-medium");
+			if (volume > 0.66 && volume <= 1.0)
+				_notification.update (_("Volume"), "", "audio-volume-high");
+			_notification.set_hint ("value", (int32)(volume * 100.0));
+			if (_active_sink_input == -1 || _valid_roles[_active_sink_input] != "multimedia") {
+				/* No audio ping if we're playing multimedia */
+				_notification.set_hint ("sound-file", "/usr/share/sounds/ubuntu/stereo/message.ogg");
+			} else {
+				_notification.set_hint ("sound-file", null);
+			}
+
+			try {
+				_notification.show ();			
+			} catch (GLib.Error e) {
+				warning("Unable to send volume change notification: %s", e.message);
+			}
+		}
+
+		if (set_volume_internal (volume)) {
 			start_local_volume_timer();
+		}
 	}
 
 	void set_mic_volume_success_cb (Context c, int success)

=== modified file 'tests/manual'
--- tests/manual	2014-01-31 20:09:51 +0000
+++ tests/manual	2014-10-10 03:15:57 +0000
@@ -22,3 +22,20 @@
 		<dd>The menu is populated with items</dd>
 </dl>
 
+Test-case indicator-sound/unity8-sound-notifications
+<dl>
+	<dt>Adjust volume using HW keys if available</dt>
+		<dd>A notification bubble should appear with the sound volume</dd>
+		<dd>An audibule sound should play at the level of the audio</dd>
+	<dt>Adjust volume with slider in sound indicator</dt>
+		<dd>A notification bubble should appear with the sound volume</dd>
+		<dd>An audibule sound should play at the level of the audio</dd>
+	<dt>Open a video with sound and play in media player</dt>
+		<dd>The video should play and the sound should be audible</dd>
+	<dt>Adjust volume using HW keys if available</dt>
+		<dd>A notification bubble should appear with the sound volume</dd>
+		<dd>No notification sound should be heard</dd>
+	<dt>Adjust volume with slider in sound indicator</dt>
+		<dd>A notification bubble should appear with the sound volume</dd>
+		<dd>No notification sound should be heard</dd>
+</dl>