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

Proposed by Ted Gould on 2014-10-08
Status: Superseded
Proposed branch: lp:~ted/indicator-sound/synchronous-notification
Merge into: lp:indicator-sound/14.10
Prerequisite: lp:~ted/indicator-sound/revert-notifications
Diff against target: 94 lines (+53/-1)
2 files modified
src/volume-control.vala (+36/-1)
tests/manual (+17/-0)
To merge this branch: bzr merge lp:~ted/indicator-sound/synchronous-notification
Reviewer Review Type Date Requested Status
Charles Kerr (community) 2014-10-08 Approve on 2014-10-15
PS Jenkins bot (community) continuous-integration Approve on 2014-10-10
Review via email: mp+237666@code.launchpad.net

This proposal has been superseded by a proposal from 2014-10-31.

Commit message

Show notifications on volume change

To post a comment you must log in.
455. By Ted Gould on 2014-10-08

Make volume string translatable

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

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

462. By Ted Gould on 2014-10-10

Ensure we send an integer to the notification service

Michał Sawicz (saviq) :
Charles Kerr (charlesk) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/volume-control.vala'
2--- src/volume-control.vala 2014-10-08 18:17:29 +0000
3+++ src/volume-control.vala 2014-10-10 03:15:57 +0000
4@@ -19,6 +19,7 @@
5 */
6
7 using PulseAudio;
8+using Notify;
9 using Gee;
10
11 [CCode(cname="pa_cvolume_set", cheader_filename = "pulse/volume.h")]
12@@ -67,6 +68,7 @@
13 private uint _accountservice_volume_timer = 0;
14 private bool _send_next_local_volume = false;
15 private double _account_service_volume = 0.0;
16+ private Notify.Notification _notification;
17
18 public signal void volume_changed (double v);
19 public signal void mic_volume_changed (double v);
20@@ -84,6 +86,13 @@
21
22 _mute_cancellable = new Cancellable ();
23 _volume_cancellable = new Cancellable ();
24+
25+ Notify.init ("Volume");
26+ _notification = new Notify.Notification(_("Volume"), "", "audio-volume-muted");
27+ _notification.set_hint ("value", 0);
28+ _notification.set_hint ("x-canonical-private-synchronous", "true");
29+ _notification.set_hint ("x-canonical-non-shaped-icon", "true");
30+
31 setup_accountsservice.begin ();
32
33 this.reconnect_to_pulse ();
34@@ -568,8 +577,34 @@
35
36 public void set_volume (double volume)
37 {
38- if (set_volume_internal (volume))
39+ /* Using this to detect whether we're on the phone or not */
40+ if (_pulse_use_stream_restore) {
41+ if (volume == 0.0)
42+ _notification.update (_("Volume"), "", "audio-volume-muted");
43+ if (volume > 0.0 && volume <= 0.33)
44+ _notification.update (_("Volume"), "", "audio-volume-low");
45+ if (volume > 0.33 && volume <= 0.66)
46+ _notification.update (_("Volume"), "", "audio-volume-medium");
47+ if (volume > 0.66 && volume <= 1.0)
48+ _notification.update (_("Volume"), "", "audio-volume-high");
49+ _notification.set_hint ("value", (int32)(volume * 100.0));
50+ if (_active_sink_input == -1 || _valid_roles[_active_sink_input] != "multimedia") {
51+ /* No audio ping if we're playing multimedia */
52+ _notification.set_hint ("sound-file", "/usr/share/sounds/ubuntu/stereo/message.ogg");
53+ } else {
54+ _notification.set_hint ("sound-file", null);
55+ }
56+
57+ try {
58+ _notification.show ();
59+ } catch (GLib.Error e) {
60+ warning("Unable to send volume change notification: %s", e.message);
61+ }
62+ }
63+
64+ if (set_volume_internal (volume)) {
65 start_local_volume_timer();
66+ }
67 }
68
69 void set_mic_volume_success_cb (Context c, int success)
70
71=== modified file 'tests/manual'
72--- tests/manual 2014-01-31 20:09:51 +0000
73+++ tests/manual 2014-10-10 03:15:57 +0000
74@@ -22,3 +22,20 @@
75 <dd>The menu is populated with items</dd>
76 </dl>
77
78+Test-case indicator-sound/unity8-sound-notifications
79+<dl>
80+ <dt>Adjust volume using HW keys if available</dt>
81+ <dd>A notification bubble should appear with the sound volume</dd>
82+ <dd>An audibule sound should play at the level of the audio</dd>
83+ <dt>Adjust volume with slider in sound indicator</dt>
84+ <dd>A notification bubble should appear with the sound volume</dd>
85+ <dd>An audibule sound should play at the level of the audio</dd>
86+ <dt>Open a video with sound and play in media player</dt>
87+ <dd>The video should play and the sound should be audible</dd>
88+ <dt>Adjust volume using HW keys if available</dt>
89+ <dd>A notification bubble should appear with the sound volume</dd>
90+ <dd>No notification sound should be heard</dd>
91+ <dt>Adjust volume with slider in sound indicator</dt>
92+ <dd>A notification bubble should appear with the sound volume</dd>
93+ <dd>No notification sound should be heard</dd>
94+</dl>

Subscribers

People subscribed via source and target branches