Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a74a6a
keep the connected hub name if any for flashing firmware dialog
afarago Jul 17, 2024
06d4467
keep the last connected device name in memory
afarago Jul 17, 2024
150b096
change to useLocalStorage and fix the dialog flow to use this as a de…
afarago Jul 18, 2024
77032c6
Merge branch 'pybricks:master' into experiment-keep-hub-name
afarago Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ble/reducers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
// Copyright (c) 2020-2022 The Pybricks Authors
// Copyright (c) 2020-2024 The Pybricks Authors
//
// Manages state for the Bluetooth Low Energy connection.
// This assumes that there is only one global connection to a single device.
Expand Down
22 changes: 18 additions & 4 deletions src/firmware/installPybricksDialog/InstallPybricksDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
// Copyright (c) 2022-2023 The Pybricks Authors
// Copyright (c) 2022-2024 The Pybricks Authors

import './installPybricksDialog.scss';
import {
Expand All @@ -23,7 +23,7 @@ import { ChevronDown, ChevronRight, Error, Heart } from '@blueprintjs/icons';
import { FirmwareMetadata, HubType } from '@pybricks/firmware';
import { fileOpen } from 'browser-fs-access';
import classNames from 'classnames';
import React, { useCallback, useState } from 'react';
import React, { useCallback, useEffect, useState } from 'react';
import { VisuallyHidden } from 'react-aria';
import { useDropzone } from 'react-dropzone';
import { useDispatch } from 'react-redux';
Expand Down Expand Up @@ -383,12 +383,14 @@ const AcceptLicensePanel: React.FunctionComponent<AcceptLicensePanelProps> = ({

type SelectOptionsPanelProps = {
hubName: string;
hubDefaultName: string;
metadata: FirmwareMetadata | undefined;
onChangeHubName(hubName: string): void;
};

const ConfigureOptionsPanel: React.FunctionComponent<SelectOptionsPanelProps> = ({
hubName,
hubDefaultName,
metadata,
onChangeHubName,
}) => {
Expand All @@ -408,7 +410,7 @@ const ConfigureOptionsPanel: React.FunctionComponent<SelectOptionsPanelProps> =
onMouseOver={(e) => e.preventDefault()}
onMouseDown={(e) => e.stopPropagation()}
intent={isHubNameValid ? Intent.NONE : Intent.DANGER}
placeholder="Pybricks Hub"
placeholder={hubDefaultName || 'Pybricks Hub'}
rightElement={
isHubNameValid ? undefined : (
<Icon
Expand Down Expand Up @@ -450,12 +452,17 @@ const BootloaderModePanel: React.FunctionComponent<BootloaderModePanelProps> = (

export const InstallPybricksDialog: React.FunctionComponent = () => {
const { isOpen } = useSelector((s) => s.firmware.installPybricksDialog);
const deviceName = useSelector((s) => s.ble.deviceName);
const inProgress = useSelector(
(s) =>
s.firmware.isFirmwareFlashUsbDfuInProgress ||
s.firmware.isFirmwareRestoreOfficialDfuInProgress,
);
const dispatch = useDispatch();
const [deviceNameLastConnected, setDeviceNameLastConnected] = useLocalStorage(
'setting.lastConnectedDeviceName',
'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make sense to move the default `Pybricks Hub' here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that empty hub name results in the default `Pybricks Hub' displayed / also on firmware side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

);
const [hubName, setHubName] = useState('');
const [licenseAccepted, setLicenseAccepted] = useState(false);
const [hubType] = useHubPickerSelectedHub();
Expand All @@ -472,6 +479,12 @@ export const InstallPybricksDialog: React.FunctionComponent = () => {
? getHubTypeFromMetadata(customFirmwareData?.metadata, hubType)
: hubType;

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (deviceName) {
setDeviceNameLastConnected(deviceName);
}
}, [deviceName, setDeviceNameLastConnected]);

return (
<MultistepDialog
title={i18n.translate('title')}
Expand All @@ -487,7 +500,7 @@ export const InstallPybricksDialog: React.FunctionComponent = () => {
firmwareInstallPybricksDialogAccept(
hubBootloaderType(selectedHubType),
selectedFirmwareData?.firmwareZip ?? new ArrayBuffer(0),
hubName,
hubName || deviceNameLastConnected,
),
),
}}
Expand Down Expand Up @@ -528,6 +541,7 @@ export const InstallPybricksDialog: React.FunctionComponent = () => {
panel={
<ConfigureOptionsPanel
hubName={hubName}
hubDefaultName={deviceNameLastConnected}
metadata={
isCustomFirmwareRequested
? customFirmwareData?.metadata
Expand Down
Loading