The Sound Menu

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

  1. synchronous-notification
  2. Merge into trunk.15.04
Proposed by Ted Gould
Status: Merged
Approved by: Charles Kerr
Approved revision: 462
Merged at revision: 461
Proposed branch: lp:~ted/indicator-sound/synchronous-notification
Merge into: lp:indicator-sound/15.04
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) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+240200@code.launchpad.net

This proposal supersedes a proposal from 2014-10-08.

Commit message

Show notifications on volume change

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
Ted Gould (ted) wrote : Posted in a previous version of this proposal

Also this bug on desktop:

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

Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
Michał Sawicz (saviq) : Posted in a previous version of this proposal
Revision history for this message
Charles Kerr (charlesk) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Charles Kerr (charlesk) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:462
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/165/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-amd64-ci/32
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-sound-utopic-armhf-ci/32/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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

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

review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) :
review: Approve

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-10 15:40:19 +0000
3+++ src/volume-control.vala 2014-10-31 02:44:43 +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@@ -572,8 +581,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-31 02:44:43 +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