Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297
Conversation
src/ble/reducers.ts
Outdated
| @@ -84,6 +84,14 @@ const deviceName: Reducer<string> = (state = '', action) => { | |||
| return state; | |||
| }; | |||
|
|
|||
| const deviceNameLastConnected: Reducer<string> = (state = '', action) => { | |||
There was a problem hiding this comment.
I don't think we need a state for this, just a hook that wraps useLocalStorage().
| @@ -527,7 +528,7 @@ export const InstallPybricksDialog: React.FunctionComponent = () => { | |||
| title={i18n.translate('optionsPanel.title')} | |||
| panel={ | |||
| <ConfigureOptionsPanel | |||
| hubName={hubName} | |||
| hubName={hubName || deviceNameLastConnected} | |||
There was a problem hiding this comment.
This doesn't look like it would work correctly. It would only display deviceNameLastConnected but not actually use this when flashing.
Instead, I would use deviceNameLastConnected as the placeholder and then modify the call to firmwareInstallPybricksDialogAccept() to add the appropriate logic.
See other uses of There currently isn't a way to get a unique identifier for each hub, so the best we can do is just store the last name entered and/or the name of the last hub connected. (Maybe there is a way to do this for USB hubs with the serial number, but I think it is more important that the UX is the same for all hubs)
Not sure what you mean by this. It is already written to flash when when firmware is flashed.
We'll also want coverage tests for this. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2297 +/- ##
==========================================
+ Coverage 72.47% 72.49% +0.01%
==========================================
Files 201 201
Lines 5126 5133 +7
Branches 1086 1089 +3
==========================================
+ Hits 3715 3721 +6
- Misses 1374 1375 +1
Partials 37 37 ☔ View full report in Codecov by Sentry. |
Thank you David for the review! The current precedence is as
The upside of this approach is that if user does not enter explicit value, connecting to a different device changes the target name. Coverage tests I am owing, trying to craft the tests running altogether. Yet my WSL it is still giving errors. WIP.
The hub does have a current value, yet as it enters DFU mode we are not aware of it anymore. |
| @@ -472,6 +479,12 @@ export const InstallPybricksDialog: React.FunctionComponent = () => { | |||
| ? getHubTypeFromMetadata(customFirmwareData?.metadata, hubType) | |||
| : hubType; | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
This means that local storage will only be update when this dialog is open, not any time a hub connects. Is that the intended behavior? In other words, the device would have to be connected at the time the dialog is opened.
I think the intended behavior though is any time the a hub is connected. In that case, I would have the saga where we handle device connection write the local storage directly. I.e. just before or after the line:
yield* put(bleDidConnectPybricks(device.id, device.name || ''));in src/ble/sagas.ts.
There was a problem hiding this comment.
This means that local storage will only be update when this dialog is open, not any time a hub connects. Is that the intended behavior? In other words, the device would have to be connected at the time the dialog is opened.
I am not an expert but as i understood all the dialogs are active always, therefore their sagas are in play all the time.
Hence the lines actually record the last connected device even if the dialog is not open or ever opened.
I also double checked this behaviour under the devtools | Application | Storage | local storage
useEffect(() => {
if (deviceName) {
setDeviceNameLastConnected(deviceName);
}
}, [deviceName, setDeviceNameLastConnected]);
You know the intended project structure way better, should I rather move the code under src/ble/sagas.ts?
I am happy to move it, just tell me so.
There was a problem hiding this comment.
Oh yes, I also recall that I was simply unable move any hooks under sagas.
React Hook "useLocalStorage" is called in function "handleBleConnectPybricks" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".eslintreact-hooks/rules-of-hooks
What I was able to do is to directly write to localStorage but that would be quite ugly.
What am I missing?
There was a problem hiding this comment.
I don't think you are missing anything, writing to local storage directly is exactly what I had in mind. Why do you think it is ugly?
| const inProgress = useSelector( | ||
| (s) => | ||
| s.firmware.isFirmwareFlashUsbDfuInProgress || | ||
| s.firmware.isFirmwareRestoreOfficialDfuInProgress, | ||
| ); | ||
| const dispatch = useDispatch(); | ||
| const [deviceNameLastConnected, setDeviceNameLastConnected] = useLocalStorage( | ||
| 'setting.lastConnectedDeviceName', | ||
| '', |
There was a problem hiding this comment.
Could make sense to move the default `Pybricks Hub' here.
There was a problem hiding this comment.
My understanding was that empty hub name results in the default `Pybricks Hub' displayed / also on firmware side.
There was a problem hiding this comment.
I think the existing implementation is assuming that Pybricks Hub is the default name on every hub. If we change it a bit so that we always provide a name instead of using the default one from the firmware, I think that is fine (and perhaps even a bit more future-proof).
Initial implementation for keeping the last connected hub id in memory. pybricks/support/#1415
Show it as the suggested name in the firmware dialog.
Next steps: