806003 - [Settings] Awkward GPS state when Airplane Mode is tapped continuously in the Settings app

Status: VERIFIED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(credit goes to Nicolas B. Pierron for showing me this)

1. Go to Settings app.
2. Turn on GPS here.
3. Quickly tap the Airplane Mode continuously.

The GPS will awkwardly move between activated and non-activated while this is happening.


===

My Git commit info currently shows:

2012-10-24 11:07:05
fcfa1857bed6596e992263206451c6814e4b2... (I see ellipsis at the end)

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #0)
> (credit goes to Nicolas B. Pierron for showing me this)
> 
> 1. Go to Settings app.
> 2. Turn on GPS here.
> 3. Quickly tap the Airplane Mode continuously.
> 
> The GPS will awkwardly move between activated and non-activated while this
> is happening.

After all awkward switches, when the airplane mode is restored to its disable state, the GPS might not come back as expected. This one can also be reproduced with a fast double-tap.

Comment 2

[Evelyn Hung]

We need a solution to prevent quickly taps on the settings related to devices, such as airplane more (infects wifi, GPS, data connection), bluetooth, wifi, data connection ... etc, because it may cause asynchronized states. 
I know some components such as bluetooth and wifi do ignore continuous setting changes in their initial period, so the problems are: 
1. do we need to gray-out/disable the checkbox UI in this period? (need UX confirm)
2. a series of settings DB writing may fail, if we won't do (1), then we should prevent this kind of failure. (every switch writes settings DB now)

Since I probably won't have time to deal with this problem now, un-assign myself first. If anyone is interested in taking this issue, feel free to assign me as your reviewer.

Comment 3

[Kevin Grandon :kgrandon]

I tool a look at this and have some ideas about how to disable switches briefly after change.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

This pull request adds a 500ms delay to the same switch control being activated. The delay is short enough that double tapping on a control will not cause it to toggle twice, and it still feels useable to me.

Let me know what you think, I'm more than happy to investigate other solutions, but this seemed like the easiest to implement.

Comment 5

[Kevin Grandon :kgrandon]

Attachment: Toggle button interaction

Comment 6

[Kevin Grandon :kgrandon]

The above video should demonstrate various interactions with the phone. Word of warning - I am not a movie producer :)

Comment 7

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 10

[Evelyn Hung]

Comment on attachment 687467 [details]
Github pull request pointer

r=me, I'm afraid that this patch doesn't solve the problem. :-(  See my comments on the PR of Github.

Comment 11

[Kevin Grandon :kgrandon]

Working on a new fix for this here: https://github.com/KevinGrandon/gaia/compare/master...bug-806003

Still need to figure out the proper way to message between system and settings apps.

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer v2

Comment 13

[Dietrich Ayala (:dietrich)]

Hi Evelyn! When do you expect to be able to review this patch?

Comment 14

[Evelyn Hung]

Comment on attachment 690893 [details]
Github pull request pointer v2

I really spent a lot of time on testing this patch, but I still find some special cases will break the rule. I tried to fix them all but they are not so trivial to resolve, may need platform dev's help. So I think it's good enough to merge this patch first, I will file another issue for the cases I found. 
Please take a look at my comments on the PR and do updates, I'll review it again if you're done. Thanks.

Comment 15

[Evelyn Hung]

(In reply to Dietrich Ayala (:dietrich) from comment #13)
> Hi Evelyn! When do you expect to be able to review this patch?

I was doing it but didn't comment here. Sorry, I should update here more frequently. :(

Comment 17

[Dietrich Ayala (:dietrich)]

Driver retriage: Very uncommon user scenario and not easy to reproduce, not going to hold the whole release for this.

Comment 18

[Evelyn Hung]

Comment on attachment 690893 [details]
Github pull request pointer v2

r=me

Comment 19

[Evelyn Hung]

Comment on attachment 690893 [details]
Github pull request pointer v2

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 20

[Ben Francis [:benfrancis]]

Comment on attachment 690893 [details]
Github pull request pointer v2

Hi, Vivien asked me to process these approval requests.

We feel that the risk to reward ratio is too high to land this bugfix for 1.0 given there are a large number of JavaScript changes to fix an edge case bug.

We'd like to take this patch in a future version though.

Comment 21

[Kevin Grandon :kgrandon]

This one has finally landed in master: https://github.com/mozilla-b2g/gaia/commit/9f96874ab1cb473f3768bae50cf2a280f30ff3f5

QAWANTED: As this fix is a fairly complicated one, I'm adding qawanted to help verify that everything is working fine with airplane mode in the settings app. Thanks!

Comment 22

[pgravel]

Requesting this for v1.0.1

Comment 23

[Alex Keybl [:akeybl]]

(In reply to pgravel from comment #22)
> Requesting this for v1.0.1

Why should this risky change block our v1.0.1 release? The STR do not feel like a common user scenario.

Comment 24

[Michael Vines [:m1] [:evilmachines]]

Alex, this is part of product stability and has been raised by our test teams as well.

Comment 25

[Jeni]

This issue will be fixed on v1.0.1 per comment 22
    Build ID 20130214070203
    Kernel: Dec 5
    gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
    Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49

Comment 26

[Alex Keybl [:akeybl]]

Despite our strong disagreement here (and high likelihood for regression), this is considered a critical stop ship blocker for QC and we've therefore marked as tef+.

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

There was a white space merge conflict that needed to be resolved here.  I've done that.

v1-train: 964907cc08b987d5efe094302f20864e92d38a07
v1.0.1: 81cc50326cddeb19bc38adc944ab804252ff1a62

Comment 28

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Playing around with the ones that landed on 1.0.1; it seems ok.

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Forgot to place the build I verified with:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/effe37a77f18
Gaia   bb633c6f2deb829b2f3d5b9e7bac7fa24467d02a
BuildID 20130221070203
Version 18.0
Unagi

Comment 30

[xiaokang.chen]

Hi kevin,
I add some comment on your code. could you have a look? Thank you!

Comment 31

[Kevin Grandon :kgrandon]

Hi, thanks for the note. How did you notice it, was something broken? It appears that the code sets self.status, but isn't really used/referenced. Looks like it was simply performing logic whenever the setting is changed. This certainly could be cleaner though.

Comment 32

[xiaokang.chen]

Fortunatey, everything is OK!
IMO, line 148~164 will never be excuted because the parameter 'value' (as an event object) will always be true. I just want to know what I didn't take into consideration ? Thank you very much!

Comment 33

[Kevin Grandon :kgrandon]

Looks like this was resolved here: https://github.com/mozilla-b2g/gaia/commit/c9d06b2fc64e21d87aa6089098ce71e0d7cb9f56. Thanks!