Indicator Date and Time

Merge lp://staging/~d.filoni/indicator-datetime/lp1364647 into lp://staging/indicator-datetime

  1. lp1364647
  2. Merge into trunk.16.10
Proposed by Devid Antonio Filoni
Status: Needs review
Proposed branch: lp://staging/~d.filoni/indicator-datetime/lp1364647
Merge into: lp://staging/indicator-datetime
Diff against target: 478 lines (+380/-2)
5 files modified
CMakeLists.txt (+2/-1)
INSTALL (+1/-0)
debian/control (+1/-0)
include/notifications/sound.h (+1/-0)
src/sound.cpp (+375/-1)
To merge this branch: bzr merge lp://staging/~d.filoni/indicator-datetime/lp1364647
Reviewer Review Type Date Requested Status
Charles Kerr Pending
Tiago Salem Herrmann Pending
Indicator Applet Developers Pending
Review via email: mp+303494@code.staging.launchpad.net

Description of the change

Hi,
commit 460 MR introduces a libpulse implementation in order to set output port to both spearkers and headphones when playing an alarm as suggested by Pat in comment #17 (bug #1364647) and as already done in telepathy-ofono ( [1] )

commit 461 introduces a g_timeout_add_seconds call to set output port after 5 seconds since alarm start. I'm not sure 5 is a reasonable amount of time, in bug #1583981 (equivalent issue for ringtone) there is a linked spec but I don't have access to it.

This was tested on Nexus 4 (there is a build in my mako PPA of the same patch).

[1] http://bazaar.launchpad.net/~phablet-team/telepathy-ofono/trunk/view/head:/qpulseaudioengine.cpp

To post a comment you must log in.
Revision history for this message
Devid Antonio Filoni (d.filoni) wrote :

I found an issue testing commit 461 and fixed it in commit 462: I unplugged headphones after 5 seconds from alarm start (so when port switched to speakers) and closing the notification port was reset to headphones, so audio was not working. With commit 462 headphones port is not restored if it's not available anymore.

Reply
Revision history for this message
Charles Kerr (charlesk) wrote :

The patch looks OK and we could take it as an interim step, but IMO these sound issues would be better handled in unity8 / unity-notifications.

IMO what indicator-datetime should be doing is passng the 'sound-file' hint to notifications. The reason it's not doing that right now is the need to loop the sound, but if unity8/unity-notifications supported a 'loop' hint then we could do away with the custom sound code in indicator-datetime completely.

As for the code itself, looks like a pretty good start. Comments inline.

Reply
Revision history for this message
Devid Antonio Filoni (d.filoni) wrote :

IMHO pulse connections and methods should be handled in a common (for all Ubuntu apps/services) C++ library, looks like we are using too many types of APIs (pulse itself, qmultimedia, gstreamer, dbus), so it's easy to introduce bugs/unwanted behavior between apps...

Thank you Charles! I'm returning to C++ after many years so I really appreciate your detailed review, I won't repeat these mistakes again ;)
I'll try and commit all your suggestions ASAP, I commented on few of them

Reply
Revision history for this message
Devid Antonio Filoni (d.filoni) :
Reply

Unmerged revisions

464. By Devid Antonio Filoni

Apply code changes suggested by charlesk

463. By Devid Antonio Filoni

Remove useless g_critical call set during sink port change debug

462. By Devid Antonio Filoni

Do not restore original sink port on alarm end if port is not available anymore or already active.

461. By Devid Antonio Filoni

Play alarms through speakers after 5 seconds when speakers are not active (LP: #1364647)

460. By Devid Antonio Filoni

Prefer speakers over headphones when playing alarm (LP: #1364647)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches