[Volume][Settings]After flashing the volume should not be set at max

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nhirata, Assigned: janjongboom)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [tef-triage] c=)

Attachments

(1 attachment)

## Environment :
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/cfef36c0c8bc
Gaia   a80be95f553de517e5d8a159e04511cda5e38be4
BuildID 20130409070205
Version 18.0
Unagi
  
## Repro :
1. flash the device
2. go to settings -> sounds
3. look at the volume

## Expected :
it's set midway

## Actual :
everything is set to max

## Note :
- Every time I forget that I just flashed it, I end up having my ears blown.
- happens in all versions.  Rather annoying.
- I believe this is the case for resetting the phone as well.
Don't think we should block on this one
Whiteboard: [tef-triage]
Given that users will not be regularly flashing phones, this is not a blocker but a nice-to-have for testers.
blocking-b2g: tef? → -
Whiteboard: [tef-triage] → c=
This is not just on flashing, also normal restart (master, on Gaia 6dbaa14222e624f0fc436c2f5b4730409b8491f8, B2G d07cc703cfa8205fd77b2d8258fc0069f2d718a0 against mozilla-central 4ac6c72b06c8+) triggers the volume to reset to max. Which is highly annoying.
Re-nominating for tef to verify if a restart also triggers a volume reset on 1.0.1.
blocking-b2g: - → tef?
(Assignee)

Updated

5 years ago
Assignee: nobody → janjongboom
Created attachment 755236 [details] [diff] [review]
Patch

When we currently start up the phone we add a bunch of observers for the settings related to the volume. All good. However, we skip the check if we have any pending requests. When the first result comes in, it can trigger a chain of reactions which then spawns new pending requests, causing the other loaded settings to be omitted and the volume of f.e. the ringer to be kept at the original of 15. This patch makes sure that the initial load event for every setting will bubble through.
Attachment #755236 - Flags: review?(timdream)
Comment on attachment 755236 [details] [diff] [review]
Patch

Redirecting review as Alive is the original care-taker of the sound manager code :)
Attachment #755236 - Flags: review?(timdream) → review?(alive)
The problem of comment 0 is because we set the default volume to max(from long time ago).
We could just set it to arbitrary value we like.

I tried on master, and after rebooting the notification/ringer volume is not max.
Will try on v1.0.1 soon.
Comment on attachment 755236 [details] [diff] [review]
Patch

Sorry, I don't think this fixes anything.

The pending request only happens when the user uses the volume rockers to adjust the volume level. For your guess, it's nearly impossible because the user won't press the volume rocker 'just before the observer callback and after booting'. It's a very short interval.

BTW, the pendingRequestCount is a number, not a object. I think pendingRequestCount[channel] will cause trouble.
Attachment #755236 - Flags: review?(alive) → review-
Comment on attachment 755236 [details] [diff] [review]
Patch

@Alive, no this is not the case. The settingsObserver also fires directly after startup of the phone and sets the initial values. You can see this behavior in https://github.com/mozilla-b2g/gaia/blob/master/shared/js/settings_listener.js#L37. This means that for all of these keys the initial value will be read immediately after startup but it'll be ignored for everyone who doesn't finish first, thus not setting the volume.

The change of pendingRequestCount was unintentional, I've got that out of the way and resquashed.
Attachment #755236 - Flags: review- → review?(alive)
It's also easily verifiable on Gaia in Firefox Nightly if you don't want to flash phone.
FYI, add on line 283: `console.log('SettingsListener.observe', channel, volume, pendingRequestCount);`

[13:51:14.604] "SettingsListener.observe" "alarm" 15 0
[13:51:14.605] "SettingsListener.observe" "notification" 11 2
[13:51:14.605] "SettingsListener.observe" "telephony" 5 2
[13:51:14.606] "SettingsListener.observe" "content" 15 2
[13:51:14.606] "SettingsListener.observe" "bt_sco" 15 2

And you'll see that at the moment stuff is not put through because of this.
still thinking it is not tef+
Whiteboard: c= → [tef-triage] c=
Comment on attachment 755236 [details] [diff] [review]
Patch

OK I know what happens.
If the pendingRequestCount is not 0,
we should find out what's happening instead skip it.

The problem is the fetchCachedVolume here, due to some race condition, it may return the result too late.

So the solution is delay the call to fetchCachedVolume function call after all of the channels are retrieved first time.

Yes this is my design fail, but we should provide a real fix.

If you don't want to work on this I will take it, thanks.
Attachment #755236 - Flags: review?(alive) → review-
Comment on attachment 755236 [details] [diff] [review]
Patch

Alive, thanks, I've changed the PR to take that into accont. Good catch. I've also seen that `CACHE_CETIMES` is declared but we write `CACHE_CETIME` to asyncStorage, so to the undefined key. I've added it to the PR, if you want it out please say so.
Attachment #755236 - Flags: review- → review?(alive)
Comment on attachment 755236 [details] [diff] [review]
Patch

Awesome. r=alive, thanks for your patch and patience.
Attachment #755236 - Flags: review?(alive) → review+
Landed in https://github.com/mozilla-b2g/gaia/commit/979126f56c6ad3608770049f5ba71cf3e0c3857f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
triage: agree with comment 2, tef-. should not block release with this.
blocking-b2g: tef? → -
Duplicate of this bug: 878740
tef? because dup 878740 is tef+
blocking-b2g: - → tef?

Updated

5 years ago
blocking-b2g: tef? → tef+
(In reply to Alive Kuo [:alive] from comment #20)
> v101
> https://github.com/mozilla-b2g/gaia/commit/
> bf10abc41a01516995a99f3c6727a612bbdfe755

is this applicable to v1-train?  If so, can you please land on that branch?
Flags: needinfo?(alive)
(In reply to John Ford [:jhford] -- If you expect a reply from me, use needsinfo? instead of CC from comment #21)
> (In reply to Alive Kuo [:alive] from comment #20)
> > v101
> > https://github.com/mozilla-b2g/gaia/commit/
> > bf10abc41a01516995a99f3c6727a612bbdfe755
> 
> is this applicable to v1-train?  If so, can you please land on that branch?

No problem.
Flags: needinfo?(alive)
https://github.com/mozilla-b2g/gaia/commit/390104667c95a8bbd416fd95117577cbaa746901
v1-train
status-b2g18: --- → fixed
status-b2g18-v1.0.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.