811860 - [Sound] SMS Needs a sound when message is sent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

Description

[Patryk Adamczyk [:patryk] UX]

When message is sent, the user would expect some feedback, ideally in form of a sound.
Attached is a sound candidate, it may still require a bit of tweaking.

Comment 1

[Patryk Adamczyk [:patryk] UX]

Attachment: Sound

Comment 2

[David Scravaglieri [:scravag]]

Nice to have but polish for v1

Comment 3

[Deepa Subramanian]

duplicated - https://bugzilla.mozilla.org/show_bug.cgi?id=807918

Comment 4

[Borja Salguero [:borjasalguero]]

Patryk, Is this one the sound to be implemented? We should define the 'Target milestone' in order to prioritize better. Thanks!

Comment 5

[Patryk Adamczyk [:patryk] UX]

Yes this is the correct sound.

Comment 6

[David Scravaglieri [:scravag]]

We would take a patch for this issue.

Comment 7

[amiller]

UCID: set-131
https://moztrap.mozilla.org/results/case/62811/

Comment 8

[Kevin Grandon :kgrandon]

Kyle - I didn't see this bug, but recently implemented the same sound for email. We have an R+ for that, so I would assume the approach would be similar here. The email pull request is here: https://github.com/mozilla-b2g/gaia/pull/7639

I'm more than happy to take this one off your plate if you're busy - just assign to me if you want.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Kevin: Reassigned. I had just picked it up while looking for some low hanging fruit, but then got sidetracked. Thanks for picking it up! :)

Comment 10

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Hi Borja,

Wondering if you could give this a quick review. This is basically the same fix as we have implemented for email. Thanks!

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 707959 [details]
Github pull request pointer

Hi, just looking for a review on this one, thanks!

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 707959 [details]
Github pull request pointer

I guess that Borja can review better than me here :)

Comment 13

[Borja Salguero [:borjasalguero]]

I will take a look asap! ;)

Comment 14

[Borja Salguero [:borjasalguero]]

From my side R+. I've added Kaze because he is in charge of Settings & l10n part. Thanks!

Comment 15

[Kevin Grandon :kgrandon]

Thanks Borja! As Kaze is out, I've gone ahead and landed this for now and he can take a look at the strings when he gets back. (Was tired of dealing with merge conflicts, and we can always update the strings at a later date).

Commit: https://github.com/mozilla-b2g/gaia/commit/536f76901b990d6378214be6ad97b271a90b7cc0

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer - performance followup

Hi Borja - Wondering if you can give this a quick look-over so we can fix the performance regression I introduced. Thanks!

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 716959 [details]
Github pull request pointer - performance followup

Clearing this review flag as it was taken care of in bug 844205.

Comment 18

[Borja Salguero [:borjasalguero]]

https://github.com/KevinGrandon/gaia/commit/5d40e508df9fe61018a3e45749a1957fe886a1a1
https://github.com/KevinGrandon/gaia/commit/5d40e508df9fe61018a3e45749a1957fe886a1a1

Comment 19

[Borja Salguero [:borjasalguero]]

Comment on attachment 707959 [details]
Github pull request pointer

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): There was no sound for sent message
User impact if declined: No sound feedback when a message is sent.
Testing completed: 
Risk to taking this patch (and alternatives if risky): Low. Small change which changes the feedback to the user.
String or UUID changes made by this patch:

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 707959 [details]
Github pull request pointer

Low risk, approving for v1-train uplift.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

Commit 5d40e508df9fe61018a3e45749a1957fe886a1a1 does not apply to v1-train.  This means that there are merge conflicts which need to be resolved.  If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x  5d40e508df9fe61018a3e45749a1957fe886a1a1
  <RESOLVE MERGE CONFLICTS>

Comment 22

[Julien Wajsberg [:julienw]]

This will probably need manual merge because the split-file bug landed on v1-train already.

(don't really know how it managed to land without a conflict though :) )

Comment 23

[Julien Wajsberg [:julienw]]

Kevin, you'll want to merge Bug 844205 as well. :)

Comment 24

[Kevin Grandon :kgrandon]

This has been applied to v1-train as: bb081b789b3edc1cbdafdd1cfafb000c20585038