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

[Devid Antonio Filoni]

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

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

[Devid Antonio Filoni (d.filoni)]

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

[Jonas G. Drange (jonas-drange)]

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)]

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)]

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]

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)]

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]

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

Fix comment again...

[Jonas G. Drange (jonas-drange)]

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

[Jonas G. Drange (jonas-drange)]

Looks good, works well. Thank you!

review: Approve

[Alberto Mardegan (mardy)]

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

review: Needs Resubmitting

[Devid Antonio Filoni (d.filoni)]

> 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)]

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)

=== modified file 'cmd/account-polld/main.go'
--- cmd/account-polld/main.go	2016-08-02 14:34:52 +0000
+++ cmd/account-polld/main.go	2016-11-22 21:16:23 +0000
@@ -201,7 +201,7 @@
 				// Play sound and vibrate on first notif only.
 				if i > 0 {
 					n.Notification.Vibrate = false
-					n.Notification.Sound = ""
+					n.Notification.Sound = false
 				}
 
 				// We're overflowing, so no popups.

=== modified file 'plugins/plugins.go'
--- plugins/plugins.go	2016-02-08 19:21:31 +0000
+++ plugins/plugins.go	2016-11-22 21:16:23 +0000
@@ -67,7 +67,7 @@
 				Popup:     true,
 				Persist:   true,
 			},
-			Sound:   DefaultSound(),
+			Sound:   true,
 			Vibrate: true,
 			Tag:     cmdName,
 		},
@@ -104,9 +104,8 @@
 // Notification (optional) describes the user-facing notifications
 // triggered by this push message.
 type Notification struct {
-	// Sound (optional) is the path to a sound file which can or
-	// cannot be played depending on user preferences.
-	Sound string `json:"sound,omitempty"`
+	// Whether to play sound. Users can disable this so don’t rely on it exclusively.
+	Sound bool `json:"sound,omitempty"`
 	// Card represents a specific bubble to give to the user
 	Card *Card `json:"card,omitempty"`
 	// Vibrate is the haptic feedback part of a notification.
@@ -231,9 +230,3 @@
 
 	return nil
 }
-
-// DefaultSound returns the path to the default sound for a Notification
-func DefaultSound() string {
-	// path is searched within XDG_DATA_DIRS
-	return "sounds/ubuntu/notifications/Blip.ogg"
-}