[DSDS] SIM manager(when PIN locked or no SIM), Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.

VERIFIED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::Settings
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Enpei, Assigned: jaoo)

Tracking

unspecified
1.3 C2/1.4 S2(17jan)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [dsds_US_test])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
SIM manager, Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.

* Build Number                
Gaia:     f64d282b6b44eaaf8cddc8be8f3250cb6fbacd95
Gecko:    0e3362fb5625eb6d98c7617b1b3019a2cc553d47
BuildID   20131202094818
Version   28.0a1

* Reproduce Steps
Enable airplane mode.

* Expected Result
Unable to access SIM manager, Call Settings, Message Settings and Cellular & Data.

* Actual Result
User can still access SIM manager, Call Settings, Message Settings and Cellular & Data.

* Occurrence rate
100%
(Reporter)

Updated

5 years ago
blocking-b2g: --- → 1.3?
Whiteboard: [dsds_US_test]
(Reporter)

Comment 1

5 years ago
Correct my expected result to follow UX spec.

* Expected Result
1. Unable to access Call Settings, Message Settings and Cellular & Data.
2. If PIN is locked or no SIM cards inserted in device, SIM manager cannot be access as well under airplane mode.
Summary: [DSDS] SIM manager, Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on. → [DSDS] SIM manager(when PIN locked or no SIM), Call Settings, Message Settings and Cellular & Data are still accessible when airplane mode is on.
(Assignee)

Comment 2

5 years ago
Call Settings and Cellular & Data wont be accessible when airplane mode is on. I'll take care of it on bugs 928297 and 928294.
Jose, note that the card state remains the same when user enabled airplane mode on the fugu device. Please use mozMobileConnection.radioState and the radiostatechanged event instead of checking the card state.
Flags: needinfo?(josea.olivera)
(Assignee)

Comment 4

5 years ago
(In reply to Arthur Chen [:arthurcc] from comment #3)
> Jose, note that the card state remains the same when user enabled airplane
> mode on the fugu device. Please use mozMobileConnection.radioState and the
> radiostatechanged event instead of checking the card state.

Oh, thanks Arthur!
Flags: needinfo?(josea.olivera)
(Reporter)

Updated

5 years ago
Blocks: 926342
Triage: user confusion.

Update assignee based on comment 2.
Assignee: nobody → josea.olivera
blocking-b2g: 1.3? → 1.3+
(Assignee)

Comment 6

5 years ago
Created attachment 8344627 [details] [diff] [review]
v1

As I said in comment #2 I would take care of Call Settings and Cellular & Data items in bugs 928297 and 928294 so we only need to take care here of disabling the SIM manager in case of SIM absent or airplane mode.

Arthur, would you review this patch please? Thanks.

PS. This bug would depend -at least- on bug 928297 per comment above.
Attachment #8344627 - Flags: review?(arthur.chen)
(Assignee)

Updated

5 years ago
Depends on: 928297
(Assignee)

Comment 7

5 years ago
Comment on attachment 8344627 [details] [diff] [review]
v1

After an off-line discussion with Arthur we agreed that we also should disable all the first level menu items in case of airplane mode and ICC card absent. That means we should disable 'Call Setting' and 'Cellular & Data' menu items as well.
Attachment #8344627 - Flags: review?(arthur.chen)
(Reporter)

Updated

5 years ago
No longer blocks: 926342
See Also: → bug 926342
(Assignee)

Comment 9

5 years ago
Created attachment 8348793 [details] [diff] [review]
v2

Implements what I commented on comment #7. I'll request review after performing some tests on the real devices.
Attachment #8344627 - Attachment is obsolete: true
Created attachment 8349507 [details] [diff] [review]
v3

Arthur, could you take a look please?

This patch implements what I commented on comment #7. Tested on both Peak and fugu devices. Seems to work correctly.
Attachment #8348793 - Attachment is obsolete: true
Attachment #8349507 - Flags: review?(arthur.chen)
Comment on attachment 8349507 [details] [diff] [review]
v3

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

Jose, thanks for the patch. 

I had some tests and found that if we enable airplane mode from quick settings or action menu (long press the power button), the states of the items are incorrect. And there are more and more similar logic dealing with card state change and radio state change (in settings.js and connectivity.js), which makes the code difficult to trace and maintain. Given that we are going to refactor airplane mode related logic in bug 948847, I would like to do refactor this part based on it. I'm okay to work on this part as you are going to have PTO soon. Please let me know what you think, thanks!
Attachment #8349507 - Flags: review?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] (PTO 12/25 ~ 1/5) from comment #11)
> Comment on attachment 8349507 [details] [diff] [review]
> v3
> 
> Review of attachment 8349507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jose, thanks for the patch.

Thanks for the review.

> I had some tests and found that if we enable airplane mode from quick
> settings or action menu (long press the power button), the states of the
> items are incorrect. And there are more and more similar logic dealing with
> card state change and radio state change (in settings.js and
> connectivity.js), which makes the code difficult to trace and maintain.

Yeah, I was tempted to unify the logic from both files but I didn't do it to be fast fixing the bug.

> Given that we are going to refactor airplane mode related logic in bug
> 948847, I would like to do refactor this part based on it. I'm okay to work
> on this part as you are going to have PTO soon. Please let me know what you
> think, thanks!

I'm ok with having a helper for dealing with radio and card states It will be useful, indeed. So lets pause a bit here and wait for the helper.

Thanks Arthur!
(In reply to Arthur Chen [:arthurcc] (PTO 12/25 ~ 1/5) from comment #11)
> Comment on attachment 8349507 [details] [diff] [review]

> I had some tests and found that if we enable airplane mode from quick
> settings or action menu (long press the power button), the states of the
> items are incorrect.

FYI I've retested those cases and everything works fine.
Arthur, after taking a look at all the pieces I have ended up with a plan for this bug. The plan is to add the helper and to unify the code from settings.js and connectivity files in settings.js file. I'll work on top of EJ's patches that way it will be faster since there won't be need to wait for the helper to be landed. I hope you will be ok with it.
A helper sounds good to me. We also need a base object that can handle the listeners on mozIcc objects so that we don't need to add or remove the listeners manually when the mozIcc objects become invalid. Let's discuss more when you start working on it.

Updated

4 years ago
Depends on: 948847
Created attachment 8357201 [details] [diff] [review]
v4

Arthur, this is the new approach so far. I've added a new singleton object that handles the menu items related to telephony features. Next would be to use the airplane mode helper being implemented by EJ. Some feedback from you would be nice.
Attachment #8357201 - Flags: feedback?(arthur.chen)
Comment on attachment 8357201 [details] [diff] [review]
v4

Thanks for the work, Jose.

It is much cleaner for settings.js and connectivity.js now. Can we further extract the logic of handling sim card state to another object? So that we can reuse the object for all items that need to reflect the sim card states.
Attachment #8357201 - Flags: feedback?(arthur.chen)
Created attachment 8358475 [details] [diff] [review]
v5

New version (v5) of the patch (PR updated also).

(In reply to Arthur Chen [:arthurcc] from comment #18)
> Comment on attachment 8357201 [details] [diff] [review]
> v4

> It is much cleaner for settings.js and connectivity.js now. Can we further
> extract the logic of handling sim card state to another object? So that we
> can reuse the object for all items that need to reflect the sim card states.

Sure, v5 patch addresses the comment above. Once EJ finishes with the airplane mode helper I'll add it to the new object that handles ICC card states and request review at you again.

Thanks!
Attachment #8357201 - Attachment is obsolete: true
https://github.com/mozilla-b2g/gaia/pull/15117 PR updated. Now we make use of the AirplaneModeHelper object implemented by EJ. I'll request review for this patch once EJ lands his work.
(Assignee)

Updated

4 years ago
Attachment #8358475 - Attachment is obsolete: true
Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Arthur, EJ's work just landed. Would you review this now please? Thanks!
Attachment #8357198 - Flags: review?(arthur.chen)
Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

I still think there is duplicate logic that should be included in the reusable object, for example, event handling things in call_iccs.js and carrier_iccs.js.

However, as we need to fix all 1.3+ bugs by the end of this week and there is a potential risk of landing such a big patch, is it possible to have a simple fix for now and create another bug for refactoring?
Attachment #8357198 - Flags: review?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #22)
> Comment on attachment 8357198 [details] [review]
> Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Thanks for the review Arthur.

> I still think there is duplicate logic that should be included in the
> reusable object, for example, event handling things in call_iccs.js and
> carrier_iccs.js.
> 
> However, as we need to fix all 1.3+ bugs by the end of this week and there
> is a potential risk of landing such a big patch, is it possible to have a
> simple fix for now and create another bug for refactoring?

Talked to Arthur on IRC. The thing here is that this bug is 1.3+ and the refactor could end up with a big patch and it could be risky landing it. The plan would be to land this patch as it is (after more test performed by Arthur) and to work on the refactor after code freeze.
Comment on attachment 8357198 [details] [review]
Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/15117

Per an offline discussion, let's land this patch as the code structure is better for the refactor in the future. r=me with the nits addressed. Thanks!
Attachment #8357198 - Flags: review+
Thanks Arthur, I've tested the path several times in both peak and fugu devices. Also I've run several times Travis. Travis's green and everything seems to work correctly.

Landed Gaia master branch at:
https://github.com/mozilla-b2g/gaia/commit/cc28d9cf5214cbefb67171551ce2c367f7f80a7f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
status-b2g-v1.3: --- → affected
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 cc28d9cf5214cbefb67171551ce2c367f7f80a7f
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(josea.olivera)
Landed on Gaia v1.3 branch at https://github.com/mozilla-b2g/gaia/commit/7443da6780336eac68d4228834372e16393d10d8
status-b2g-v1.3: affected → fixed
Flags: needinfo?(josea.olivera)
(Reporter)

Comment 28

4 years ago
Verified on fugu.

Gaia      ee25b0e45649d2955f340ce4f71ad55712dd0fab
Gecko     913cf2b92845441c9578787362ddad6f2ac15df6
BuildID   20140121095108
Version   28.0a2
Status: RESOLVED → VERIFIED
status-b2g-v1.3: fixed → verified
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
Created attachment 8407252 [details]
GrayOut.png

Works on v1.4 Fugu with airplane mode on. 

GAIA_REV=441c4bcd8ac4f8c01a9bc5a2f8d64eaa87844803
GECKO_REV=0c18beacb37a7e1e80bcf6d46cc8ff9a18c0c653
GAIA_BRANCH=mozillaorg/v1.4
GECKO_BRANCH=mozillaorg/v1.4
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-25
BuildID=20140416041552
You need to log in before you can comment on or make changes to this bug.