Chore/user config handling and performance fixes #2856
Conversation
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Hellgartner
force-pushed
the
chore/user-config-handling-and-performance-fixes
branch
from
0a59632 to
0f3855f
Compare
tests showed: electron-settings do not reliably update if multiple updates are pending at the same time Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
src/ElectronBackend/main/__tests__/user-settings-provider.test.ts
Outdated
Show resolved
Hide resolved
| UserSettings, | ||
| } from '../../shared/shared-types'; | ||
|
|
||
| export class UserSettingsProvider { |
There was a problem hiding this comment.
- git does not recognize that this file has been renamed from the old one. could you please fix this to keep the git history intact?
- i'm not sure the name is great. in React, a "provider" usually refers to context provider but what you have here is a service/util. what's wrong with keeping the old name of "UserSettings"?
There was a problem hiding this comment.
Concerning 1)
It does for me, both in IntelliJ as well as in GitHub
There was a problem hiding this comment.
- Take UserSettingsService
There was a problem hiding this comment.
does it really make things easier to have one selector per setting? this way, i have to know exactly the name of what i'm looking for. it seems to me it would be easier to have a single selector for the user settings and then just do settings.MY_SETTING which comes with autocomplete.
| import { useAppDispatch } from '../hooks'; | ||
|
|
||
| // should only be called once | ||
| export function useInitialSyncUserSettings() { |
There was a problem hiding this comment.
hook name no longer matches file name. but as mentioned in my other comment: perhaps we still want a single useUserSettings hook.
| }; | ||
| } | ||
|
|
||
| export function toggleAreHiddenSignalsVisible( |
There was a problem hiding this comment.
you are breaking naming convention. i would have expected "updateAreHiddenSignalsVisible". also, why do only some user settings have a dedicated action while others do not?
suggestion: create only a single action that can be used to update any user setting. then use this action in the single hook for updating any user setting.
| @@ -58,14 +58,14 @@ export function useAuditingOptions({ | |||
| }) { | |||
| const dispatch = useAppDispatch(); | |||
| const store = useAppStore(); | |||
| const [qaMode] = useUserSetting({ key: 'qaMode' }); | |||
| const qaMode = useAppSelector(getQaMode); | |||
There was a problem hiding this comment.
it's strange to me that for some user settings we have dedicated hooks for getting and setting but other user settings we call useAppSelector. suggestion: have a single hook for getting and setting all user settings such as useUserSettings.
There was a problem hiding this comment.
not a great fan of redux action tests. better to write integration or e2e tests.
Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Note: Now the old signal for fetching a single backend information should be unused Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
only allow one channel to get the full user-settings information from the backend Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
… chore/user-config-handling-and-performance-fixes Conflicts: src/ElectronBackend/main/main.ts src/ElectronBackend/main/menu/viewMenu.ts src/shared/shared-types.ts
in order to avoid clashes with react naming where a provider has a fixed meaning Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
…unction Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
…e functions Signed-off-by: Dominikus Hellgartner <dominikus.hellgartner@tngtech.com>
Summary of changes
Exchange the current setup for using electron settings in the frontend with
Context and reason for change
The current setup for the user configuration does not scale to the higher number of uses.
Using
useUserSettingleads to a lot ofhandlers being registered. This has a few unpleasant consequences:
App.tsxcausing re-renders of the entire app on changeThis makes even handling small files hard to work with and makes the app crash on larger, more realistic, files.
How can the changes be tested
Full regression test
Note: Please review the guidelines for contributing to this repository.