866907 - [Gaia]NFC enable/disable widget in Settings

Status: RESOLVED FIXED

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

Description

[Garner Lee]

Gecko and Gaia changes.

Comment 1

[Garner Lee]

Attachment: Gaia side changes to enable/disable nfc.enabled settings property.

Initial Gaia settings update + Review(er) request. Known issues: Missing NFC toggle icon, no communication between settings and actual system property (nfc.enabled) controlling the nfc daemon being set (fail/success).

Comment 2

[Garner Lee]

Attachment: Gecko side changes to nfc.enabled system property toggle.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 764428 [details] [diff] [review]
Gaia side changes to enable/disable nfc.enabled settings property.

Can't review until webnfc has landed. Re-request review when patches code are on top of have landed.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 764430 [details] [diff] [review]
Gecko side changes to nfc.enabled system property toggle.

Can't review until webnfc has landed. Re-request review when patches code are on top of have landed.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Garner

Do you think we should have API for enable/disable , so we don't have to add Settings?

Recently there are some discussions the abuse of 
settings.
(RIL has the similar problem).

Do you think we could ask Fabrice Desre in dev-b2g or this bug?

Thanks

Comment 6

[Garner Lee]

Hi Yoshi,

It's not too difficult to add. do you have a pointer on the nature of, and how the settings abuse happens in RIL?

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi,
There are some lengthy discussion in web-webapi,
subject is "Are we abusing Settings API"

Also for the RIL, we have a bug for that, Bug 856553.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #7)
> 
> Also for the RIL, we have a bug for that, Bug 856553.

But actually the RIL one is more related to session management in voice/data networks, which is not what I ask in the Comment 5.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Ha, I found the bug for abuse of Settings API, Bug 861794.

Comment 10

[psiddh]

Hi Yoshi,
The Bug 861794 indeed talks about having to remove some unnecessary APIs for settings. However NFC ON/OFF settings is necessary and useful should the user feels a need to toggle the settings. I think we have also seen this requirement being mentioned in Operator's MTR documents. (Note that Android does have similar settings)

Comment 12

[psiddh]

Couple of things to consider, When user enables / disables NFC settings

1) Upon disabling through settings app, should we completely stop nfcd daemon and viceversa?

   This approach means that nfcd socket end point shuts itself off, while nfc_worker side end point would continuously try to connect to nfcd socket in a never ending loop. May be we can introduce a delay while connecting to socket.
(UnixSocket , already supports 'connectSocket' with a delay). But 'NFC settings'
can be turned off by the user for longer duration. In that scenario it will keep
trying to connect to nfcd (with a delay say of 4s) till nfcd daemon is UP and RUNNING.

2) Alternate approach is to expose a passage (extend the JSON / binary contract) through protocol (b/w nfc_worker & nfcd) that relays ON and OFF requests to daemon. The daemon would call vendor chip-set specific APIs to turn ON / OFF discovery. (NXP supports such calls and provides APIs to do the same)

   This approach means that we would not shut the daemon down, but selectively turning ON/OFF chip-set capabilities.

3) Do we have any other suggestions / alternatives here?

Comment 13

[Garner Lee]

FYI: Gecko side implementation (without Gaia UI) has been posted in the WebNfc bug:
https://bug674741.bugzilla.mozilla.org/attachment.cgi?id=800288

Comment 14

[Ken Chang[:kenkai]]

Garner, Moz UX team has put their design on bug 904246. I wonder if it is okay for you to base on moz UX design to implement NFC enable/disable feature. And When airplane mode is on, the NFC has to be disabled.

Comment 15

[Garner Lee]

(In reply to Ken Chang from comment #14)
> Garner, Moz UX team has put their design on bug 904246. I wonder if it is
> okay for you to base on moz UX design to implement NFC enable/disable
> feature. And When airplane mode is on, the NFC has to be disabled.

I think UX design and icon is relatively straightforward and looks useful, though there's always a bit of confusion possible: "wireless symbol on tag" --> NFC Tag, or RFID tag?. I don't know if there's a official guideline for a NFC icon, though the NFC Forum org trademark is here:

http://www.nfc-forum.org/home/

If all is fine, at implementation time, just tell me where to pick up the png.

Comment 16

[Ken Chang[:kenkai]]

Juwei, any idea for Garner's question.

Comment 17

[Garner Lee]

There is a proposal by the FAA in the US at least to loosening the meaning of "Airplane mode", which could affect settings design directly tied to "ril.radio.disabled" in gaia/apps/settings/js/airplane_mode.js:

http://www.theverge.com/2013/9/26/4775608/faa-panel-formally-recommends-loosening-electronics-restrictions

Comment 18

[GoodMike]

UX team plans to design the NFC icon looks like "NFC"(which is used widely in many devices already), it is more straightforward and clear to users while they turn it on. Same mental model as BT and Wi-Fi. For airplane mode, Juwei will add this spec, when turning on Airplane mode, NFC will be disabled. As to Garner's FAA reference, if they loosen the restriction in the future, we should refine NFC settings/behavior in Airplane mode in respond to it. I am wondering if I do answer the questions ?

Comment 19

[Juwei Huang[:juwei]]

Hi Ken,
I will provide NFC icons first (for both status bar & settings app), then add one spec for Airplane mode.
thanks,

Comment 20

[Garner Lee]

Attachment: Patch (v2) Gaia side changes to enable/disable nfc.enabled settings property. Also includes Airplane mode.

Comment 21

[Garner Lee]

Attachment: Patch (v3) Gaia side changes to enable/disable nfc.enabled settings property. Also includes Airplane mode.

Comment 22

[Garner Lee]

Comment on attachment 825718 [details] [diff] [review]
Patch (v3) Gaia side changes to enable/disable nfc.enabled settings property. Also includes Airplane mode.

Please view V3 (added airplane mode from system as well).

Comment 23

[Evelyn Hung]

Comment on attachment 825718 [details] [diff] [review]
Patch (v3) Gaia side changes to enable/disable nfc.enabled settings property. Also includes Airplane mode.

Review of attachment 825718 [details] [diff] [review]:
-----------------------------------------------------------------

In case of device doesn't have NFC support, I think the UI toggle should be hidden. Please add this check and also also address my comment inline.

If you can send a pull request on the Github instead of a patch file, I will appreciate. A pull request will go through our test cases, and it's easier to merge. You can refer this wiki page to know more: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking#Submitting_a_patch

Thanks for the patch.

::: apps/settings/js/nfc.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Gaia is Apache License, and we normally don't add licensing blurbs on top of the files. Please remove these three lines.

@@ +7,5 @@
> +'use strict';
> +
> +// handle Nfc settings
> +navigator.mozL10n.ready(function nfcSettings() {
> +  var DEBUG = true;

DEBUG flag should be turned off by default.

@@ +27,5 @@
> +    }
> +  }
> +
> +  // activate main button
> +  gNfcCheckBox.onchange = function changeNfc() {

Every checkbox with name=<setting_key> attribute will be handled by settings.js to update its corresponding value into mozSettings database. So you don't need to update again here.

@@ +66,5 @@
> +  // register an observer to monitor nfc.enabled changes
> +  settings.addObserver('nfc.enabled', function(event) {
> +    debug('NFC enabled change detected: ' + event.settingValue);
> +    updateNfcSettingUI();
> +  });

similar to above, all settings update will be caught by settings.js and update corresponding UI toggle.

@@ +73,5 @@
> +  navigator.mozSetMessageHandler('nfc-powerlevel-change',
> +                                 function(message) {
> +    debug('Re-activate NFC checkbox interaction');
> +    gNfcCheckBox.disabled = false;
> +  });

I don't understand what's the |nfc-powerlevel-change| for? Why it's a system message? the purpose of using system message is you want Settings app been launched (if it hasn't) while receiving this message. I don't see the use case here because it's simply for update the toggle (if Settings is not opened, we don't need to update the toggle).

Comment 24

[Garner Lee]

I'll investigate the comments, update, and do pull requests on GitHub. However, it seems the review process is still here on bugzilla?

If system messages will also start the Settings UI, (it is also expected to be controlled by Settings UI, though nfc_manager.js in system app can also manage the screen states to try to save power), is there better method to disable the control, while waiting for a response? It's intended to prevent spamming enable/disable requests to the firefox platform level NFC Daemon process.

Comment 25

[Kai-Chih Hu [:khu]]

Hi, do we have a clear view on the target milestone on this bug now? Thanks.

Comment 26

[Ken Chang[:kenkai]]

(In reply to Garner Lee from comment #24)
> I'll investigate the comments, update, and do pull requests on GitHub.
> However, it seems the review process is still here on bugzilla?
Yes.
> If system messages will also start the Settings UI, (it is also expected to
> be controlled by Settings UI, though nfc_manager.js in system app can also
> manage the screen states to try to save power), is there better method to
If nfc manager wants to save power, can it directly use NFC webapi? Why does
it need to start Setting UI for this?
> disable the control, while waiting for a response? It's intended to prevent
> spamming enable/disable requests to the firefox platform level NFC Daemon
> process.

Comment 27

[Evelyn Hung]

(In reply to Garner Lee from comment #24)
> I'll investigate the comments, update, and do pull requests on GitHub.
> However, it seems the review process is still here on bugzilla?
> 
We(Gaia reviewers) usually give detailed comments on Github, and set review result (r+/r-) on bugzilla with a briefly comment. I know it's a bit confusing. :(

Comment 28

[Garner Lee]

There's only one value in use currently, and in setttings: nfc.powerlevel's value.

They are related in the sense the settings UI needs to pay attention to the same power events as System (which has no UI). Flip the UI, System (nfc_manager) has to be notified somehow as well. If the screen is off/locked, nfc_manager notices, then goes off and changes the settings state, which may notifiy the UI, if it is running.

My question then is, if there's a better way (as this methond apparently can unnecessarily start the UI), I'm willing to use another proposed messaging method (i.e set yet another settings var for pending power changes).

Comment 29

[Ken Chang[:kenkai]]

(In reply to Garner Lee from comment #28)
> There's only one value in use currently, and in setttings: nfc.powerlevel's
> value.
> 
> They are related in the sense the settings UI needs to pay attention to the
> same power events as System (which has no UI). Flip the UI, System
> (nfc_manager) has to be notified somehow as well. If the screen is
> off/locked, nfc_manager notices, then goes off and changes the settings
> state, which may notifiy the UI, if it is running.
I wonder if nfc_manager need to change the value of settings when system wants to do power saving mode. To enable/disable NFC and to enter different power mode seem different things. To enable/disable NFC is what user want. To enter different power mode is what system want. I would like to suggest separating these things.

Hi Evelyn, any suggestion? Thanks.

Comment 30

[Ken Chang[:kenkai]]

It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.

Comment 31

[Evelyn Hung]

(In reply to Ken Chang from comment #29)
> (In reply to Garner Lee from comment #28)
> > There's only one value in use currently, and in setttings: nfc.powerlevel's
> > value.
> > 
> > They are related in the sense the settings UI needs to pay attention to the
> > same power events as System (which has no UI). Flip the UI, System
> > (nfc_manager) has to be notified somehow as well. If the screen is
> > off/locked, nfc_manager notices, then goes off and changes the settings
> > state, which may notifiy the UI, if it is running.
> I wonder if nfc_manager need to change the value of settings when system
> wants to do power saving mode. To enable/disable NFC and to enter different
> power mode seem different things. To enable/disable NFC is what user want.
> To enter different power mode is what system want. I would like to suggest
> separating these things.
> 
Yes. They are different cases. Here is my suggestion to achieve your goals:

1) enable/disable: You may notice that we currently use mozSettings to enable/disable BT/Wifi/..., but we are planing to get rid of this way since it introduces some racing issues. So I don't recomment it. Instead, please use an explicit API call, like mozNfc.setEnable() and mozNfc.setDisable() to turn it on/off, and leave nfc.enabled as a purely user preference in mozSettings database. You will also need to add an event listener to know the state transitioning from disable -> enabling -> enabled for UI update. (refer bug 928851 comment 6 for API design example.) 

2) power mode: I can think of a similar use case, that is wifi. When the screen is off, wifi connection will be suspend for power saving. The connection will be restored (wifi ON) when screen is turned on. I guess it's the case you are looking for, and you may have checked the code "wifi.js" in System app. It's all done in System app, and Settings isn't (and doesn't need to) aware of this change because when the user switches to Settings, he/she still see the NFC is ON per his/her request last time. So there is no state sync effort between System app and Settings app when NFC is suspend due to screen off. Does this case fulfill your needs?

Comment 32

[Garner Lee]

Thanks, I'll decouple Settings UI (User only on/off setting) and NFC Manager (power management).

Comment 33

[Garner Lee]

Attachment: Patch (v4) Gaia side changes to enable/disable nfc.enabled settings property. Also includes Airplane mode.

Changes implemented, github pull request submitted (as "v2"), while this one gets reviewed/commented on.

About the patch naming, v4 overall, but our branch puts it at v2, ignore version.

Comment 34

[Garner Lee]

github pull at: https://github.com/mozilla-b2g/gaia/pull/13723

Comment 35

[Garner Lee]

Attachment: 0001-Bug-866907-v5-Settings-UI-setting-and-airplane-mode.patch

Removed powerlevel from settings manifest.

Comment 36

[Garner Lee]

New pull request for v5: https://github.com/mozilla-b2g/gaia/pull/13725
(powerlevel stays, it's a shared global setting currently until nfc setConfig nfcd changes are made to use an event, or a privilaged NFC DOM API).

Comment 37

[Evelyn Hung]

(In reply to Garner Lee from comment #36)
> New pull request for v5: https://github.com/mozilla-b2g/gaia/pull/13725
> (powerlevel stays, it's a shared global setting currently until nfc
> setConfig nfcd changes are made to use an event, or a privilaged NFC DOM
> API).

You can just paste your Github pull request URL directly in the attachment
text box and it will auto-detect. No need to attach an extra patch file.

Comment 38

[Evelyn Hung]

Comment on attachment 832707 [details] [diff] [review]
0001-Bug-866907-v5-Settings-UI-setting-and-airplane-mode.patch

Please write more meaningful commit message instead of "(v5) Settings UI, setting, and airplane mode". See more detail comments on Github.

Comment 39

[Garner Lee]

Attachment: V6 - Settings-UI

Bug 866907: (v6) Simplified settings UI nfc.js, review changes implemented.  
    - Greatly simplified nfc.js (uses name, and targeted IDs instead for performance)
    - nfc.powerlevel will probably be updated and removed (Bug 933184)
    - nits.

Comment 40

[Garner Lee]

Attachment: (v6) Settings UI

Comment 41

[Garner Lee]

Attachment: (v6) Settings UI

Comment 42

[Juwei Huang[:juwei]]

Attachment: [NFC] NFC Setting_20131118.pdf

Hi all, spec updated: Added airplane mode

Comment 43

[Evelyn Hung]

Comment on attachment 833288 [details] [review]
(v6) Settings UI

Basically the patch is okay, I commented some nits on Github, please check there. Also, I don't like the commit message, please remove meaningless (to other people) words like "(v6)" and "review changes implemented." The most important things you've done in this patch are 1. add a toggle for enabling and disabling NFC; 2. turn off NFC when entering airplane mode
Please make these points in your commit message. 
feedback+, I'd like to review again and r+ after my comments been addressed. Thanks!

Comment 44

[Garner Lee]

Attachment: NFC Settings UI implemenation + Airplane mode. (v7)

Comment 45

[Evelyn Hung]

Comment on attachment 8334751 [details] [review]
NFC Settings UI implemenation + Airplane mode. (v7)

I don't have a device with NFC chip to test this patch, but the code looks good to me.
r+, thanks for your hard work.

Comment 46

[Evelyn Hung]

merged into gaia-master:
https://github.com/mozilla-b2g/gaia/commit/25c52793f418e870319fb72247e2faeed141fd6c

Comment 47

[Garner Lee]

Thanks! Now it just needs an icon :)

Comment 48

[Fabien Cazenave [:kaze]]

FTR, an l10n key has been added and left undefined in this patch — see bug 941951.