Merge lp:~d.filoni/account-polld/lp1481202 into lp:~ubuntu-push-hackers/account-polld/trunk

Proposed by Devid Antonio Filoni on 2016-09-01
Status: Rejected
Rejected by: Alberto Mardegan on 2016-12-02
Proposed branch: lp:~d.filoni/account-polld/lp1481202
Merge into: lp:~ubuntu-push-hackers/account-polld/trunk
Diff against target: 47 lines (+4/-11)
2 files modified
cmd/account-polld/main.go (+1/-1)
plugins/plugins.go (+3/-10)
To merge this branch: bzr merge lp:~d.filoni/account-polld/lp1481202
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) Resubmit on 2016-12-01
Jonas G. Drange (community) 2016-09-01 Approve on 2016-11-23
Review via email: mp+304710@code.launchpad.net

Commit message

Use bool sound value to patch and fix bug #1481202 properly

Description of the change

Hi,
this branch introduces a dbus connection in order to retrieve IncomingMessageSound propery from com.ubuntu.touch.AccountsService.Sound interface. If the set sound file is not in /usr/share or a dbus error occour then Blip.ogg is used as fallback.
This was tested on mako (rc channel)
Please review this code carefully, I'm not a golang dev

To post a comment you must log in.
Devid Antonio Filoni (d.filoni) wrote :

Hi,
is there any news about this MR? I don't know who to ping ;)
Thank you!
Devid

Jonas G. Drange (jonas-drange) wrote :

Sorry for the lag, Devid. Thanks for the branch.

I think the execution is great, but I worry about duplicating code that already exist in ubuntu-push. If you look in ubuntu-push/bus/notifications/raw.go, this Presenter is the one dispatching the Unity Notification to Unity8 (which is ultimately responsible for producing a bubble and a sound).

Further, ubuntu-push is already informed about settings in AccountsService. Maybe ubuntu-push should make the decision what sound to play?

So I think the real issue is that account-polld is providing a sound file at all.

What do you think?

review: Needs Information
Bill Filler (bfiller) wrote :

I thought there was an existing way for the app to supply the sound file as
part of the click package and specify that in the manifest? Adding Arthur
here as I believe Telegram app is already doing this..

On Tue, Nov 22, 2016 at 7:49 AM, Jonas G. Drange <<email address hidden>
> wrote:

> Review: Needs Information
>
> Sorry for the lag, Devid. Thanks for the branch.
>
> I think the execution is great, but I worry about duplicating code that
> already exist in ubuntu-push. If you look in ubuntu-push/bus/notifications/raw.go,
> this Presenter is the one dispatching the Unity Notification to Unity8
> (which is ultimately responsible for producing a bubble and a sound).
>
> Further, ubuntu-push is already informed about settings in
> AccountsService. Maybe ubuntu-push should make the decision what sound to
> play?
>
> So I think the real issue is that account-polld is providing a sound file
> at all.
>
> What do you think?
> --
> https://code.launchpad.net/~d.filoni/account-polld/lp1481202/+merge/304710
> Your team Ubuntu Push Hackers is subscribed to branch
> lp:~ubuntu-push-hackers/account-polld/trunk.
>

Bill Filler (bfiller) wrote :

Sorry I misunderstood the issue but just re-read the bug.
The UI is very confusing. Changing the Sounds->Messages->Message received sound is really only specific to the Messaging App and isn't used by other apps (I believe).
Apps that support ubuntu-push/account-polld are shown in the Settings->Notifications panel and currently they either use a) a default system wide sound or b) they supply a custom sound in the click package/manifest. Eventually this panel would allow selection of a custom sound per app but we haven't got there and the Messaging App would move to the Notifiations panel so it's consistent.

Sounds like what you are asking for is to have apps that show up in Notifications panel use the sound selected in Sounds->Messages->Message received as their default sound instead of hardcoded default they currently use? This sounds like a reasonable approach to use that value for the default if the click doesn't provide it's own.

lp:~d.filoni/account-polld/lp1481202 updated on 2016-11-22
179. By Devid Antonio Filoni on 2016-11-22

Revert default notification sound patch and fix bug #1481202 properly

180. By Devid Antonio Filoni on 2016-11-22

Fix comment

Devid Antonio Filoni (d.filoni) wrote :

Hi,
@Jonas: I agree, I didn't know about ubuntu-push implementation, the bug was a bytesize one assigned to account-polld and I haven't looked into other projects...
I committed a new smaller fix, thank you for pointing me to proper (and easier) solution ;)
@Bill: I think the approach you described is exactly what user asks for (and what my branch fixes)

I tested the new patch on mako

lp:~d.filoni/account-polld/lp1481202 updated on 2016-11-22
181. By Devid Antonio Filoni on 2016-11-22

Fix comment again...

Jonas G. Drange (jonas-drange) wrote :

Thanks for addressing that, Devid. Will try it out tomorrow!

Jonas G. Drange (jonas-drange) wrote :

Looks good, works well. Thank you!

review: Approve
Alberto Mardegan (mardy) wrote :

The project code was moved to the ~online-accounts team, so the target branch is wrong: you should simply pick lp:account-polld.

review: Resubmit
Devid Antonio Filoni (d.filoni) wrote :

> The project code was moved to the ~online-accounts team, so the target branch
> is wrong: you should simply pick lp:account-polld.

Hi Alberto,
no problem, I'll resubmit the branch today once I'm back at home ;)
Thank you,
Devid

Devid Antonio Filoni (d.filoni) wrote :

I created ~d.filoni/account-polld/lp1481202_rebased branch.

MR: https://code.launchpad.net/~d.filoni/account-polld/lp1481202_rebased/+merge/312282

Unmerged revisions

181. By Devid Antonio Filoni on 2016-11-22

Fix comment again...

180. By Devid Antonio Filoni on 2016-11-22

Fix comment

179. By Devid Antonio Filoni on 2016-11-22

Revert default notification sound patch and fix bug #1481202 properly

178. By Devid Antonio Filoni on 2016-09-03

Properly check if notification sound is in xdg data dirs

177. By Devid Antonio Filoni on 2016-09-01

Connect to dbus to get notification sound (LP: #1481202)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/account-polld/main.go'
2--- cmd/account-polld/main.go 2016-08-02 14:34:52 +0000
3+++ cmd/account-polld/main.go 2016-11-22 21:16:23 +0000
4@@ -201,7 +201,7 @@
5 // Play sound and vibrate on first notif only.
6 if i > 0 {
7 n.Notification.Vibrate = false
8- n.Notification.Sound = ""
9+ n.Notification.Sound = false
10 }
11
12 // We're overflowing, so no popups.
13
14=== modified file 'plugins/plugins.go'
15--- plugins/plugins.go 2016-02-08 19:21:31 +0000
16+++ plugins/plugins.go 2016-11-22 21:16:23 +0000
17@@ -67,7 +67,7 @@
18 Popup: true,
19 Persist: true,
20 },
21- Sound: DefaultSound(),
22+ Sound: true,
23 Vibrate: true,
24 Tag: cmdName,
25 },
26@@ -104,9 +104,8 @@
27 // Notification (optional) describes the user-facing notifications
28 // triggered by this push message.
29 type Notification struct {
30- // Sound (optional) is the path to a sound file which can or
31- // cannot be played depending on user preferences.
32- Sound string `json:"sound,omitempty"`
33+ // Whether to play sound. Users can disable this so don’t rely on it exclusively.
34+ Sound bool `json:"sound,omitempty"`
35 // Card represents a specific bubble to give to the user
36 Card *Card `json:"card,omitempty"`
37 // Vibrate is the haptic feedback part of a notification.
38@@ -231,9 +230,3 @@
39
40 return nil
41 }
42-
43-// DefaultSound returns the path to the default sound for a Notification
44-func DefaultSound() string {
45- // path is searched within XDG_DATA_DIRS
46- return "sounds/ubuntu/notifications/Blip.ogg"
47-}

Subscribers

People subscribed via source and target branches