Closed Bug 876597 Opened 11 years ago Closed 11 years ago

[FM Radio] FM is not geting disable in Airplane Mode

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::FMRadio
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(blocking-b2g:leo+, b2g18 verified)

Status:
RESOLVED FIXED
Milestone:
1.1 QE2 (6jun)
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
b2g18 --- verified

People

(Reporter: leo.bugzilla.gaia, Assigned: arthurcc)

Details

(Whiteboard: MiniWW, late-l10n)

Attachments

(2 files, 1 obsolete file)

Link to https://github.com/mozilla-b2g/gaia/pull/10051
182 bytes, text/html
wchang
: review+
Details
screenshot of the prompt
13.62 KB, image/png
Details 
[FM Radio] FM is not geting disable in airplane Mode
1. Title: FM works in Airplane mode 
2. Precondition: Set the airplane mode
3. Tester's Action: (1) Insert HEADSET
                    (2) Open FM
                    (3) FM is playing
4. Detailed Symptom (ENG.) :a) when airplae mode is set and user open the FM, it starts playing. b) when FM is playing in background -> airplane mode get set -> FM get power off.But ones again -> Open FM -> Power on -> start playing							
5. Expected: Fm shou;d diable fully in Airplane mode
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on Gaia master
8. Gaia Revision:  9380ceb81b3eac45861b8d1be07ab7f748ed52a3
9. Personal email id: ntiaj12345@gmail.com
As discussed in San Diego work week marking this as leo+.
blocking-b2g: --- → leo+
Whiteboard: MiniWW
Assignee: nobody → arthur.chen
Per airline requirement described in

http://www.united.com/web/en-US/content/travel/baggage/devices.aspx

Devices that are not permitted for use at any time:
    battery-operated personal air-purifying devices
    TVs
    radio receivers and/or transmitters (including AM/FM/SW, CB and scanners)
    remote-control toys.
Pin, could you help review the change? I simply make the buttons disabled when the airplane mode is turned on. Thanks!
Attachment #755068 - Flags: review?(pzhang)
Hi Arthur, about the symptom listed in the description:
  a) when airplane mode is set and user open the FM, it starts playing

does your patch fix this?
(In reply to Pin Zhang [:pzhang] from comment #4)
> Hi Arthur, about the symptom listed in the description:
>   a) when airplane mode is set and user open the FM, it starts playing
> 
> does your patch fix this?

And I am thinking that should we handle this in Gecko layer instead of Gaia app?
Yes my patch disabled the button when the airplane mode is turned on, so that users cannot open FM. It would be great if gecko handles this, but I think we still need some UI changes here.
(In reply to Arthur Chen [:arthurcc] from comment #6)
> Yes my patch disabled the button when the airplane mode is turned on, so
> that users cannot open FM. It would be great if gecko handles this, but I
> think we still need some UI changes here.

But the app enables the FMRadio in the `load` event handler without checking if the setting 'ril.radio.disabled' is false, it only observes the setting:

   SettingsListener.observe('ril.radio.disabled', false, function(value) { 

which means the handler only reacts when the setting is changed.
I am working on Bug 870676, and I will handle this in new patch.
Is there any ETA for bug 870676? This bug is expected to be fixed in the mini work week. 

And you are right, we need to maintain the current status of `ril.radio.disabled` for checking. I'll update the patch.
(In reply to Arthur Chen [:arthurcc] from comment #9)
> Is there any ETA for bug 870676? 

Not yet, it's only marked as [MemShrink:P3].
I updated the patch. Added a variable for tracking the status of `ril.radio.disabled`. The data attribute was also renamed.
(In reply to Arthur Chen [:arthurcc] from comment #11)
> I updated the patch. Added a variable for tracking the status of
> `ril.radio.disabled`. The data attribute was also renamed.

I am afraid it still doesn't cover the case mentioned in comment 4, let's consider:

  1) Open Settings APP, and Enable airplane mode
  2) Go back to homescreen, and open the FM APP

the FM app will call `enableFMRadio()` in `window.onload` handler, but the initial value of `rilDisabled` is false, so the FM Radio will still be turned on. So I think we should read the setting value to initialize `rilDisabled`, and then decide whether call MRadio.enable() or not.

BTW, there are several TAB characters in the index.html, could you help to replace them with spaces? Thanks.
Pin, thank you for reviewing. I've updated the patch addressing your comments. If the implementation is okay for you, I'll add corresponding test cases for it.
(In reply to Arthur Chen [:arthurcc] from comment #13)
> Pin, thank you for reviewing. I've updated the patch addressing your
> comments. If the implementation is okay for you, I'll add corresponding test
> cases for it.

Arthur, we already have function `updatePowerUi` to do the styling stuff for `#power-switch`, so I think we can merge the function `updateRadioUI` into it.
The other parts look good to me
Pin, I've addressed your comment and added the test cases. Would you mind take a look again? Thanks!
Target Milestone: --- → 1.1 QE2 (6jun)
(In reply to Arthur Chen [:arthurcc] from comment #15)
> Pin, I've addressed your comment and added the test cases. Would you mind
> take a look again? Thanks!

Hi Arthur, please check my comments on the pr, thanks
Attached image screenshot of disabled UI (obsolete) — 
Casey, we are trying to disable the airplane mode related UIs when the airplane mode is turned on. Could you help check the screenshot to see if the style is okay?
Flags: needinfo?(kyee)
Arthur, if you don't get UX feedback today lets land this and we can do a separate follow-up if the feedback comes back different.
Hi guys, a better UX solution would be to generate an error prompt when the user opens the app while Airplane Mode is On. This prompt should be similar to the "Must plug in your headphones" prompt that appears when the user opens the app without first plugging in headphones. The string should read something like:

Title: Airplane mode is on.
Body: Turn off Airplane mode to use FM Radio.
Button: Cancel (closes app and returns user to Home app)

Is there time to land the strings required for this change?
The reason this approach is better, btw, is it explains to the user the cause of the error state, and how to resolve it.
Arthur, you said on PR:
  Before applying this patch, FM Radio is disabled from gecko when ril.radio.disabled is set to true. 

But I didn't find the related codes to do this, would you mind providing more infos?
I'm not sure where the code is. The statement was made by the observation: FM radio is stopped immediately when the airplane mode is turned on.
Thanks Josh.

Wayne, are we able to add new strings now?
Flags: needinfo?(wchang)
Thanks, Josh. Also flagging stas for localization.
Flags: needinfo?(stas)
Whiteboard: MiniWW → MiniWW, late-l10n
We haven't gotten input on a string freeze for 1.1 yet.
Flags: needinfo?(stas)
(In reply to Arthur Chen [:arthurcc] from comment #22)
> I'm not sure where the code is. The statement was made by the observation:
> FM radio is stopped immediately when the airplane mode is turned on.

Aha, I found the codes in $GAIA/apps/system/js/airplane_mode.js line 115:

  // Turn off FM Radio.
  if (fmRadio && fmRadio.enabled) {
    fmRadio.disable();
  }
Removing the flag for Casey since Josh addressed this. Thanks Josh!
Flags: needinfo?(kyee)
I don't want to block on UX here. Let's land the patch from comment 17, fulfilling our commitments to requirements, and we can add this prompt separately.
I've created a separate bug for implementing the Airplane Mode, here:

"Add prompt for airplane mode"
https://bugzilla.mozilla.org/show_bug.cgi?id=878135
I've created a separate bug for implementing the Airplane Mode prompt, here:

"Add prompt for airplane mode"
https://bugzilla.mozilla.org/show_bug.cgi?id=878135
Josh, actually I've added the prompt UI. Could you help confirm it?
Attachment #755737 - Attachment is obsolete: true
How do we treat similar strings? Are the words "turn off" always together? If so, let's stay consistent (i.e. the way it is in comment 31). Otherwise, I'd do this:

Turn Airplane Mode off to use FM Radio.
Pin, I implemented Josh's proposal. Would you mind take it a look? Thanks!
ni?josh for comment 31 :)
Flags: needinfo?(wchang) → needinfo?(jcarpenter)
(In reply to Arthur Chen [:arthurcc] from comment #31)
> Created attachment 756720 [details]
> screenshot of the prompt
> 
> Josh, actually I've added the prompt UI. Could you help confirm it?

The prompt looks good, but those strings need localization. We can only implement this if the L10N team confirms they can support the translation work in time. Axel, per comment #25, what's the way forward here?
Flags: needinfo?(l10n)
Please land this first and have l10n tracked after.
Josh, we found a problem which is when users try to close the app using the close button in the airplane mode warning page, there is a white screen flashing. So I remove the button here to avoid the problem, which also makes it consistent to the antenna unavailable warning page. We can use the follow up bug to track this issue.
Comment on attachment 755068 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10051

r=gal

Andreas has reviewed this during the workweek.
Attachment #755068 - Flags: review?(pzhang) → review+
Josh, we ended up not having a close button. The dialog auto-hides when the user switches airplane mode off. This is consistent with the way the headphone jack detection works.

There is a race condition when we trigger closing the app via the close button, which is why we punted on that for now.
We've not been included in schedule discussions so far.
Flags: needinfo?(l10n)
(In reply to Andreas Gal :gal from comment #40)
> Josh, we ended up not having a close button. The dialog auto-hides when the
> user switches airplane mode off. This is consistent with the way the
> headphone jack detection works.
> 
> There is a race condition when we trigger closing the app via the close
> button, which is why we punted on that for now.

Makes perfect sense. Sounds good.
Flags: needinfo?(jcarpenter)
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap-
Flags: in-moztrap- → in-moztrap+
Unagi
Gaia:     55ed5e08a2250ea2d3571fff860c39e66fabed14
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8
BuildID   20130715070218
Version   18.0

Verified.
status-b2g18: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: