-
Notifications
You must be signed in to change notification settings - Fork 182
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.
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
OLD - frontend: PluginSettings: Rework plugin settings local storage usage #2596
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,12 @@ import Box from '@mui/material/Box'; | |
import Button from '@mui/material/Button'; | ||
import Link from '@mui/material/Link'; | ||
import { MRT_Row } from 'material-react-table'; | ||
import { useEffect, useState } from 'react'; | ||
import { useMemo, useState } from 'react'; | ||
import { useTranslation } from 'react-i18next'; | ||
import { useDispatch } from 'react-redux'; | ||
import helpers from '../../../helpers'; | ||
import { useFilterFunc } from '../../../lib/util'; | ||
import { PluginInfo, reloadPage, setPluginSettings } from '../../../plugin/pluginsSlice'; | ||
import { PluginInfo, reloadPage, setEnablePlugin } from '../../../plugin/pluginsSlice'; | ||
import { useTypedSelector } from '../../../redux/reducers/reducers'; | ||
import { Link as HeadlampLink, SectionBox, Table } from '../../common'; | ||
import SectionFilterHeader from '../../common/SectionFilterHeader'; | ||
|
@@ -24,6 +24,7 @@ import SectionFilterHeader from '../../common/SectionFilterHeader'; | |
*/ | ||
export interface PluginSettingsPureProps { | ||
plugins: PluginInfo[]; | ||
pluginsEnabledMap: Record<string, boolean>; | ||
onSave: (plugins: PluginInfo[]) => void; | ||
saveAlwaysEnable?: boolean; | ||
} | ||
|
@@ -92,20 +93,15 @@ const EnableSwitch = (props: SwitchProps) => { | |
|
||
/** PluginSettingsPure is the main component to where we render the plugin data. */ | ||
export function PluginSettingsPure(props: PluginSettingsPureProps) { | ||
const { t } = useTranslation(['translation']); | ||
|
||
/** Plugin arr to be rendered to the page from prop data */ | ||
const pluginArr: any = props.plugins ? props.plugins : []; | ||
const dispatch = useDispatch(); | ||
|
||
/** enableSave state enables the save button when changes are made to the plugin list */ | ||
const [enableSave, setEnableSave] = useState(false); | ||
const { t } = useTranslation(['translation']); | ||
|
||
/** | ||
* pluginChanges state is the array of plugin data and any current changes made by the user to a plugin's "Enable" field via toggler. | ||
* The name and origin fields are split for consistency. | ||
* Copy of plugins with pending changes | ||
*/ | ||
const [pluginChanges, setPluginChanges] = useState(() => | ||
pluginArr.map((plugin: PluginInfo) => { | ||
const [pluginChanges, setPluginChanges] = useState<PluginInfo[]>( | ||
props.plugins.map((plugin: PluginInfo) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using |
||
const [author, name] = plugin.name.includes('@') | ||
? plugin.name.split(/\/(.+)/) | ||
: [null, plugin.name]; | ||
|
@@ -119,60 +115,53 @@ export function PluginSettingsPure(props: PluginSettingsPureProps) { | |
); | ||
|
||
/** | ||
* useEffect to control the rendering of the save button. | ||
* By default, the enableSave is set to false. | ||
* If props.plugins matches pluginChanges enableSave is set to false, disabling the save button. | ||
* pendingPluginsEnabled is either from the local storage or the prop data | ||
*/ | ||
useEffect(() => { | ||
/** This matcher function compares the fields of name and isEnabled of each object in props.plugins to each object in pluginChanges */ | ||
function matcher(objA: PluginInfo, objB: PluginInfo) { | ||
return objA.name === objB.name && objA.isEnabled === objB.isEnabled; | ||
} | ||
|
||
/** | ||
* arrayComp returns true if each object in both arrays are identical by name and isEnabled. | ||
* If both arrays are identical in this scope, then no changes need to be saved. | ||
* If they do not match, there are changes in the pluginChanges array that can be saved and thus enableSave should be enabled. | ||
*/ | ||
const arrayComp = props.plugins.every((val, key) => matcher(val, pluginChanges[key])); | ||
const [pendingPluginsEnabled, setPendingPluginsEnabled] = useState<Record<string, boolean>>( | ||
props.pluginsEnabledMap | ||
); | ||
|
||
/** For storybook usage, determines if the save button should be enabled by default */ | ||
if (props.saveAlwaysEnable) { | ||
setEnableSave(true); | ||
} else { | ||
if (arrayComp) { | ||
setEnableSave(false); | ||
} | ||
if (!arrayComp) { | ||
setEnableSave(true); | ||
} | ||
} | ||
}, [pluginChanges]); | ||
const enableSave = useMemo(() => { | ||
return !pluginChanges.every((plugin: { name: string }) => { | ||
return ( | ||
Boolean(props.pluginsEnabledMap[plugin.name]) === | ||
Boolean(pendingPluginsEnabled[plugin.name]) | ||
); | ||
}); | ||
}, [pendingPluginsEnabled, pluginChanges, props.pluginsEnabledMap]); | ||
|
||
/** | ||
* onSaveButton function to be called once the user clicks the Save button. | ||
* This function then takes the current state of the pluginChanges array and inputs it to the onSave prop function. | ||
*/ | ||
function onSaveButtonHandler() { | ||
props.onSave(pluginChanges); | ||
dispatch(setEnablePlugin(pendingPluginsEnabled)); | ||
dispatch(reloadPage()); | ||
} | ||
|
||
/** | ||
* On change function handler to control the enableSave state and update the pluginChanges state. | ||
* This function is called on every plugin toggle action and recreates the state for pluginChanges. | ||
* Once the user clicks a toggle, the Save button is also rendered via setEnableSave. | ||
*/ | ||
function switchChangeHanlder(plug: { name: any }) { | ||
function switchChangeHandler(plug: { name: any }) { | ||
const plugName = plug.name; | ||
|
||
setPluginChanges((currentInfo: any[]) => | ||
currentInfo.map((p: { name: any; isEnabled: any }) => { | ||
setPluginChanges([ | ||
...pluginChanges.map(p => { | ||
if (p.name === plugName) { | ||
return { ...p, isEnabled: !p.isEnabled }; | ||
return { | ||
...p, | ||
isEnabled: !p.isEnabled, | ||
}; | ||
} | ||
return p; | ||
}) | ||
); | ||
}), | ||
]); | ||
|
||
setPendingPluginsEnabled({ | ||
...pendingPluginsEnabled, | ||
[plugName]: !pendingPluginsEnabled[plugName], | ||
}); | ||
} | ||
|
||
return ( | ||
|
@@ -235,7 +224,9 @@ export function PluginSettingsPure(props: PluginSettingsPureProps) { | |
if (plugin.isCompatible === false) { | ||
return t('translation|Incompatible'); | ||
} | ||
return plugin.isEnabled ? t('translation|Enabled') : t('translation|Disabled'); | ||
return pendingPluginsEnabled[plugin.name] | ||
? t('translation|Enabled') | ||
: t('translation|Disabled'); | ||
}, | ||
}, | ||
{ | ||
|
@@ -247,8 +238,8 @@ export function PluginSettingsPure(props: PluginSettingsPureProps) { | |
return ( | ||
<EnableSwitch | ||
aria-label={`Toggle ${plugin.name}`} | ||
checked={plugin.isEnabled} | ||
onChange={() => switchChangeHanlder(plugin)} | ||
checked={pendingPluginsEnabled[plugin.name]} | ||
onChange={() => switchChangeHandler(plugin)} | ||
color="primary" | ||
name={plugin.name} | ||
/> | ||
|
@@ -284,15 +275,60 @@ export function PluginSettingsPure(props: PluginSettingsPureProps) { | |
export default function PluginSettings() { | ||
const dispatch = useDispatch(); | ||
|
||
const pluginSettings = useTypedSelector(state => state.plugins.pluginSettings); | ||
const pluginData = useTypedSelector(state => state.plugins.pluginData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all this logic should probably be moved to a useEffect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have tried placing it in useEffect before but it would either error out entirely or cause extra re renders often so I had opted for controlled re renders using the page reload |
||
|
||
const oldLocalEnabledList = localStorage.getItem('headlampPluginSettings'); | ||
const localEnabledList = localStorage.getItem('enabledPluginsList'); | ||
|
||
let pluginsEnabledMap; | ||
|
||
/** | ||
* Note: this is a temporary fix to handle the migration of the old plugin settings. | ||
*/ | ||
if (oldLocalEnabledList) { | ||
const oldSettings = JSON.parse(oldLocalEnabledList); | ||
|
||
pluginsEnabledMap = oldSettings.reduce( | ||
(acc: Record<string, any>, p: { name: string; isEnabled: boolean }) => { | ||
acc[p.name] = !!p.isEnabled; | ||
return acc; | ||
}, | ||
{} as Record<string, any> | ||
); | ||
|
||
localStorage.setItem('enabledPluginsList', JSON.stringify(pluginsEnabledMap)); | ||
localStorage.removeItem('headlampPluginSettings'); | ||
|
||
dispatch(setEnablePlugin(pluginsEnabledMap)); | ||
dispatch(reloadPage()); | ||
} else { | ||
/** | ||
* NOTE: For compatibility with old settings, we need to check if the `localEnabledList` exists. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we have 2 deprecated items? headlampPluginSettings + enabledPluginsList? |
||
* If `localEnabledList` exists, parse it and assign it to `pluginsEnabledMap`. | ||
* This indicates that previous plugin settings have been saved and can be used. | ||
* | ||
* If `localEnabledList` does not exist, it means the settings are not initialized | ||
* and no previous plugin settings have been saved. In this case, default the plugins | ||
* to being disabled to allow users to turn on their desired plugins. | ||
*/ | ||
if (localEnabledList) { | ||
pluginsEnabledMap = JSON.parse(localEnabledList) as Record<string, boolean>; | ||
} else { | ||
pluginsEnabledMap = pluginData.reduce((acc, p) => { | ||
acc[p.name] = !!p.isEnabled; | ||
return acc; | ||
}, {} as Record<string, boolean>); | ||
|
||
dispatch(setEnablePlugin(pluginsEnabledMap)); | ||
dispatch(reloadPage()); | ||
} | ||
} | ||
|
||
return ( | ||
<PluginSettingsPure | ||
plugins={pluginSettings} | ||
onSave={plugins => { | ||
dispatch(setPluginSettings(plugins)); | ||
dispatch(reloadPage()); | ||
}} | ||
plugins={pluginData} | ||
pluginsEnabledMap={pluginsEnabledMap} | ||
onSave={() => {}} | ||
Comment on lines
-292
to
+331
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So on save we do nothing? I am confused with what this PR is supposed to fix now. |
||
/> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overly complex for its purpose. Also it says it creates a list but it creates a record.