1128812 - [Statusbar][Bluetooth] Airplanemode and Bluetooth icon not work properly with Bluetooth APIv2

Status: RESOLVED FIXED

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

Description

[[e-mail]]

With new Bluetooth APIv2 gecko patch, the statusbar Airplanemode and Bluetooth icon not work properly

Comment 1

[[e-mail]]

The cause is the event 'bluetooth-adapter-added' and 'bluetooth-disabled' are not triggered anymore when BT is enabled/disable. 

We need to listen to defaultAdapter.state change in v2 instead.

Comment 2

[GitHub Autolander Bot]

Attachment: [PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Comment 3

[[e-mail]]

WIP is now workable on both BT APIv1/v2 devices,

The patch did following work:

* rename `bluetooth-adapter-added` to `bluetooth-enabled` to denote proper event naming
* update settings value and enable bluetooth hardware with new bluetooth v2 module

would do more polish work then ask for the feedback.

Comment 4

[[e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Hi Ian & Alive,

Since Bluetooth v2 API is changed to use 'bluetooth.defaultAdapter' focused API, the quick settings need to be triggered by BTv2 API. Therefore the patch bring bluetooth_v2.js first to provide needed functionality and event to make quick settings works on APIv2. (quick settings was work because of old gecko APIv2 implementation is not correct)

The current implementation tests ok for bluetooth/airplanemode quick settings button on both v1/v2 device.


The next step is try to make new module fits better under bluetooth_core structure.

Comment 5

[[e-mail], [e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Per offline discussion, we will keep to refactor these modules which are relative with mozBluetooth. Once v1 is no longer useful, we should start to make it more modulised. Such as put get default adapter in BluetoothCore. These sub modules won't try to reach it by itself. It's economy to use adapter instance in system app.

I leave some comment on GitHub. Please be careful to add event listener before access default adapter from platform. Beside of the point, feedback+ with me.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Please make BluetoothV2 instantiable unless you have a reason not to do.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #6)
> Comment on attachment 8559010 [details] [review]
> [PullReq] gasolin:issue-1128812 to mozilla-b2g:master
> 
> Please make BluetoothV2 instantiable unless you have a reason not to do.

Also please lazy load bluetooth_v2.js in BTCore; I don't think there is a reason to put v1/v2 pre-loaded together.

Comment 8

[[e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Thanks for feedback. Now it's lazy load and instantiable.

set f? again to collect any concern needs to be addressed

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

f+ with nits

Comment 10

[[e-mail]]

nits fixed, test coverage to 75%. test fine with quick settings bar.

investing the integration test Fail with following steps:

1. switching bt from settings
2. toggle bt and airplane mode via quick settings

expect:

quick settings airplane & bt works

actual:

quick settings airplane work, bt not work

Comment 11

[[e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

The above inter-operation test passed.

The root cause is the origin operation in quick settings depends on 
1. listen to settings stat change, add a tag to make button not pressable
2. receive a bt enable/disable event, remove the tag and allow user press

New BTv2 approach is changed to only listen to mozBluetooth adapter stat.


During final rebase I comment out the statusbar icon related change since bug 1098168 was backed out, and it should not affect this issue. 

(will open another bug to solve v2 status bar icon related issue)


related improvements are document on PR.

Comment 12

[[e-mail]]

made the icon fix once bug 1098168 status icon patch is landed
https://github.com/gasolin/gaia/commit/94f4d0cf4b8364dae7ed902818cc2f811dbb0553

(will open another bug)

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

I took a look and I dislike quickSettings has to know much difference of v1 and v2. What's the plan to improve this?

Comment 14

[[e-mail]]

hmm good question...another approach is make quick settings only reflect to bt event status.

1. Once request-enable/disable-bluetooth event is triggered -> add bluetooth.dataset.initializing flag to disable quick settings operation

2. once 'bluetooth-enabled/disabled' event is received -> remove initializing flag and update button enabled state.

Since we already added those events to btv1, switch to this way does not need change btv1.


PR updated. please kindly check it.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

One nit, please fix or comment if disagree.

Comment 16

[[e-mail]]

Thanks for review. Service.query('Bluetooth.isEnabled') looks good in this case. I'll modify and test with that.

Comment 17

[[e-mail], [e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Fred, I leave some comment on GitHub. Please take a look for suggestion and nits. I would like to review it again. In part of getAdapter() function, it won't work in case of no adapter. 

The major change which I care:

* Promise work to implement getAdapter. 
  ** Per offline discussion, we can do it in other patch. And might considerate to pass default adapter via message. That would be more decoupling module and re-use instance.

* Add/Remove event listener.
* Some of the function/handler name. Please make it more readable.

Thanks for the work. I know it's hard to do for fitting v1/v2.

Comment 18

[[e-mail]]

followup of comment 15. I found quicksettings is initiate before Bluetooth core, so Service.query('Bluetooth.isEnabled') return 'undefined' instead of the correct value. 

Is there a way to make sure Bluetooth core is loaded before quicksettings?

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fred Lin [:gasolin] from comment #18)
> followup of comment 15. I found quicksettings is initiate before Bluetooth
> core, so Service.query('Bluetooth.isEnabled') return 'undefined' instead of
> the correct value. 
> 
> Is there a way to make sure Bluetooth core is loaded before quicksettings?

Please read my comment in github again.

Comment 20

[[e-mail]]

When device is boot up, we still need to set the default state to quick settings button. And for default state bluetooth module will not trigger any event. I decide to remove settings and Service.query but dispatch a enable/disable event in Bluetooth at it's init time, so quick settings can only deal with events.

> Alive: Use Service.query('Bluetooth.isEnabled') and delete this.bluetooth.dataset.initializing only when the value is not undefined - because the remaining event part is also relying on bluetooth(v1/v2).js to tell you it's enabled or not.

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fred Lin [:gasolin] from comment #20)
> When device is boot up, we still need to set the default state to quick
> settings button. And for default state bluetooth module will not trigger any
> event. I decide to remove settings and Service.query but dispatch a
> enable/disable event in Bluetooth at it's init time, so quick settings can
> only deal with events.

Please do. It makes more sense than observing bluetooth.enabled settings everywhere in the system app.

> 
> > Alive: Use Service.query('Bluetooth.isEnabled') and delete this.bluetooth.dataset.initializing only when the value is not undefined - because the remaining event part is also relying on bluetooth(v1/v2).js to tell you it's enabled or not.

Comment 22

[[e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

@Ian as discussed offline, I change the getAdapter to cache the promise that returned for callers, to avoid multi resolve in defaultAdapter call. And add test cases to make sure cached promise works as we expected. Current test coverage is 85%

I also add BluetoothAdapter change handler so all eventListener could be removed.

Please kindly review it again.

Comment 23

[[e-mail], [e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

I'm still in the reviewing work for the patch. Then, I will give some suggestion later. And I would like to nominate Arthur to be reviewer too. He is familiar with the new Bluetooth v2 API since we have finished basic functionality in Settings app.

In part of system app, we have to be careful to expose Bluetooth module for enable/disable Bluetooth. Especially, System app is the first road to turn Bluetooth on. We are wondering no default adapter while the platform is booting..

Comment 24

[[e-mail], [e-mail]]

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Fred, thanks for your updated for this patch soon. I still leave some comment on GitHub.

Per schedule issue, optional:

In part of cache promise 'getAdapter', I have a idea to make it more readable without modules dependancy. Since System app have BaseModule prototype, it is a way to inherit it for construct Bluetooth module here. It will support us to publish event to let outer modules knows that the default adapter is ready or not. One the outer module receives the specific publish event, it can catch the event.detail to get the default adapter. In this way, we can give up to cache promise for 'getAdapter' in bluetooth_v2.js.

I know it is a big effort to implement it. But it's more readable in the feature. And we can make sure one default adapter that we use and listen in System app. The default adapter is managed and notified from this Bluetooth module. 

Besides, we also have way to separate many functionality(adapter management, on/off, icon status) in Bluetooth module currently. Because an outer module is easy to get default adapter from published event.

Comment 25

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

Comment on attachment 8559010 [details] [review]
[PullReq] gasolin:issue-1128812 to mozilla-b2g:master

Please check my comments in github. A major comment is that we should make the functions related to using an adapter event based. The reason is that the starting point of the whole thing is gecko instead of gaia. It would be easier to use event handlers instead of trying to wrap things in async functions. Let me know if any problems, thanks.

Comment 26

[[e-mail]]

Sounds like a good plan, I'd remove the getAdapter and send window.event with adapter instead.
So the cached promise and multiple listeners are removed, the code will be cleaner.

Comment 27

[[e-mail]]

Attachment: pull request redirect to github

@ian & @arthur I've removed the getadapter function and use event dispatch to carry the adapter info to other system part.
Test on device ok. Please give some feedback if the patch better served our goal.

Comment 28

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

Comment on attachment 8571168 [details] [review]
pull request redirect to github

We should not assume BluetoothAdapter is always ready when loading the module. Please initialize the module based on the current adapter (no matter it is ready or not) and respond to adapter changes.

Comment 29

[[e-mail], [e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

Per offline discussion, bluetooth module also needs to init in case of no default adapter. It would need to fire 'disabled' event while the module is ready to service even if there is no default adapter, I think. Thanks.

Comment 30

[[e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

Thanks for feedback. I've revised the call flow based on out offline discussion, please check it again, thanks!

Comment 31

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

Fred, it seems conflict to the current master. Could you help resolve it? Thanks.

Comment 32

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

Comment on attachment 8571168 [details] [review]
pull request redirect to github

Please check my comments in github, thanks.

Comment 33

[[e-mail], [e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

I put a little bit comment on GitHub. Be careful the naming you used in Bluetooth v2 module. Please given explicit function name for what the event we are listening.

Such as: _registerListeners() in start() function. I cannot understand it via function name immediately. Especially, many events be listened in start() function.

Comment 34

[[e-mail]]

Thanks for feedback. The statusbar icon bug 1098168 is landed yesterday, I'd rebase and fix the conflict

Comment 35

[[e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

Rebased with Statusbar icon fix, comment addressed, quick settings works on v1/v2 branch.

Please kindly check it again :)

Comment 36

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

Comment on attachment 8571168 [details] [review]
pull request redirect to github

f+ with the two nits addressed, thanks.

Comment 37

[[e-mail], [e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

Looks good! f+ with me.

Comment 38

[[e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

@Ian & @Arthur Thanks for feedback!
The patch has been test with 83% unittest coverage and manual test on BT APIv1/v2 devices.

@alive since the new statusbar icon is landed, I rebased and added BTv2 icon related test cases to new PR.

please help review it.

Comment 39

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

Comment on attachment 8571168 [details] [review]
pull request redirect to github

There is one last comment to be addressed from my side before merging. I just found that we can refine the way of updating the bluetooth toggle as we are adding new events to the bluetooth module in this patch.

The idea is that the state of the bluetooth toggle can be determined more robustly by considering all of the states reported by gecko. Detail please check my comments in github, thanks!

Comment 40

[[e-mail]]

I've replied in github that the reason not to emit bt state by event is to deal with backward compatibility with BTv1, which does not have "enabling", "disabling" state. Current approach help us keep BTv1 with less change as well.

Though I think it's possible to mimic the "enabling", "disabling" state by emit events at the time BTv1 try to change mozsettings value. Make sense?

Comment 41

[[e-mail]]

another reason is the quick settings `initializing` flag is bound to button press-able state. If we bind BT enabling/disabling state with quick setting, user press quickly on quick settings button might cause unexpected behavior.

Comment 42

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

Comment on attachment 8571168 [details] [review]
pull request redirect to github

hmm, considering the backward compatibility, let's do this once we move to v2 completely. r=me.

Comment 43

[[e-mail], [e-mail]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

There is still one race condition potentially. I leave some comment on GitHub. Please do '_initDefaultAdapter()' after listened 'attributechanged' from BluetoothManager. 

Beside of problem, r+ with me. Thanks for your effort.

Comment 44

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8571168 [details] [review]
pull request redirect to github

1. Don't listen to the event fired by yourself unless you have a good reason (for instance, the event dispatcher will be extracted to another module in the future.)
2. Fix the bluetooth icon test failure.

Comment 45

[[e-mail]]

Thanks for address that, I've changed to execute the handler then emit the event and fixed the icon test issue. Will land until the order of initDefaultAdapter debate is settled and treeherder green.

Comment 46

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/a307d387b52f98f8d79247cf3141123533b6c6a7

thanks!

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=7c94a828bff9 The calendar multi-days test fail is permanent for recent treeherder build, should not block the landing.