982949 - Ringer selection should have a default value.

Status: RESOLVED FIXED

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

Description

[Stephany Wilkes]

Settings > Sound > Ringer (Helix 20140306040204): There is no default value. Expected: There is a default value selected.

Also flagging Omega to specify the default value.

Comment 1

[Jason Smith [:jsmith]]

I think this is a dupe.

Comment 2

[Omega Feng [:Omega] [:馮於懋]]

Attachment: [1.5 Settings] Ringer & Alerts default values v1.0.pdf

Here is the spec.

Comment 3

[Omega Feng [:Omega] [:馮於懋]]

See Bug 991026 for the Sound UX spec update.

Comment 4

[Dominic Kuo [:dkuo]]

Attachment: patch after bug 973445 landed

Arthur,

This patch is to fix the default values of the tones in the sound settings, it will fix bug 982951 as well since the code blocks are the same. The main idea is to use the file paths as id to find the preloaded ringer/alert tones then display them on the ui, also the tone name can be localized if the users change the languages, which also fixed the potential bugs. As you know, sound settings are being refactoring in bug 973445, my patch is base on the refactored code so probably you will need to wait for it landed or pull the patch in bug 973445 yourself to test it, sorry about that. Would you please review this patch? thanks.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 8415033 [details] [review]
patch after bug 973445 landed

This patch also modified the build script so I think we should let owner/peer to also know about this, Yuren, would you please see the build script part and give us some feedback? thanks.

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8415033 [details] [review]
patch after bug 973445 landed

looks good to me for build script part.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8415033 [details] [review]
patch after bug 973445 landed

Thanks for the patch, dominic! Please check my comments in github.

Comment 8

[Jim Porter (:squib)]

Please, I beg of you: don't land this. It will bitrot my ringtones branch pretty badly. I promise I'll incorporate whatever changes are here into my branch.

Comment 9

[Jim Porter (:squib)]

Oh, and see bug 960329 for my branch.

Comment 10

[Jim Porter (:squib)]

Ok, looking at this a bit more, I guess I should probably explain some of my motivation for asking that this not get landed until my patch does: the main reason is that I'm changing how we store ringtones in mozSettings, and the way I'm doing things should simplify a few parts of this patch. Overall, I think it makes more sense to wait on this bug until after bug 960329 is landed, since that will reduce the total amount of changes we need to make in the settings app.

The main thing I've added in my branch that helps with this is that every ringtone now has a unique ID that gets stored in mozSettings. I'm also thinking that instead of deriving the l10n ID for ringtones from the filename, we can use some of the changes I made to actually store the l10n ID in mozSettings and just share l10n files between the ringtones and settings apps.

Comment 11

[Dominic Kuo [:dkuo]]

Don't worry Jim, I am not going to land this yet and it's also blocked by bug 973445. Apologize for not notifying those changes about the ringtones to you because I am working on the 1.3T+ music blockers(you knew that) and just let this patch pending here. And thanks for letting us know the huge patch of ringtones, I think your idea is very similar to my patch and also address the localization issue in ringtones, we should land bug 960329 first, it will be nice that this bug can concentrate on the default values for the ring/alert tones, which most of the works should be in the build script and some in the sound.js in settings.

Comment 12

[Jim Porter (:squib)]

Here's a quick idea for what we could do based on my branch: https://github.com/jimporter/gaia/commit/243d514a3d3b4cc0d7b9e5885d977269dde5726a

The only downside to the above strategy is that I'm changing how dialer.ringtone.name is defined so that I can store either a string *or* an L10N ID in it. Maybe we can migrate the old values somehow.

Comment 13

[Jim Porter (:squib)]

I fixed a few issues in my branch, so here's the commit now: https://github.com/jimporter/gaia/commit/c1ed7b1eaa97fb6258d3038a29fb2f1ae4e5d445

Comment 14

[Jim Porter (:squib)]

Updated my branch again. Here's the whole branch (just look at the latest commit, though): https://github.com/jimporter/gaia/commits/ringtones-default

Comment 15

[Dominic Kuo [:dkuo]]

Cool, and if you don't have time to write the tests, you can just pick up those I have for now or I can help to add after you landed you branch, great work Jim!

Comment 16

[Jim Porter (:squib)]

Ok, I'll merge your code into my branch. I should be able to keep most everything you wrote; it's just that it'll be easier to do the merge with your code on top of mine, rather than the other way around.

Taking this bug to reflect that I'm finishing things up.

Comment 17

[Jim Porter (:squib)]

Landed as part of bug 960329.