1121904 - [Bluetooth][Settings] Show connect/disconnect/unpair dialog for a paired device/headset.

Status: RESOLVED FIXED

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

Description

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

STR:

0. Already paired devices/headset
1. Click paired device
2. Show connect/disconnect/unpair dialog corresponding of device type.

Comment 1

[GitHub Autolander Bot]

Attachment: [PullReq] ian-liu:settings/bluetooth/bug1121904_implement_connect/disconnect_dialog_for_a_paired_device/headset_for_v2 to mozilla-b2g:master

Comment 2

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

Attachment: pull request 28134

Arthur, the patch is implementing the user story of the connection device after the Bluetooth code base changed for supporting API v2. I would like your feedback here. Thanks.

Comment 3

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Please check my comments in github. One general comment would be when adding debugging messages, only display the messages that are meaningful and do help on debugging. It is not required to add messages for every action.

Take an example in this patch, a function A calls to function B and then a gecko API C. Displaying an error message when the API C fails is sufficient for identifying the problem if function A and function B fail only when API C fails.

Comment 4

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

Per offline discussion with Arthur, we will do some architecture changed between 'bluetooth_device' module and 'bt_templete_factory'. It will make the 'bt_templete_factory' more simply.

Comment 5

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Arthur, I have updated the patch with your suggestion. Let's define device description in bluetooth_device module. It's making the template more simply as discussion. I will need your review thanks.

Comment 6

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

Comment on attachment 8562565 [details] [review]
pull request 28134

The major comments are as the following:
- Generate the handlers for each profile based on BluetoothConnectionManager.Profiles instead of hard coding them.
- Cache the connected address and their connected profiles.
- BluetoothConnectionManager.connectedAddress and BluetoothConnectionManager.connectedProfiles only provides the information of the latest connected device, and which leads to workarounds in BluetoothContext. We should use an array providing information of all connected devices.

Let me know if any problems, thanks!

Comment 7

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Arthur, I have updated the patch with your suggestion(comment 6 and offline discussion). I would need your review. Thanks.

The major change in this patch:
- Support cache connected device information. We only do adapter.getConnectedDevices() while init the cache. It improves the performance while the adapter is connecting/disconnecting with a device. It also improves refresh paired devices list. I feel more better than v1 code base. Thanks for your suggestion here:)
- ConnectionManager is not a observable object. We use event listener to notify outer modules.
- Update Bluetooth Context for some duplicated code. Such as: `_findDeviceByAddress'.
- Move updateConnectionInfo(), updateDescriptionText() to be inner function in BluetoothDevice module.

I will add unit test when you think no more change we have to improve in this patch.

Comment 8

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

I file a Bug 1139260 to note the functionality of connection multiple Bluetooth devices.

Comment 9

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Good job! There are only minor issues to be addressed as the following:

- The implementation regarding events could be simplified. Detail please refer to github.
- The access to `this._connectedDevicesInfo` should be handled more carefully. The initialization of `_connectedDevicesInfo` is asynchronous and we are allowed to updated it only when it is initialized. The solution would be making the getter of it return a promise. Any direct access to `this._connectedDevicesInfo` should be avoided. Always use the getter even in `BluetoothConnectionManager`. Should any problems we can discuss it in person.
- We need unit tests for critical functions. :)

Comment 10

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Arthur, I have updated the patch with your suggestion besides of reduced '_findDeviceByAddress'. I left some reason on GitHub. The major change of this patch is converting 'getConnectedDevices' to be a promise call. Will add unit test if you feel well in this patch. Thanks for your reviewing patient here.

Comment 11

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Looks good! Thanks.

There is only one nit. I noticed that there are `getConnectedDevices` and `_getConnectedDevices` defined in the module and it is kind of confusing because `_getConnectedDevices` is actually trying to query the result from gecko, so I would suggest to rename the function.

Comment 12

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Hi Arthur, I update the patch with your suggestion. And fix two issues in some edge cases. Since we have fixed other two issues, I also have a check with rebasing the latest code base. These refinement is needed with your review again. Thanks.

Update:

* Rename _getConnectedDevices to be _getConnectedDevicesWithHFPAndA2DP.
* Fix connected device information error. 
  ** Re-launch settings app while device is connected.
* Fix device description missed to update while ListView is re-render.

Comment 13

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

Comment on attachment 8562565 [details] [review]
pull request 28134

There are a few minor comments but overall it is looking good. Please address the comments and add the unit tests, thanks!

Comment 14

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Updated patch with unit test to cover bluetooth_context, bluetooth_device, bluetooth_connection_manager modules. Arthur, if you think the test is okay, I will clone these change to update in Bluetooth app. Thanks.

Comment 15

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

Comment on attachment 8562565 [details] [review]
pull request 28134

Good work! r=me with the comment addressed. Please remember to update the modules in the bluetooth app, thanks!

Comment 16

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

Thanks for Arthur's reviewing effort with such patient. And update these change in Bluetooth app. Since the patch is landed, we can close the issue now.

Gaia/master:
https://github.com/mozilla-b2g/gaia/commit/4d83e5f453dedbcac5dbe1cb5c889e7fc0987fe9