Closed Bug 1014009 (Ringtones-2.0) Opened 6 years ago Closed 6 years ago

(2.0-visual-refresh)[Ringtones] - Add New Tones

Categories

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

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.1)

VERIFIED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: padamczyk, Assigned: pivanov)

References

Details

(Whiteboard: interaction-design [2.1-feature-qa+])

Attachments

(2 obsolete files)

For the visual design refresh we have created a new set of ringtones to better reflect the visual style. These cover:
+ ringtones
+ alarms
+ message notifications

1. The WAV files are here: https://mozilla.box.com/s/6x15jupkdv98qbrrsgpy they need to be compressed. 
2. Current tones need to be replaced within the OS.
Pavel can you add the tones in this link to the builds: https://mozilla.box.com/s/mnekb49udsazzjpxj3f5
1. Remove all ringtones and replacement with what is the "Ringtones" folder.
2. Make "ringer_firefox" the default
3. Remove all alarms and replacement with what is the "Alarms" folder.
4. Make "ac_awake" the default
5. Add all the new notification tones, keep the older ones
Alias: Sound
Assignee: nobody → pivanov
Group: mozilla-employee-confidential
Flags: needinfo?(pivanov)
Attached file patch for Gaia/master (obsolete) —
Hey Patryk,
can you check this one to be sure that everything is OK
Attachment #8435770 - Flags: feedback?(padamczyk)
Flags: needinfo?(pivanov)
I get this error: make: *** [operatorvariant] Error 3
when I try to "make reset-gaia"
Flags: needinfo?(pivanov)
Did you try to unlock the device and then flash it?
Flags: needinfo?(pivanov)
Attachment #8435770 - Flags: feedback?(padamczyk)
Pavel, can you have a look at this? Maybe we need to split this in 3 patches. 
Ringtones is #1 priority.
Lets make 2 separate bugs for notifications and alarms.
Flags: needinfo?(pivanov)
Flags: needinfo?(pivanov)
Summary: (2.0-visual-refresh) Add New Tones → (2.0-visual-refresh)[Ringtones] - Add New Tones
No longer blocks: 1023133, 1023134
Sure :)
PR for Ringtones is updated now. Also we have Bug 1023133 for Notifications and Bug 1023134 for Alarms
Attachment #8435770 - Flags: feedback?(padamczyk)
This still gives me:

make: *** [operatorvariant] Error 3
when I try to "make reset-gaia"

This happens right at the end and messes up my phone so that I have to reflash it with a fresh build.
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Flagging Tif on ui-review? for the IxD side and anything else she feels like mentioning. :)
Attachment #8435770 - Flags: ui-review?(tshakespeare)
This needs new strings before anything here is going to work. Look in shared/locales/sounds/
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

As Jim mentioned, the patch is resulting in empty lists. LMK when it's working and  I'll ui-review. Thanks guys!
Attached image Shot from Device (obsolete) —
Hey Tiffanie,
based on Jim's comment I made some changes and I think it works now but please check it again because I'm not familiar with this part of Gaia

Thanks :)
Attachment #8438313 - Flags: ui-review?(tshakespeare)
Still get the same error. :(
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Ringtones portion of it looks good. Text is now showing up in both the manage list and the ringer selection list.

One question I have is on the previous screen, Sound, in the Ringer button we show the name of the current selection. This works in master. But in this patch, I notice that it's reverted to the "Change". Will that get resolved when this patch is merged?
Attachment #8435770 - Flags: ui-review?(tshakespeare) → ui-review+
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(pivanov)
Whiteboard: interaction-design
The patch still doesn't change everything that needs to be changed, unfortunately, which is causing the issue in comment 13. If you grep the source tree for "ringer_", you should find all the necessary places. For completeness, here they are from my tree:

build/test/unit/settings.test.js
111:        'ringer_classic_courier.opus';
116:                       {l10nID: 'ringer_classic_courier'});
118:                   'builtin:ringer_classic_courier');
120:                   'builtin:ringer_classic_courier');
275:          'dialer.ringtone.name': {l10nID: 'ringer_classic_courier'},
276:          'dialer.ringtone.id': 'builtin:ringer_classic_courier',
277:          'dialer.ringtone.default.id': 'builtin:ringer_classic_courier',
308:          'dialer.ringtone.name': {l10nID: 'ringer_classic_courier'},
309:          'dialer.ringtone.id': 'builtin:ringer_classic_courier',
310:          'dialer.ringtone.default.id': 'builtin:ringer_classic_courier',
339:          'dialer.ringtone.name': {l10nID: 'ringer_classic_courier'},
340:          'dialer.ringtone.id': 'builtin:ringer_classic_courier',
341:          'dialer.ringtone.default.id': 'builtin:ringer_classic_courier',
373:          'dialer.ringtone.name': {l10nID: 'ringer_classic_courier'},
374:          'dialer.ringtone.id': 'builtin:ringer_classic_courier',
375:          'dialer.ringtone.default.id': 'builtin:ringer_classic_courier',

build/settings.js
65:  // Grab ringer_classic_courier.opus and convert it into a base64 string
67:  let ringtone_name = 'ringer_classic_courier.opus';

apps/ringtones/test/unit/system_tones_test.js
106:        defaultIDs.ringtone = 'builtin:ringer_classic_courier';

apps/settings/js/sound.js
140:          'ringer_classic_courier.opus';

shared/locales/sounds/sounds.en-US.properties
3:ringer_classic_prism = Classic Prism
4:ringer_classic_courier = Classic Courier
5:ringer_classic_touchmatic = Classic Touchmatic
6:ringer_classic_electric = Classic Electric
7:ringer_classic_wallphone = Classic Wallphone
8:ringer_disco_drive = Disco Drive
9:ringer_vamos_la_elektro = Vamos Là Elektro
10:ringer_digitalascent = Analog Ascent
11:ringer_low_bit_swing = Low Bit Swing
12:ringer_ringing_gems = Ringing Gems
13:ringer_rain_echoes = Rain Echoes
14:ringer_progressive_dapple = Progressive Dapple
15:ringer_digital_dapple = Analog Dapple
16:ringer_loud_windchimes = Wind Chimes
17:ringer_bitbounce = Bit Bounce

Sorry for not being clearer about this earlier.
Flags: needinfo?(squibblyflabbetydoo)
Hey Jim,
thanks for the help I update the patch based on you comments ... just have one question ... I need to look only on build/ dir right? because I find that we also have build_stage/? Because I need to change the default Alarm tone in other bug and I think I need to follow the same logic as here

Big Thanks :)
Flags: needinfo?(pivanov)
I'm pretty sure build_stage is just where we throw files that are generated *during* the build, so you don't need to touch them (I could be wrong though).
Depends on: 1026698
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Pavel, finally got this patch working thanks to Tif. Looks like a good start, the ringtones seem to be in okay, however we still need to add the alarms and assign them correctly.
Attachment #8435770 - Flags: feedback?(padamczyk) → feedback-
Depends on: 1023133
Alias: Sound → Ringtones
Changing the alias again because the current one makes it impossible for me to quicksearch for ringtones bugs. :)
Alias: Ringtones → Ringtones-2.0
Attachment #8435770 - Flags: feedback- → feedback?(padamczyk)
Attachment #8435770 - Flags: review?(clee)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Looks good, push them in!
Attachment #8435770 - Flags: review?(clee)
Attachment #8435770 - Flags: feedback?(padamczyk)
Attachment #8435770 - Flags: feedback+
Attachment #8435770 - Flags: review?(dflanagan)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Passing review to Jim
Attachment #8435770 - Flags: review?(dflanagan) → review?(squibblyflabbetydoo)
Patryk,

Why is this bug marked confidential?
Flags: needinfo?(padamczyk)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Like in the other bug, we're going to have to decide what to do here about migration of the default ringer. Should we just reset everyone to use one of the new ringtones?
Attachment #8435770 - Flags: review?(squibblyflabbetydoo) → review-
Unmarked confidential, looks like I cloned it from a confidential bug.
Group: mozilla-employee-confidential
Flags: needinfo?(padamczyk)
Hey Patryk,
what do you think about Jim's comment?
Flags: needinfo?(padamczyk)
Since this is a V2 product, I think its perfectly fine to reset people's tones. We will provide a link to download the v.1 tones, but the v.2 tones are so much better, I believe most people won't mind.
Flags: needinfo?(padamczyk)
Hey Jim,
based on Patryk's comment ... do we need to do something?
Flags: needinfo?(squibblyflabbetydoo)
Yes. We'll need to add a migration step that sets the ringtone to the new default upon upgrade to 2.0. I'm not sure how to do this, though.
Flags: needinfo?(squibblyflabbetydoo)
huh ... me neither ... do you know who can help with this?
Flags: needinfo?(squibblyflabbetydoo)
Yuren: you know about the build system, right? Do you know what the best way would be to change all users' ringtones to a new default when they upgrade to v2.0?
Flags: needinfo?(squibblyflabbetydoo) → needinfo?(yurenju.mozilla)
if you mean OTA, it's not a part of build system. for build system you can just replace the old one. schien may know about OTA.
Flags: needinfo?(schien)
Flags: needinfo?(yurenju.mozilla)
Yeah, OTA is probably what we want. Thanks!
For settings stored in indexedDB, application developers need to explicitly write code for data migration by detecting the indexedDB version number. However, rington setting is stored via Settings API which has no version number. @arthur, can you help answer the question in comment #29?
Flags: needinfo?(schien) → needinfo?(arthur.chen)
Currently there is no version number for settings DB as suggested in comment 32. Regarding the case of ringtone setting, what I can think of is to add code that maintains the version number of ringtone setting in system app. By doing this we could have a chance to update default ringtones at the first restart after OTA. In other cases of settings migration (ex: voicemail) we use similar ways.
Flags: needinfo?(arthur.chen)
Hey Arthur,
do you know who can help us with this?
Flags: needinfo?(arthur.chen)
Depends on: 1029096
This is not a blocker. When a patch is ready, please ensure it gets approval?/+ so that it can be uplifted for 2.0 when ready. Thanks!
Add Dominic in as he may know things about OTA. 

Dominic, it seems we could just reset the default ringtones per comment 25.
Flags: needinfo?(arthur.chen) → needinfo?(dkuo)
(In reply to Arthur Chen [:arthurcc] from comment #36)
> Add Dominic in as he may know things about OTA. 
> 
> Dominic, it seems we could just reset the default ringtones per comment 25.

Although Settings API does not provide version number, but we could use some other schemes to manage the ringtone's versions from 2.0 in the ringtone app, maybe use asyncStorage to keep the version number if we will have further ringtones update in 3.0, 4.0...

The migration part could be an separated issue and we can fix it as a followup, because I feel there are more details we have to consider, such as Patryk's suggestion in comment 25 and it should related to the implementation, then this one should be clean for visual update and at least we are sure the 2.0 branch works on the new devices that are not OTA from old versions. make sense?
Flags: needinfo?(dkuo)
Comment on attachment 8438313 [details]
Shot from Device

clearing ui-review flag - I ui-reivew +'d the patch.
Attachment #8438313 - Flags: ui-review?(tshakespeare)
(In reply to Dominic Kuo [:dkuo] from comment #37)
> (In reply to Arthur Chen [:arthurcc] from comment #36)
> > Add Dominic in as he may know things about OTA. 
> > 
> > Dominic, it seems we could just reset the default ringtones per comment 25.
> 
> Although Settings API does not provide version number, but we could use some
> other schemes to manage the ringtone's versions from 2.0 in the ringtone
> app, maybe use asyncStorage to keep the version number if we will have
> further ringtones update in 3.0, 4.0...

The main thing I want is the ability to run some sort of migration step immediately when OTA happens. The build system should handle things for new users, but for existing users, we need to switch to the new ringtones immediately so that any incoming calls will use the new sounds. I'm just not sure how to do that, if it's even possible.

I don't think storing version info in the ringtones app will help, since we should be running this upgrade process even if the user never opens the ringtones app.
(In reply to Jim Porter (:squib) from comment #39)
> The main thing I want is the ability to run some sort of migration step
> immediately when OTA happens. The build system should handle things for new
> users, but for existing users, we need to switch to the new ringtones
> immediately so that any incoming calls will use the new sounds. I'm just not
> sure how to do that, if it's even possible.
>
> I don't think storing version info in the ringtones app will help, since we
> should be running this upgrade process even if the user never opens the
> ringtones app.

This sounds like something we should do in runtime before the whole system boot up, if we want to change the system ringtone from the current one to an new one without entering the ringtone app once. I don't know if there is already a mechanism to handle this in gaia, but if not, we should build it then put all the migration things together, and execute them while the system is booting so that the user is able to get the new settings/features after OTA, like the new 2.0 ringtones.

Or this is something that gecko already have?
Dominic: Yeah, that sounds right. Who should we ask to figure out how to do this?
If we want to implement this in gaia, then I believe it should live in system, I will go to Tim or Alive and see if we can do something for this.
Based on Dominic's comment #42 NI'ing Alive (who appears to be OOO) and Tim.
Flags: needinfo?(timdream)
Flags: needinfo?(alive)
This should have been flagged as 2.0 so I am adding it. There are a lot of other 2.0 tags and flags here but not that one. This was obviously in the Media backlog and being worked on throughout 2.0.
feature-b2g: --- → 2.0
Depends on: 1031430
Swapping existing ringtones with new ones and dealing with migration is way too late for 2.0. We are well past the feature landing date (June 9th) and furthermore, new ringtones will need new labels which will impact L10n as well. We are in string freeze at the moment. 

Based on a quick meeting with Stephany, Tif, Patryk, David and I on 6/27: here is what we plan to do

1. Open a new bug just to add one new Ringtone with Firefox label (no impact to localization since we are using Firefox as the label for this ringtone) --> this bug will be targeted for 2.0 (Patryck to open bug and assign to Jim)

2. Keep this bug for tracking migration work and swaping old ringtones with new ones in the next release 2.1. UX Team: Please keep Tim Chien's team also in the loop for this work.

Thanks
Hema
No longer depends on: 1031430
Flags: needinfo?(swilkes)
Flags: needinfo?(padamczyk)
Per Hema and our chat this morning, noting this is now for 2.1 migration work.

See the new bug Hema mentions above for a single new ringtone in bug #1031430.
feature-b2g: 2.0 → 2.1
Depends on: 1031430
Flags: needinfo?(swilkes)
Hey everyone, there *is* a version number for the whole mozSettings store! Let me explain that in detail, if you haven't read Settings.jsm yourself :)

(I remember asking Gregor to document that somewhere -- but apparently no one need documentation when you have code, <sarcastic>right?</sarcastic>)

(In reply to Tiffanie Shakespeare from comment #43)
> Based on Dominic's comment #42 NI'ing Alive (who appears to be OOO) and Tim.

To recap what I said offline:

User ringtone is currently saved as a blob in mozSettings store under the key "media.ringtone". If there is no user-set ringtone in the user profile, the store will return the default ringtone set on build time, which is swapped during OTA, and the user will hear the new default ringtone. If that's what's needed we are all set, no migration code need to be written -- we should simply swap the media file in the tree and bump the version number for mozSettings store (I am pretty sure we have already done that some point after 1.4). Bumping that number ensures the new default from b2g/defaults/settings.json gets pickup by Gecko.

(In reply to Patryk Adamczyk [:patryk] UX from comment #25)
> Since this is a V2 product, I think its perfectly fine to reset people's
> tones. We will provide a link to download the v.1 tones, but the v.2 tones
> are so much better, I believe most people won't mind.

However, it look like we want to overwrite user-set ringtones during upgrade. I personally don't like the idea because a user-set ringtone might be come from anywhere (e.g. Music app), not just one of the built-in ringtones. Overwrite that might cause the user to lost that data forever, and fail to pick up the first phone call after upgrade. (Flagging :patryk to ensure he realizes the consequence.)

That said, if we really want to do that, we might as well as switch the key for ringtones to "media.ringtone2". An OTA'd user will get a ringtone just like anyone else who get a new phone.

Additionally we may delete the old blob to recover a bit of space if we want to. That would be the only "migration" we need.
Flags: needinfo?(timdream)
Flags: needinfo?(alive)
Thanks Tim for the explanation, and sounds to have a new settings key for the ringtone is an approach we could solve the migration issue, I will file a new bug then let's move these discussions and continue on that bug.

Pavel, would you please update your patch with the test fixed and ask review from Jim again? and after we landed this I think the followup could cover the migration part, thanks.
Flags: needinfo?(pivanov)
Blocks: 1032675
Attachment #8435770 - Flags: review- → review?(squibblyflabbetydoo)
Flags: needinfo?(pivanov)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #47)
> However, it look like we want to overwrite user-set ringtones during
> upgrade. I personally don't like the idea because a user-set ringtone might
> be come from anywhere (e.g. Music app), not just one of the built-in
> ringtones. Overwrite that might cause the user to lost that data forever,
> and fail to pick up the first phone call after upgrade. (Flagging :patryk to
> ensure he realizes the consequence.)

Just to note: this only applies to custom ringtone from the music app that was created in version 1.4. In version 2.0, we store copies of *all* custom-created ringtones, even ones that aren't currently set as the default ringer. For custom ringtones created in 2.0, we won't have any data loss.
Flags: needinfo?(padamczyk)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Most of the stuff for changing the default ringer has moved to bug 1031430, so this PR won't apply to master once that lands (which should be later today).

I think we also probably want all the sounds to be encoded as Opus files. I encoded ringer_firefox.opus with a bitrate of 64kbps, so I think we should do that here as well.
Attachment #8435770 - Flags: review?(squibblyflabbetydoo) → review-
Hey Jim - would it help at all if you got the raw file of the Firefox ringtone from Patryk for this release?
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(padamczyk)
Patryk already posted the raw .wav files and I encoded them into Opus.
Flags: needinfo?(squibblyflabbetydoo)
Whoops, wrong bug. Yes, that would help.
Hey Jim,
Does that mean this bug is obsolete or we can mark it as duplicate?
Flags: needinfo?(squibblyflabbetydoo)
No, we need to add all the *other* ringtones (i.e. not ringer_firefox) in this bug.
Flags: needinfo?(squibblyflabbetydoo)
Oh, and sorry for all the confusion. This is a bit convoluted!
no problem :) OK I will rebase this one after bug 1031430 and I will ping you back for r?
Hey Jim, the encoding you did on the firefox ringtone sounds good. So lets keep the same quality throughout. Here are the raw files:
https://mozilla.box.com/s/uc8k5bo0c12g15zh9sl2

Thanks!
Flags: needinfo?(padamczyk) → needinfo?(squibblyflabbetydoo)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Hey Jim,
hope everything is ok now
Attachment #8435770 - Flags: review- → review?(squibblyflabbetydoo)
Comment on attachment 8435770 [details] [review]
patch for Gaia/master

Clearing review for now, since we should convert the sounds to .opus and we also need to discuss what the correct string for ringer_firefox should be.
Attachment #8435770 - Flags: review?(squibblyflabbetydoo)
Flags: needinfo?(squibblyflabbetydoo)
I thought it was just Firefox?
Flags: needinfo?(padamczyk)
All, the single ringtone is being tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=1031430

That's where the use of "Firefox" is noted so as not to have a new string. 

Patryk is on PTO.
Flags: needinfo?(padamczyk)
No longer blocks: 2.0-visual-refresh
No longer depends on: 1026698
No longer depends on: 1029096
No longer depends on: 1023133
QA Whiteboard: [2.1-feature-qa+]
Flags: in-moztrap?(mozillamarcia.knous)
QA Contact: mozillamarcia.knous
QA Whiteboard: [2.1-feature-qa+]
Whiteboard: interaction-design → interaction-design [2.1-feature-qa+]
Flags: in-moztrap?(mozillamarcia.knous) → in-moztrap?
QA Contact: mozillamarcia.knous
Flags: in-moztrap? → in-moztrap?(ashiue)
QA Contact: ashiue
QA Whiteboard: [COM=Ringtones]
QA Whiteboard: [COM=Ringtones] → [COM=Gaia::Ringtones]
Target Milestone: --- → 2.1 S2 (15aug)
Flags: in-moztrap?(ashiue) → in-moztrap-
Given we're in sprint 3 now, can we update the target milestone? If we're comfortable to land this by this month, please change the target milestone to sprint 3. Otherwise, please highlight it, and we should come out some mitigation plans. Thank you.
Flags: needinfo?(pivanov)
Close it because of Bug 1046399.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(pivanov)
Resolution: --- → INVALID
Pavel - All of our tracking flags are currently set on this bug, not bug 1046399. As such, I would prefer that we would keep this bug open for tracking & have the other bug duped this bug, as it will make EPM & QA tracking easier given this is the bug with the feature flag. Does that make sense?
Flags: needinfo?(pivanov)
Sure ... my mistake sorry
Status: RESOLVED → REOPENED
Flags: needinfo?(pivanov)
Resolution: INVALID → ---
Attachment #8435770 - Attachment is obsolete: true
Attachment #8438313 - Attachment is obsolete: true
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
(In reply to Jason Smith [:jsmith] from comment #65)
> Pavel - All of our tracking flags are currently set on this bug, not bug
> 1046399. As such, I would prefer that we would keep this bug open for
> tracking & have the other bug duped this bug, as it will make EPM & QA
> tracking easier given this is the bug with the feature flag. Does that make
> sense?

Thanks, Jason. Sorry, I don't really understand what you mean here. We're expecting all feature-b2g bugs should be closed @ FL. Do we expect this one to be closed at that time as well? This bug looks exactly the same as bug 1046399, and it looks like we're working on the engineering stuff in bug 1046399, but you set tracking flags in this bug, right? If bug 1046399 contains the required engineering stuff for this bug, we may need to configure the dependencies: bug 1014009 depends on bug 1046399, right?
Flags: needinfo?(jsmith)
(In reply to Kevin Hu [:khu] from comment #67)
> (In reply to Jason Smith [:jsmith] from comment #65)
> > Pavel - All of our tracking flags are currently set on this bug, not bug
> > 1046399. As such, I would prefer that we would keep this bug open for
> > tracking & have the other bug duped this bug, as it will make EPM & QA
> > tracking easier given this is the bug with the feature flag. Does that make
> > sense?
> 
> Thanks, Jason. Sorry, I don't really understand what you mean here. We're
> expecting all feature-b2g bugs should be closed @ FL. Do we expect this one
> to be closed at that time as well? This bug looks exactly the same as bug
> 1046399, and it looks like we're working on the engineering stuff in bug
> 1046399, but you set tracking flags in this bug, right? If bug 1046399
> contains the required engineering stuff for this bug, we may need to
> configure the dependencies: bug 1014009 depends on bug 1046399, right?

I think we can close this since bug 1046399 landed. Right, this bug has the tracking flags set on it.
No longer blocks: 1046399
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Depends on: 1046399
Flags: needinfo?(jsmith)
Resolution: --- → FIXED
Verified landed
Gaia      2be78d83a760fa3b9638fe51c266b442d14597f1
Gecko     https://hg.mozilla.org/mozilla-central/rev/1db35d2c9a2f
BuildID   20140831160203
Version   34.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.