1145055 - [Settings] Extract out AsyncOperator and CallForwarding

Status: RESOLVED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

In this patch, we have to move out AsyncOperator and CallForwarding related functions to make sure we can make call.js cleaner and easy to refactor later.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Arthur, can you help me check this patch ? I'll add tests later if this approach is on the right direction. Thanks :)

Note: I will keep trying to fetch codes out as standalone modules to make sure we can easily refactor call.js in the end.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

We need an instance of CallForwarding module (similar to call_barring.js) for each sim that provides methods for setting call forwarding, reports the current settings and status via events or observables. `panels/call/panel.js` then binds the UI element to the module.

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Yes I know, I just try to move components out one by one slowly, if for this patch, it would be also better to move out the whole CallForwarding module, I will try how to do that later. Thanks !

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

1st stage feedback !

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arthur, in the end, I directly refactor the whole call panel here in this patch. haha

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Let's focus on the CallForwarding module first.

The module supports the call forwarding panel. It should provide information that can be used to compose the text displayed on the panel. These information include:
- the setting value of each type of call forwarding
- the current querying state

And then we need to identify what method the module should provide and how the method change the value of the information. In this case:
- set call forwarding settings
- query the current settings

Let me know should any questions, thanks!

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arthur, can you help me check this updated patch ?

In this patch, I removed all events and also make sure all related logics were hidden in its own panels. For now, the codebase looks better and robust than before now, I think there are still some stuffs that can be fine tuned here, just want to have your feedback first and see what to update later.

Thanks !!

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

After reviewing what we've discussed earlier, I realized that what we need is a call settings specific module that schedules requests.

All call settings related modules are built on top of this scheduling module. Those modules would have method for get/set settings, and observable properties reporting state required for updating the UI. For example, we need to know whether gecko is occupied so that we can disable the UI making requests. The current querying status is also required for displaying sub titles like "requesting call forwarding info...".

I believe with theses fundamental modules implemented composing the call settings panels would become bery easy.

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Arthur, I did update this patch based on your comments, can you give me any feedback on current status ? BTW, I improved some codes to make it more robust.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Basically f+ except for `CallForwarding` and 'call_forwarding/panel.js` for call forwarding. I would expect `CallForwarding` actively reporting the current settings and querying status as suggested in comment 7.

Regarding the dependency between `CallForwarding`and `CallSettingsTaskScheduler`, `CallForwarding` is the one wrapping tasks and put them into the task scheduler, not panel.js. Task scheduler is considered as a underlying scheduling mechanism and should be transparent to UI. The UI only knows that it issues an async call but has no idea on what may block the call. Because the patch is focusing on call forwarding, I'm fine with enqueuing tasks in `call_settings/panel.js`. In the long term I would expect a separate module wrapping tasks for each type of call setting.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Note !

In Bug 1155590, we add one more check in call settings. Remember to add that part back in this patch.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Arthur, I think this patch is ready to be reviewed now ! Please help me do a final check before landing, thank :)

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

EJ, I added my comments in github. The short version is that we should remove the logic regarding settings db and make the call forwarding module easier to be used from UI. And make sure we have a separate call forwarding instance for each sim card.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

Arthur, I just updated the patch and right now CallForwarding is a instanceable class that we will create as much as CF instances if needed based on how many cards we have.

And also, with Observable, we can directly observe changes from call_forwarding/panel.js and directly reflect information on UI layer.

So I think this patch is good enough to get reviewed if you have time, thanks !

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

I believe this is the last round and should be quick. We can open follow-up bugs for other improvements. Please check my comments in github, thanks!!

Comment 17

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

This patch looooooks nice now :P, any idea for this patch, Arthur ?

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8579902 [details] [review]
[gaia] EragonJ:bug-1145055 > mozilla-b2g:master

r=me, thank you!!

Comment 19

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#y1Nphw4PTRawfMd4grEJVg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, this patch was landed at Gaia/master : 

https://github.com/mozilla-b2g/gaia/commit/385e7b9be7d7f8c5564bfa9a305c9e92be2a5e25