From 861ea34d8638c45dbd0dbf0ea93a10af8580bec0 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Tue, 15 Mar 2022 16:25:43 -0500 Subject: [PATCH] settings: convert flashCurrentProgram to react hook --- src/firmware/FlashButton.tsx | 10 +++- src/firmware/actions.ts | 13 ++++-- src/firmware/sagas.test.ts | 63 ++++++++++++++++--------- src/firmware/sagas.ts | 6 +-- src/settings/SettingsDrawer.test.tsx | 16 +++++++ src/settings/SettingsDrawer.tsx | 18 ++++---- src/settings/actions.ts | 31 +------------ src/settings/defaults.ts | 14 ------ src/settings/hooks.ts | 10 +++- src/settings/reducers.test.ts | 16 +------ src/settings/reducers.ts | 24 +--------- src/settings/sagas.test.ts | 27 +---------- src/settings/sagas.ts | 69 +--------------------------- 13 files changed, 100 insertions(+), 217 deletions(-) diff --git a/src/firmware/FlashButton.tsx b/src/firmware/FlashButton.tsx index 2deae414..fa04db94 100644 --- a/src/firmware/FlashButton.tsx +++ b/src/firmware/FlashButton.tsx @@ -7,6 +7,7 @@ import { BleConnectionState } from '../ble/reducers'; import { BootloaderConnectionState } from '../lwp3-bootloader/reducers'; import * as notificationActions from '../notifications/actions'; import { useSelector } from '../reducers'; +import { useSettingFlashCurrentProgram } from '../settings/hooks'; import OpenFileButton, { OpenFileButtonProps } from '../toolbar/OpenFileButton'; import { TooltipId } from '../toolbar/i18n'; import { flashFirmware } from './actions'; @@ -19,6 +20,7 @@ const FlashButton: React.VoidFunctionComponent = ({ id }) => { const bleConnection = useSelector((s) => s.ble.connection); const flashing = useSelector((s) => s.firmware.flashing); const progress = useSelector((s) => s.firmware.progress); + const [isSettingFlashCurrentProgramEnabled] = useSettingFlashCurrentProgram(); const dispatch = useDispatch(); @@ -34,7 +36,9 @@ const FlashButton: React.VoidFunctionComponent = ({ id }) => { } showProgress={flashing} progress={progress === null ? undefined : progress} - onFile={(data) => dispatch(flashFirmware(data))} + onFile={(data) => + dispatch(flashFirmware(data, isSettingFlashCurrentProgramEnabled)) + } onReject={(file) => dispatch( notificationActions.add( @@ -43,7 +47,9 @@ const FlashButton: React.VoidFunctionComponent = ({ id }) => { ), ) } - onClick={() => dispatch(flashFirmware(null))} + onClick={() => + dispatch(flashFirmware(null, isSettingFlashCurrentProgramEnabled)) + } /> ); }; diff --git a/src/firmware/actions.ts b/src/firmware/actions.ts index eaebea53..f12ac9b3 100644 --- a/src/firmware/actions.ts +++ b/src/firmware/actions.ts @@ -120,11 +120,16 @@ export type FailToFinishReason = /** * Creates a new action to flash firmware to a hub. * @param data The firmware zip file data or `null` to get firmware later. + * @param flashCurrentProgram If true, flash the current program from the editor, + * otherwise use the program from firmware.zip. */ -export const flashFirmware = createAction((data: ArrayBuffer | null) => ({ - type: 'flashFirmware.action.flashFirmware', - data, -})); +export const flashFirmware = createAction( + (data: ArrayBuffer | null, flashCurrentProgram: boolean) => ({ + type: 'flashFirmware.action.flashFirmware', + data, + flashCurrentProgram, + }), +); /** * Action that indicates flashing firmware started. diff --git a/src/firmware/sagas.test.ts b/src/firmware/sagas.test.ts index f84522af..e2de32bc 100644 --- a/src/firmware/sagas.test.ts +++ b/src/firmware/sagas.test.ts @@ -86,7 +86,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -231,7 +231,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -280,7 +280,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -347,7 +347,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -410,7 +410,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -476,7 +476,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -535,7 +535,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -600,7 +600,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -681,7 +681,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -746,7 +746,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -840,7 +840,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -943,7 +943,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -1086,7 +1086,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader @@ -1228,7 +1228,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // the first step is to compile main.py to .mpy @@ -1372,7 +1375,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // should get failure due to missing file @@ -1415,7 +1421,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // should get failure due to unsupported mpy-cross version @@ -1457,7 +1466,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // the first step is to compile main.py to .mpy @@ -1512,7 +1524,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // the first step is to compile main.py to .mpy @@ -1568,7 +1583,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // the first step is to compile main.py to .mpy @@ -1627,7 +1645,10 @@ describe('flashFirmware', () => { // saga is triggered by this action saga.put( - flashFirmwareAction(await zip.generateAsync({ type: 'arraybuffer' })), + flashFirmwareAction( + await zip.generateAsync({ type: 'arraybuffer' }), + false, + ), ); // the first step is to compile main.py to .mpy @@ -1715,7 +1736,7 @@ describe('flashFirmware', () => { // saga is triggered by this action - saga.put(flashFirmwareAction(null)); + saga.put(flashFirmwareAction(null, false)); // first step is to connect to the hub bootloader diff --git a/src/firmware/sagas.ts b/src/firmware/sagas.ts index 568f4a2c..d786757c 100644 --- a/src/firmware/sagas.ts +++ b/src/firmware/sagas.ts @@ -286,11 +286,7 @@ function* handleFlashFirmware(action: ReturnType): Generat let program: string | undefined = undefined; - const flashCurrentProgram = yield* select( - (s: RootState) => s.settings.flashCurrentProgram, - ); - - if (flashCurrentProgram) { + if (action.flashCurrentProgram) { const editor = yield* getContext('editor'); // istanbul ignore if: it is a bug to dispatch this action with no current editor diff --git a/src/settings/SettingsDrawer.test.tsx b/src/settings/SettingsDrawer.test.tsx index de017420..27921f6f 100644 --- a/src/settings/SettingsDrawer.test.tsx +++ b/src/settings/SettingsDrawer.test.tsx @@ -39,6 +39,22 @@ describe('showDocs setting switch', () => { }); }); +describe('flashCurrentProgram setting switch', () => { + it('should toggle the setting', () => { + const [settings] = testRender( + undefined} />, + ); + + expect(localStorage.getItem('setting.flashCurrentProgram')).toBe(null); + + settings.getByLabelText('Include current program').click(); + expect(localStorage.getItem('setting.flashCurrentProgram')).toBe('true'); + + settings.getByLabelText('Include current program').click(); + expect(localStorage.getItem('setting.flashCurrentProgram')).toBe('false'); + }); +}); + describe('about dialog', () => { it('should open the dialog when the button is clicked', async () => { const [settings] = testRender( diff --git a/src/settings/SettingsDrawer.tsx b/src/settings/SettingsDrawer.tsx index 9cb24c07..c064c903 100644 --- a/src/settings/SettingsDrawer.tsx +++ b/src/settings/SettingsDrawer.tsx @@ -35,9 +35,9 @@ import { pseudolocalize } from '../i18n'; import { useSelector } from '../reducers'; import ExternalLinkIcon from '../utils/ExternalLinkIcon'; import { isMacOS } from '../utils/os'; -import { setBoolean, setString } from './actions'; -import { BooleanSettingId, StringSettingId } from './defaults'; -import { useSettingIsShowDocsEnabled } from './hooks'; +import { setString } from './actions'; +import { StringSettingId } from './defaults'; +import { useSettingFlashCurrentProgram, useSettingIsShowDocsEnabled } from './hooks'; import { SettingsStringId } from './i18n'; import en from './i18n.en.json'; import './settings.scss'; @@ -59,7 +59,8 @@ const SettingsDrawer: React.VoidFunctionComponent = ({ const [isAboutDialogOpen, setIsAboutDialogOpen] = useState(false); const { isDarkMode, toggle: toggleDarkMode } = useDarkMode(); - const flashCurrentProgram = useSelector((s) => s.settings.flashCurrentProgram); + const [isFlashCurrentProgramEnabled, setIsFlashCurrentProgramEnabled] = + useSettingFlashCurrentProgram(); const isServiceWorkerRegistered = useSelector( (s) => s.app.isServiceWorkerRegistered, ); @@ -169,13 +170,10 @@ const SettingsDrawer: React.VoidFunctionComponent = ({ label={i18n.translate( SettingsStringId.FirmwareCurrentProgramLabel, )} - checked={flashCurrentProgram} + checked={isFlashCurrentProgramEnabled} onChange={(e) => - dispatch( - setBoolean( - BooleanSettingId.FlashCurrentProgram, - (e.target as HTMLInputElement).checked, - ), + setIsFlashCurrentProgramEnabled( + (e.target as HTMLInputElement).checked, ) } /> diff --git a/src/settings/actions.ts b/src/settings/actions.ts index 5228cc66..bdadd76b 100644 --- a/src/settings/actions.ts +++ b/src/settings/actions.ts @@ -2,36 +2,7 @@ // Copyright (c) 2021-2022 The Pybricks Authors import { createAction } from '../actions'; -import { BooleanSettingId, StringSettingId } from './defaults'; - -/** Creates an action to set/store a setting. */ -export const setBoolean = createAction((id: BooleanSettingId, newState: boolean) => ({ - type: 'settings.action.setBoolean', - id, - newState, -})); - -/** Creates an action to toggle a setting. */ -export const toggleBoolean = createAction((id: BooleanSettingId) => ({ - type: 'settings.action.toggleBoolean', - id, -})); - -/** Creates an action indicating that setting/storing a setting failed. */ -export const didFailToSetBoolean = createAction((id: BooleanSettingId, err: Error) => ({ - type: 'settings.action.didFailToSetBoolean', - id, - err, -})); - -/** Creates an action that indicates a stored boolean setting value changed. */ -export const didBooleanChange = createAction( - (id: BooleanSettingId, newState: boolean) => ({ - type: 'settings.action.didBooleanChange', - id, - newState, - }), -); +import { StringSettingId } from './defaults'; /** Creates an action to set/store a setting. */ export const setString = createAction((id: StringSettingId, newState: string) => ({ diff --git a/src/settings/defaults.ts b/src/settings/defaults.ts index 1d353ba6..cf352d71 100644 --- a/src/settings/defaults.ts +++ b/src/settings/defaults.ts @@ -3,20 +3,6 @@ // Definitions for user selectable settings. -export enum BooleanSettingId { - FlashCurrentProgram = 'flashCurrentProgram', -} - -export function getDefaultBooleanValue(id: BooleanSettingId): boolean { - switch (id) { - case BooleanSettingId.FlashCurrentProgram: - return false; - // istanbul ignore next: it is a programmer error if we hit this - default: - throw Error(`Bad BooleanSettingId: ${id}`); - } -} - export enum StringSettingId { HubName = 'hubName', } diff --git a/src/settings/hooks.ts b/src/settings/hooks.ts index ea7521f3..53ef9fc4 100644 --- a/src/settings/hooks.ts +++ b/src/settings/hooks.ts @@ -1,9 +1,12 @@ // SPDX-License-Identifier: MIT // Copyright (c) 2022 The Pybricks Authors -import { useCallback } from 'react'; +import { Dispatch, SetStateAction, useCallback } from 'react'; import { useLocalStorage } from 'usehooks-ts'; +// this is private type from usehooks-ts +type SetValue = Dispatch>; + /** Hook for "showDocs" setting. */ export function useSettingIsShowDocsEnabled(): { isSettingShowDocsEnabled: boolean; @@ -26,3 +29,8 @@ export function useSettingIsShowDocsEnabled(): { toggleIsSettingShowDocsEnabled, }; } + +/** Hook for "flashCurrentProgram" setting. */ +export function useSettingFlashCurrentProgram(): [boolean, SetValue] { + return useLocalStorage('setting.flashCurrentProgram', false); +} diff --git a/src/settings/reducers.test.ts b/src/settings/reducers.test.ts index 4201464e..2c9fd972 100644 --- a/src/settings/reducers.test.ts +++ b/src/settings/reducers.test.ts @@ -2,8 +2,8 @@ // Copyright (c) 2021-2022 The Pybricks Authors import { AnyAction } from 'redux'; -import { didBooleanChange, didStringChange } from './actions'; -import { BooleanSettingId, StringSettingId } from './defaults'; +import { didStringChange } from './actions'; +import { StringSettingId } from './defaults'; import reducers from './reducers'; type State = ReturnType; @@ -11,24 +11,12 @@ type State = ReturnType; test('initial state', () => { expect(reducers(undefined, {} as AnyAction)).toMatchInlineSnapshot(` Object { - "flashCurrentProgram": false, "hubName": "", "isHubNameValid": true, } `); }); -describe('flashCurrentProgram', () => { - test('setting changed', () => { - expect( - reducers( - { flashCurrentProgram: false } as State, - didBooleanChange(BooleanSettingId.FlashCurrentProgram, true), - ).flashCurrentProgram, - ).toBe(true); - }); -}); - describe('hubName', () => { const testName = 'test name'; test('setting changed', () => { diff --git a/src/settings/reducers.ts b/src/settings/reducers.ts index 44a8fc3a..46be2ab8 100644 --- a/src/settings/reducers.ts +++ b/src/settings/reducers.ts @@ -2,30 +2,11 @@ // Copyright (c) 2021-2022 The Pybricks Authors import { Reducer, combineReducers } from 'redux'; -import { didBooleanChange, didStringChange } from './actions'; -import { - BooleanSettingId, - StringSettingId, - getDefaultBooleanValue, - getDefaultStringValue, -} from './defaults'; +import { didStringChange } from './actions'; +import { StringSettingId, getDefaultStringValue } from './defaults'; const encoder = new TextEncoder(); -const flashCurrentProgram: Reducer = ( - state = getDefaultBooleanValue(BooleanSettingId.FlashCurrentProgram), - action, -) => { - if (didBooleanChange.matches(action)) { - if (action.id === BooleanSettingId.FlashCurrentProgram) { - return action.newState; - } - return state; - } - - return state; -}; - const hubName: Reducer = ( state = getDefaultStringValue(StringSettingId.HubName), action, @@ -59,7 +40,6 @@ const isHubNameValid: Reducer = (state = true, action) => { }; export default combineReducers({ - flashCurrentProgram, hubName, isHubNameValid, }); diff --git a/src/settings/sagas.test.ts b/src/settings/sagas.test.ts index adb1e741..e39d21de 100644 --- a/src/settings/sagas.test.ts +++ b/src/settings/sagas.test.ts @@ -5,14 +5,12 @@ import { AsyncSaga } from '../../test'; import { - didBooleanChange, didFailToSetString, didStringChange, - setBoolean, setString, settingsToggleShowDocs, } from './actions'; -import { BooleanSettingId, StringSettingId } from './defaults'; +import { StringSettingId } from './defaults'; import settings from './sagas'; afterEach(() => { @@ -44,29 +42,6 @@ describe('store settings to local storage', () => { await saga.end(); }); - - test('flashCurrentProgram', async () => { - const saga = new AsyncSaga(settings); - - saga.updateState({ settings: { flashCurrentProgram: true } }); - - const mockSetItem = jest - .spyOn(Object.getPrototypeOf(window.localStorage), 'setItem') - .mockImplementation((key, value) => { - expect(key).toBe('setting.flashCurrentProgram'); - expect(value).toBe('false'); - }); - - saga.put(setBoolean(BooleanSettingId.FlashCurrentProgram, false)); - expect(mockSetItem).toHaveBeenCalled(); - - const action = await saga.take(); - expect(action).toEqual( - didBooleanChange(BooleanSettingId.FlashCurrentProgram, false), - ); - - await saga.end(); - }); }); describe('storage monitor', () => { diff --git a/src/settings/sagas.ts b/src/settings/sagas.ts index a5b0ed4c..deb5a76c 100644 --- a/src/settings/sagas.ts +++ b/src/settings/sagas.ts @@ -11,25 +11,12 @@ import { didStart } from '../app/actions'; import { RootState } from '../reducers'; import { ensureError } from '../utils'; import { - didBooleanChange, - didFailToSetBoolean, didFailToSetString, didStringChange, - setBoolean, setString, settingsToggleShowDocs, - toggleBoolean, } from './actions'; -import { - BooleanSettingId, - StringSettingId, - getDefaultBooleanValue, - getDefaultStringValue, -} from './defaults'; - -function stringToBoolean(value: string): boolean { - return value.toLowerCase().match(/(true|yes|1)/) !== null; -} +import { StringSettingId, getDefaultStringValue } from './defaults'; function createLocalStorageEventChannel(): EventChannel { return eventChannel((emitter) => { @@ -58,16 +45,6 @@ function* monitorLocalStorage(): Generator { const id = event.key.replace(/^setting\./, ''); - if (Object.values(BooleanSettingId).includes(id as BooleanSettingId)) { - yield* put( - didBooleanChange( - id as BooleanSettingId, - stringToBoolean(event.newValue || 'false'), - ), - ); - continue; - } - if (Object.values(StringSettingId).includes(id as StringSettingId)) { yield* put(didStringChange(id as StringSettingId, event.newValue || '')); continue; @@ -79,17 +56,6 @@ function* monitorLocalStorage(): Generator { } function* loadSettings(): Generator { - for (const id of Object.values(BooleanSettingId)) { - const storageValue = localStorage.getItem(`setting.${id}`); - const defaultValue = getDefaultBooleanValue(id); - const value = - storageValue === null ? defaultValue : stringToBoolean(storageValue); - - if (value !== defaultValue) { - yield* put(didBooleanChange(id, value)); - } - } - for (const id of Object.values(StringSettingId)) { const storageValue = localStorage.getItem(`setting.${id}`); const defaultValue = getDefaultStringValue(id); @@ -101,37 +67,6 @@ function* loadSettings(): Generator { } } -function* storeBooleanSetting(action: ReturnType): Generator { - const key = `setting.${action.id}`; - const newValue = String(action.newState); - - try { - localStorage.setItem(key, newValue); - } catch (err) { - yield* put(didFailToSetBoolean(action.id, ensureError(err))); - } - - // storage event is only raised when a value is changed externally, so we - // mimic the event when we call setItem(), whether it actually succeeded - // or not. - const oldState = yield* select((s: RootState) => s.settings[action.id]); - if (action.newState !== oldState) { - window.dispatchEvent( - new StorageEvent('storage', { - key, - newValue, - oldValue: String(oldState), - storageArea: localStorage, - }), - ); - } -} - -function* toggleBooleanSetting(action: ReturnType): Generator { - const oldValue = yield* select((s: RootState) => s.settings[action.id]); - yield* storeBooleanSetting(setBoolean(action.id, !oldValue)); -} - function* storeStringSetting(action: ReturnType): Generator { const key = `setting.${action.id}`; const newValue = action.newState; @@ -180,8 +115,6 @@ function* handleToggleShowDocs(): Generator { export default function* (): Generator { yield* fork(monitorLocalStorage); yield* takeEvery(didStart, loadSettings); - yield* takeEvery(setBoolean, storeBooleanSetting); - yield* takeEvery(toggleBoolean, toggleBooleanSetting); yield* takeEvery(setString, storeStringSetting); yield* takeEvery(settingsToggleShowDocs, handleToggleShowDocs); }