feat: Bluetooth handle connection status for knownDevices #17221
Conversation
|
🚀 Expo preview is ready!
|
peter-sanderson
added
the
no-project
szymonlesisz
force-pushed
the
feat/rust-bluetooth
branch
from
bd1861f to
9d12edb
Compare
peter-sanderson
force-pushed
the
bluetooth-suite4
branch
3 times, most recently
from
78738fc to
d94c687
Compare
szymonlesisz
force-pushed
the
feat/rust-bluetooth
branch
from
9d12edb to
8d82c24
Compare
peter-sanderson
force-pushed
the
bluetooth-suite4
branch
3 times, most recently
from
89455d9 to
c227f7d
Compare
szymonlesisz
force-pushed
the
feat/rust-bluetooth
branch
from
8d82c24 to
6929adb
Compare
peter-sanderson
force-pushed
the
bluetooth-suite4
branch
2 times, most recently
from
d2f7759 to
633b19d
Compare
…dux state and implement connect/disconnect
peter-sanderson
force-pushed
the
bluetooth-suite4
branch
from
633b19d to
9a68d09
Compare
… IPC is probably bugged
peter-sanderson
force-pushed
the
bluetooth-suite4
branch
from
b613cda to
ba961f1
Compare
| return true; | ||
| }); | ||
| const devices = allDevices; | ||
| // .filter(it => { |
There was a problem hiding this comment.
This will be uncommented, I need a fix for this from Szymon
|
|
||
| if (!result.success) { | ||
| dispatch( | ||
| bluetoothActions.connectDeviceEventAction({ |
There was a problem hiding this comment.
change of the connection state is now responsibility of the bluetoothIpc and the connection status update will come from it as an update
|
|
||
| return Array.from(map.values()).sort( | ||
| (a, b) => b.device.lastUpdatedTimestamp - a.device.lastUpdatedTimestamp, |
There was a problem hiding this comment.
Sorting is now responsibility of the bluetoothIpc code
There was a problem hiding this comment.
Looks ok, though I cannot test (no BT device), which means I also did not check id design matches figma.
Codewise LGTM with some comments to be addressed.
Overall, I don't understand why the refactoring from deviceState to device with connectionStatus is necessary, though it certainly looks good
| @@ -15,27 +15,15 @@ export const bluetoothConnectDeviceThunk = createThunk< | |||
| `${BLUETOOTH_PREFIX}/bluetoothConnectDeviceThunk`, | |||
| async ({ id }, { fulfillWithValue, dispatch }) => { | |||
| const result = await bluetoothIpc.connectDevice(id); | |||
| console.log('_____calling: bluetoothIpc.connectDevice(id)', id); | |||
There was a problem hiding this comment.
📝 reminder to remove console log.
Also, why remove connectDeviceEventAction? Is it guaranteed that when bluetoothIpc.connectDevice finishes, it will trigger deviceActions.connectDevice (so it'd be useless to call it twice), or how does it work?
| // Devices with 'pairing-error' status should NOT be displayed in the list, as it | ||
| // won't be possible to connect to them ever again. User has to start pairing again, | ||
| // which would produce a device with new id. | ||
| .filter(nearbyDevice => nearbyDevice.connectionStatus?.type !== 'pairing-error') |
There was a problem hiding this comment.
Why do we persist them in redux if they cannot be connected ever again, and have to be paired again to (creating new device object)?
If any device's state becomes 'pairing-error', does it make any sense to keep it in redux for the rest of Suite session?
If not, let's filter them right at the source, in bluetoothReducer: case for bluetoothActions.nearbyDevicesUpdateAction.
Else, how about filtering them in storageActions: saveKnownDevices, so that at least we don't remember them forever?
| }; | ||
|
|
||
| describe('bluetoothSelectors', () => { | ||
| it('selects knownDevices and nearbyDevices in one list fot the UI', () => { | ||
| it('selects knownDevices and nearbyDevices in one list fot the UI, all known devices are', () => { |
| } | ||
| }); | ||
| const nearbyDevicesCopy = [...nearbyDevices]; | ||
| nearbyDevicesCopy.reverse(); |
There was a problem hiding this comment.
why reverse? So that device discovered most recently is displayed first?
Please add comment; it's clear what it does, but not why.
| >( | ||
| `${BLUETOOTH_PREFIX}/bluetoothConnectDeviceThunk`, | ||
| async ({ id }, { fulfillWithValue, dispatch }) => { | ||
| console.log('_____calling: bluetoothIpc.disconnectDevice(id)', id); |
There was a problem hiding this comment.
same as in bluetoothConnectDeviceThunk.ts
|
|
||
| const onDisconnect = async () => { | ||
| await dispatch(bluetoothDisconnectDeviceThunk({ id: device.id })).unwrap(); | ||
| }; |
There was a problem hiding this comment.
this thunk can also result in !result.success right? So onError should be called?
| connected: onDisconnect, | ||
| connecting: undefined, | ||
| disconnected: onConnect, | ||
| paired: undefined, |
There was a problem hiding this comment.
when paired, the Button is neither disabled, nor loading, but doesn't do anything, is that right?
| <Text variant="warning">{secAgo}</Text>s ago | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This could be extracted as a reusable component..
Partially addresses: #15390
Allows for disconnect:
Add cancel for paring mode:
