1031430 - (2.0-visual-refresh)[Notifications and Alarms] - Add New Signature Firefox OS tone

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Ringtones, defect)

Description

[Patryk Adamczyk [:patryk] UX]

Attachment: Archive.zip

Please add this signature Firefox OS ringtone and notification sound to the builds. Both of these sounds should have the name "Firefox" in the selection list.

Comment 1

[Jim Porter (:squib)]

So, since I can't add new localizations, where does the Firefox string come from? I assume there's branding somewhere.

Comment 2

[Stephany Wilkes]

We only discussed the existing user/upgrade case. Jim brought up a good point that we should add this new ringtone as a default for new profiles (new Firefox OS device).  Lets go with that for 2.0. Noting it here as a requirement.

Comment 3

[Stephany Wilkes]

Marking this feature-b2g because it is taking the place of bug #1014009, which is moving to feature-b2g=2.1 due to risk.

Comment 4

[Stephany Wilkes]

Confirming this is the string: branding.en-US.properties:brandShortName = Firefox OS

Comment 5

[Jim Porter (:squib)]

I'm not sure I like having both of these have the same name (it can be a bit confusing, since the default ringer for the device can actually be one of the notification tones and vice versa), but as long as we promise to think about better names for 2.1, I don't have a huge problem with this.

Comment 6

[Jim Porter (:squib)]

Attachment: Add the tones

Ok, here we go. David: sorry to request yet another review from you, but this should be pretty straightforward!

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8448170 [details] [review]
Add the tones

Do you want the sounds to be called "Firefox" per Patryk or "Firefox OS" per Stephany?  Check that before landing. One other nit and one possible bug noted on github.

Otherwise, though, this looks good to me. I have not tried actually running the patch myself.

Comment 8

[David Flanagan [:djf]]

Also: the filesize of the new ringer is really big.  

The existing .ogg files are 48000 samples per second and about 80000 bps.  This new file is 44000Hzm, but 128000bps.  It either has worse compression or is using a higher number of bits per sample. It sounds beautiful, but is it worth almost half a megabyte in the settings db?

Jim: did Patryk supply .wav files and leave the encoding to you?  Is there any way to get a smaller file and still have acceptable sound quality?  Its not like our users are going to be listening to the ringtone with headphones on.

Comment 9

[Jim Porter (:squib)]

Ok, I commented on the PR about your comments.

Patryk supplied .ogg files that I just dropped into the patch. I'm fine with changing the bitrate, but I'd need lossless files to do that. And maybe we'd want to use Opus for that anyway, since I seem to recall that Opus uses less memory (they may already be Opus; I didn't actually check).

Comment 10

[Stephany Wilkes]

Flagging Patryk for comment.

Comment 11

[Patryk Adamczyk [:patryk] UX]

Here is the uncompressed WAVs

Notify WAV
https://mozilla.box.com/s/ezybxcm980878mop7cd8

In the past I had Ben Kelly compress these into OPUS however was on PTO for a few weeks when initially we wanted these ringtones in prior to complications. Ideally we keep the higher BPS if possible, but compress them more efficiently with OPUS. The current ringtones are smaller, but they also suffer due to the lower BPS and we're not shipping solely on entry level devices with 2.0+

Comment 12

[Patryk Adamczyk [:patryk] UX]

Attachment: ringer_firefox.wav

Attached is a shorter WAV of the firefox ringer. The repetitions are removed from the sound file so that we can optimize the memory size, while keeping the quality.

Comment 13

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 8448170 [details] [review]
Add the tones

Overall the tones work correctly (when a new notification comes in or when a new call comes in). However I minused this patch as the labels in the settings for both the notifier and ringtones are "B2G OS" instead they should be "Firefox"

Comment 14

[Jim Porter (:squib)]

(In reply to Patryk Adamczyk [:patryk] UX from comment #13)
> Comment on attachment 8448170 [details] [review]
> Add the tones
> 
> Overall the tones work correctly (when a new notification comes in or when a
> new call comes in). However I minused this patch as the labels in the
> settings for both the notifier and ringtones are "B2G OS" instead they
> should be "Firefox"

It sounds like you built with the unofficial branding. You need to set MOZILLA_OFFICIAL=1 when flashing to get the official branding. Then you should see "Firefox OS". I know that's not "Firefox", but if I use the branding string that says "Firefox", then unofficially-branded devices will show that ringtone as "Web Browser", which is pretty weird.

I could make a new string for this, but that means we'd need late-l10n, and I thought the whole point here was to avoid doing that.

Comment 15

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 8448170 [details] [review]
Add the tones

Ok I ran it with Official, works as expected. Thanks. And yes we'll adjust the strings in v.2.1 with L10n.

Comment 16

[Patryk Adamczyk [:patryk] UX]

Can we push this in now?

Comment 17

[Jim Porter (:squib)]

(In reply to Patryk Adamczyk [:patryk] UX from comment #16)
> Can we push this in now?

I'm waiting until I see green test results so I don't inadvertently cause a regression. Hopefully the tests will finish up later this afternoon and I can merge.

Comment 18

[Jim Porter (:squib)]

Ok, the tests failed, so I fixed them. I also switched the audio files to be 64kbps mono Opus. They sound fine to me, even when using headphones.

Comment 19

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/6ced055111a6a96462fa3a1019c4240f5baa77f0

Comment 20

[Francesco Lodolo [:flod]]

I tried to read the bug, but I'm not sure what the plan is, especially after reading comment 14. So, feel free to discard this comment if I'm making the wrong assumptions.

This patch can't land on 2.0, since it adds 2 new strings. It doesn't matter they're just using a variable which is not even localizable on official branding, it's still +2 strings. If you need this on 2.0, you'll need an approach like bug 990537.

Also, if you landed this with the plan to change the string on master later, keep in mind that you'll need new string IDs.

Comment 21

[Jim Porter (:squib)]

(In reply to Francesco Lodolo [:flod] from comment #20)
> Also, if you landed this with the plan to change the string on master later,
> keep in mind that you'll need new string IDs.

Sadly, this isn't possible. The L10N ID is stored in mozSettings, and we need the ID to always map to a translation. There's some talk about resetting users' settings here, which would resolve this issue, but otherwise, we can't do as you suggest.

Comment 22

[Jim Porter (:squib)]

Stephany: Do you have any thoughts regarding comment 20?

Background: We need L10N IDs for our ringtones to be stable across versions, since those IDs are stored in the mozSettings database. While it's theoretically possible to migrate these during an upgrade, it's not the easiest process: every single place that uses the existing setting would need to add some migration logic. I'd rather not do this, since weird stuff could happen if we miss something.

(As an aside, I was under the impression that our L10N tools had finally been improved to the point that we could change a string without changing the ID. Is this not true?)

While we can do the hack suggested in comment 20 (though I'd much rather not, since it's adding complexity to something that's already landing pretty late), we still need a way forward so that we can provide better localizations in the future.

If we've decided that we're going to reset all users' ringtones to the factory-default in 2.1, we can just rely on that to clear out the bad old l10n ID. Let me know if that's the plan, since that will change how we backport this fix.

Comment 23

[Stephany Wilkes]

I have no idea. We need to ask Axel.

Comment 24

[Francesco Lodolo [:flod]]

Wrong mail, :pike is already CCed to the bug.

"Our l10n tools" is not really a concept in real life, since each localizer uses whatever he wants, from a text editor+hg to Transifex, Pootle and other web tools.

I'm far from being a developer, but I don't see why you can't have a mapping between "ringtone name" and "ringtone file", and store that mapping. I don't mean to sound disrespectful, but I've heard the complaint "I can't change string IDs" several times in the past, and it's always been a matter of can't vs don't want.

Also, about the general process: why not asking before landing changes? Don't make assumptions on the l10n system or tools.

Comment 25

[Jim Porter (:squib)]

(In reply to Francesco Lodolo [:flod] from comment #24)
> I'm far from being a developer, but I don't see why you can't have a mapping
> between "ringtone name" and "ringtone file", and store that mapping. I don't
> mean to sound disrespectful, but I've heard the complaint "I can't change
> string IDs" several times in the past, and it's always been a matter of
> can't vs don't want.

As mentioned, we store the l10n ID of the default ringtone (and alert tone) in mozSettings so that we can change the displayed name whenever the user changes their system language. This relies on the l10n ID not going away when we upgrade the device. It's technically possible to change l10n IDs, but the fact that it's necessary indicates that there is a problem with how we manage localizations on some level (the blame doesn't necessarily lie on l10n's side).

In general, the reason it's always been a matter of can't vs don't want is because it's always possible to add an extra layer of indirection to the code, mapping stable IDs to potentially-changing ones. However, all this does is offload the complexity of changing strings onto each app developer because there's no existing general-purpose solution for it. Localization is something every app has to do, and it's pretty common to want to change the contents of a localized string when working on a patch. Rather than leaving this as a sharp corner for developers to cut themselves on, we should design our systems to prevent these problems from appearing in the first place. One way to do that would be to make whatever changes are necessary to let developers reuse l10n IDs.

> Also, about the general process: why not asking before landing changes?
> Don't make assumptions on the l10n system or tools.

According to the email thread about this, we'd already discussed things with Axel and settled upon this as a plan. Unfortunately, I wasn't there for that discussion, so I don't know what people said.

Comment 26

[Francesco Lodolo [:flod]]

It's been a while, but this comes out cyclically (last I remember was bug 849633).

(In reply to Jim Porter (:squib) from comment #25)
> However, all this does is offload the complexity of changing strings onto each app developer
> because there's no existing general-purpose solution for it. 

Right, let's offload this to an unknown number of tools on which we don't have any kind of control.

> Localization is something every app has to do, and it's pretty common to want to change the
> contents of a localized string when working on a patch. 

In fact we've been localizing Firefox OS for over a year, it worked fine so far.

> According to the email thread about this, we'd already discussed things with
> Axel and settled upon this as a plan. 

I wasn't part of the discussion, so I can't talk about that.

Comment 27

[Axel Hecht [:Pike]]

This bug wasn't part of any email conversation with me. There was a discussion about bug 1014009 and that not being 2.0 material, that was basically the full scope of the conversation.

The idea that the same ring tone would suddenly show up under a different name sounds like bad UX to me, btw.

As flod raised, the patch as is isn't 2.0 material.

I don't think that assumptions on toolchains is a good topic for random bug discussions, if you want to have a conversation on that, I suggest you open a conversation in .gaia.

Comment 28

[Jim Porter (:squib)]

(In reply to Francesco Lodolo [:flod] from comment #26)
> It's been a while, but this comes out cyclically (last I remember was bug
> 849633).
> 
> (In reply to Jim Porter (:squib) from comment #25)
> > However, all this does is offload the complexity of changing strings onto each app developer
> > because there's no existing general-purpose solution for it. 
> 
> Right, let's offload this to an unknown number of tools on which we don't
> have any kind of control.

That doesn't follow from what I said at all. I've posted a longer discussion about this to dev-gaia, but as mentioned, the problem here isn't necessarily l10n's responsibility. One option would be to add versioning info to our translations; this would *look* like an ID change to a localizer, but developers could keep using the old IDs in their code.

Comment 29

[Hema Koka [:hema]]

> While we can do the hack suggested in comment 20 (though I'd much rather
> not, since it's adding complexity to something that's already landing pretty
> late), we still need a way forward so that we can provide better
> localizations in the future.
> 
> If we've decided that we're going to reset all users' ringtones to the
> factory-default in 2.1, we can just rely on that to clear out the bad old
> l10n ID. Let me know if that's the plan, since that will change how we
> backport this fix.

Patryk/Stephany/Sri

To go with the workaround suggested, we need to know if the users' ringtones are going to be reset to default for 2.1 (when we delete old ringtones and add a bunch of new ones in the next release). Please confirm in order to proceed with this hack.

All,

We are trying to do what we can to help get at least one 2.0 ringtone in with least impact on localization and risk to the release. Apologies on behalf of the team that this work has come in late. 

Thanks
Hema

Comment 30

[Stephany Wilkes]

Thank you, Hema. I just spoke with Patryk. We've decided that we can reset all users' ringtones to the factory-default in 2.1, so we can (as sugggested) rely on that to clear out the bad old l10n ID.

Comment 31

[Jim Porter (:squib)]

Attachment: 2.0 version

The build doesn't seem to be working at the moment, but I think this is the right thing to do...

Comment 32

[Jim Porter (:squib)]

Comment on attachment 8453513 [details] [review]
2.0 version

Ok, this should work for 2.0.

Dave: This is the exact same patch as what landed for 2.1, except that I moved the strings to a different file that will (hopefully) not be picked up by the localization dashboard.

Comment 33

[Francesco Lodolo [:flod]]

Comment on attachment 8453513 [details] [review]
2.0 version

Same approach of bug 990537, makes sense, thanks.

Comment 34

[Jim Porter (:squib)]

Comment on attachment 8453513 [details] [review]
2.0 version

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 #): N/A
[User impact] if declined: New users won't have the new "Firefox" ringtone/notification sounds
[Testing completed]: Backed by integration/build tests
[Risk to taking this patch] (and alternatives if risky): Low-risk
[String changes made]: Added some strings, but they just forward on to brandShortName, so no localization is needed

Comment 35

[Axel Hecht [:Pike]]

(In reply to Jim Porter (:squib) from comment #34)
> [String changes made]: Added some strings, but they just forward on to
> brandShortName, so no localization is needed

To be extra-precise here, we're exposing brand names in the Interface, they're not new localizable strings in this patch, though. This patch is using the non-localized-.properties file trick we used in other bugs.

Just making this explicit 'cause the bugmail made me freak out for a second.

Comment 36

[bhavana bajaj [:bajaj]]

Flagging this as verifyme and NI marcia, to help verify this on master first.

Comment 37

[Marcia Knous [:marcia]]

Verified fixed on master. Tested on Flame Device using:

Gaia   c47094a26c87ba71a3da4bae54febd0da21f3393
SourceStamp 1b1296d00330
BuildID 20140711040202
Version 33.0a1
Base image: 122

Verified that tone works for an inbound phone call as well as an inbound text message.

Comment 38

[Stephany Wilkes]

Please see comment #4 as to the expected ringtone name. Since I last flashed my device, it has gone from "Firefox OS" to "B2G OS" which is not acceptable. "B2G OS" has landed in master but is not, and should not be, uplifted with that name. Thanks!

Comment 39

[Stephany Wilkes]

The patch also did not get ui-review+ when the name was "B2G OS," so the patch that Patryk gave a + to should probably go back to ui-review- and be resubmitted.

Comment 40

[Jim Porter (:squib)]

(In reply to Stephany Wilkes from comment #38)
> Please see comment #4 as to the expected ringtone name. Since I last flashed
> my device, it has gone from "Firefox OS" to "B2G OS" which is not
> acceptable. "B2G OS" has landed in master but is not, and should not be,
> uplifted with that name. Thanks!

It's only "B2G OS" if you haven't flashed with official branding. Flash with MOZILLA_OFFICIAL=1 to use the "Firefox" branding. (We shouldn't be saying "Firefox" anywhere in an unofficially-branded build, since that's a trademark issue.)

Comment 41

[Stephany Wilkes]

That's good enough for me, as long as it doesn't ship this way. Thanks!

Comment 42

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/2f882a831d678826ea2ccb071ec38be2b1c12d77

Comment 43

[Shally Li（leave from Mozilla）]

Hi Reporter,

    Could you help to confirm whether the steps are correct?

Thank you.
 
This bug has been successfully verified on Flame v2.0 by the following steps.
See attachment: verified_v2.0.png.
Reproduce rate: 0/5.

STR:
1.Go to Settings/Sound.
2.Verify the Firefox ringtone.
**It displays "Firefox OS" in ringtones list.

Flame 2.0 build:
Gaia-Rev        2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5bf8087572c3
Build-ID        20150128000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150128.033208
FW-Date         Wed Jan 28 03:32:20 EST 2015
Bootloader      L1TC000118D0

Comment 44

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.0.png

Comment 45

[Patryk Adamczyk [:patryk] UX]

Thanks looks correct to me!

Comment 46

[Shally Li（leave from Mozilla）]

(In reply to Patryk Adamczyk [:patryk] UX from comment #45)
> Thanks looks correct to me!

Thanks

Per Comment 43 & Comment 45,modify status.