From 4751b4dd74ca5482e9ad54a00f9fbd6ff4118c30 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Fri, 14 Jun 2024 11:36:44 -0700 Subject: [PATCH] Mobile: Plugin settings screen: Fix plugin states not set correctly when installing multiple plugins at once (#10580) Co-authored-by: Laurent Cozic --- .../controls/plugins/PluginsStates.tsx | 9 ++++-- .../controls/plugins/SearchPlugins.tsx | 5 ++- .../plugins/utils/usePluginCallbacks.ts | 11 ++++--- .../config/plugins/useOnDeleteHandler.ts | 32 +++++++++++++------ .../plugins/useOnInstallHandler.test.ts | 4 +-- .../config/plugins/useOnInstallHandler.ts | 5 +-- 6 files changed, 44 insertions(+), 22 deletions(-) diff --git a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx index 651a61047..119efe79a 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import PluginService, { defaultPluginSetting, Plugins, PluginSetting, PluginSettings } from '@joplin/lib/services/plugins/PluginService'; import { _ } from '@joplin/lib/locale'; import styled from 'styled-components'; @@ -214,8 +214,11 @@ export default function(props: Props) { props.onChange({ value: pluginService.serializePluginSettings(event.value) }); }, [pluginService, props.onChange]); - const onDelete = useOnDeleteHandler(pluginSettings, onPluginSettingsChange, false); - const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettings, repoApi, onPluginSettingsChange, true); + const pluginSettingsRef = useRef(pluginSettings); + pluginSettingsRef.current = pluginSettings; + + const onDelete = useOnDeleteHandler(pluginSettingsRef, onPluginSettingsChange, false); + const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettingsRef, repoApi, onPluginSettingsChange, true); const onToolsClick = useCallback(async () => { const template = [ diff --git a/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx b/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx index ee7d0e356..0190dffd6 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx @@ -40,7 +40,10 @@ export default function(props: Props) { const [installingPluginsIds, setInstallingPluginIds] = useState>({}); const [searchResultCount, setSearchResultCount] = useState(null); - const onInstall = useOnInstallHandler(setInstallingPluginIds, props.pluginSettings, props.repoApi, props.onPluginSettingsChange, false); + const pluginSettingsRef = useRef(props.pluginSettings); + pluginSettingsRef.current = props.pluginSettings; + + const onInstall = useOnInstallHandler(setInstallingPluginIds, pluginSettingsRef, props.repoApi, props.onPluginSettingsChange, false); useEffect(() => { setSearchResultCount(null); diff --git a/packages/app-mobile/components/screens/ConfigScreen/plugins/utils/usePluginCallbacks.ts b/packages/app-mobile/components/screens/ConfigScreen/plugins/utils/usePluginCallbacks.ts index 77feb7490..6da44ba2a 100644 --- a/packages/app-mobile/components/screens/ConfigScreen/plugins/utils/usePluginCallbacks.ts +++ b/packages/app-mobile/components/screens/ConfigScreen/plugins/utils/usePluginCallbacks.ts @@ -4,7 +4,7 @@ import useOnInstallHandler from '@joplin/lib/components/shared/config/plugins/us import NavService from '@joplin/lib/services/NavService'; import { PluginSettings, defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService'; import RepositoryApi from '@joplin/lib/services/plugins/RepositoryApi'; -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useMemo, useRef, useState } from 'react'; interface Props { updatePluginStates: (settingValue: PluginSettings)=> void; @@ -44,14 +44,17 @@ const usePluginCallbacks = (props: Props) => { updatePluginEnabled(pluginId, !settings.enabled); }, [props.pluginSettings, updatePluginEnabled]); - const onDelete = useOnDeleteHandler(props.pluginSettings, onPluginSettingsChange, true); + const pluginSettingsRef = useRef(props.pluginSettings); + pluginSettingsRef.current = props.pluginSettings; + + const onDelete = useOnDeleteHandler(pluginSettingsRef, onPluginSettingsChange, true); const [updatingPluginIds, setUpdatingPluginIds] = useState>({}); - const onUpdate = useOnInstallHandler(setUpdatingPluginIds, props.pluginSettings, props.repoApi, onPluginSettingsChange, true); + const onUpdate = useOnInstallHandler(setUpdatingPluginIds, pluginSettingsRef, props.repoApi, onPluginSettingsChange, true); const [installingPluginIds, setInstallingPluginIds] = useState>({}); const onInstall = useOnInstallHandler( - setInstallingPluginIds, props.pluginSettings, props.repoApi, onPluginSettingsChange, false, + setInstallingPluginIds, pluginSettingsRef, props.repoApi, onPluginSettingsChange, false, ); const onShowPluginLog = useCallback((event: ItemEvent) => { diff --git a/packages/lib/components/shared/config/plugins/useOnDeleteHandler.ts b/packages/lib/components/shared/config/plugins/useOnDeleteHandler.ts index fb9a92190..2003530d6 100644 --- a/packages/lib/components/shared/config/plugins/useOnDeleteHandler.ts +++ b/packages/lib/components/shared/config/plugins/useOnDeleteHandler.ts @@ -1,11 +1,12 @@ import { _ } from '../../../../locale'; import PluginService, { PluginSettings, defaultPluginSetting } from '../../../../services/plugins/PluginService'; +import type * as React from 'react'; import shim from '../../../../shim'; import produce from 'immer'; import { ItemEvent, OnPluginSettingChangeHandler } from './types'; const useOnDeleteHandler = ( - pluginSettings: PluginSettings, + pluginSettingsRef: React.RefObject, onSettingsChange: OnPluginSettingChangeHandler, deleteNow: boolean, ) => { @@ -15,11 +16,6 @@ const useOnDeleteHandler = ( const confirmed = await shim.showConfirmationDialog(_('Delete plugin "%s"?', item.manifest.name)); if (!confirmed) return; - let newSettings = produce(pluginSettings, (draft: PluginSettings) => { - if (!draft[item.manifest.id]) draft[item.manifest.id] = defaultPluginSetting(); - draft[item.manifest.id].deleted = true; - }); - if (deleteNow) { const pluginService = PluginService.instance(); @@ -29,12 +25,28 @@ const useOnDeleteHandler = ( if (plugin) { await pluginService.unloadPlugin(item.manifest.id); } - - newSettings = await pluginService.uninstallPlugins(newSettings); } - onSettingsChange({ value: newSettings }); - }, [pluginSettings, onSettingsChange, deleteNow]); + // Important: To avoid race conditions, don't run any async code between fetching plugin + // settings from the ref and calling onSettingsChange. + let newSettings = pluginSettingsRef.current; + if (deleteNow) { + newSettings = produce(newSettings, (draft: PluginSettings) => { + delete draft[item.manifest.id]; + }); + onSettingsChange({ value: newSettings }); + + await PluginService.instance().uninstallPlugin(item.manifest.id); + } else { + // Setting .deleted causes the app to delete this plugin on startup. + newSettings = produce(newSettings, (draft: PluginSettings) => { + if (!draft[item.manifest.id]) draft[item.manifest.id] = defaultPluginSetting(); + draft[item.manifest.id].deleted = true; + }); + onSettingsChange({ value: newSettings }); + } + + }, [pluginSettingsRef, onSettingsChange, deleteNow]); }; export default useOnDeleteHandler; diff --git a/packages/lib/components/shared/config/plugins/useOnInstallHandler.test.ts b/packages/lib/components/shared/config/plugins/useOnInstallHandler.test.ts index 4d30d4f40..6d5455b14 100644 --- a/packages/lib/components/shared/config/plugins/useOnInstallHandler.test.ts +++ b/packages/lib/components/shared/config/plugins/useOnInstallHandler.test.ts @@ -20,13 +20,13 @@ const itemEvent = ({ } as ItemEvent); const callHook = (isUpdate: boolean, pluginEnabled = true, pluginInstalledViaGUI = true) => () => useOnInstallHandler( setInstallingPluginIds, - { + { current: { [pluginId]: pluginInstalledViaGUI ? { enabled: pluginEnabled, deleted: false, hasBeenUpdated: false, } : undefined, - }, + } }, repoApi, onPluginSettingsChange, isUpdate, diff --git a/packages/lib/components/shared/config/plugins/useOnInstallHandler.ts b/packages/lib/components/shared/config/plugins/useOnInstallHandler.ts index da8df24f5..7ee746917 100644 --- a/packages/lib/components/shared/config/plugins/useOnInstallHandler.ts +++ b/packages/lib/components/shared/config/plugins/useOnInstallHandler.ts @@ -13,7 +13,7 @@ type GetRepoApiCallback = ()=> RepositoryApi; const useOnInstallHandler = ( setInstallingPluginIds: React.Dispatch>>, - pluginSettings: PluginSettings, + pluginSettingsRef: React.RefObject, getRepoApi: GetRepoApiCallback|RepositoryApi, onPluginSettingsChange: OnPluginSettingChangeHandler, isUpdate: boolean, @@ -45,6 +45,7 @@ const useOnInstallHandler = ( } if (!installError) { + const pluginSettings = pluginSettingsRef.current; const newSettings = produce(pluginSettings, (draft: PluginSettings) => { draft[pluginId] = defaultPluginSetting(); if (isUpdate) { @@ -71,7 +72,7 @@ const useOnInstallHandler = ( { buttons: [_('OK')] }, ); } - }, [getRepoApi, isUpdate, pluginSettings, onPluginSettingsChange, setInstallingPluginIds]); + }, [getRepoApi, isUpdate, pluginSettingsRef, onPluginSettingsChange, setInstallingPluginIds]); }; export default useOnInstallHandler;