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.

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

Chore/user config handling and performance fixes #2856

Open
wants to merge 18 commits into
base: feat/make-classification-and-criticality-optional
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7c6cc6f
chore: proper default handling removing necessity for most of the nulls
Hellgartner Mar 14, 2025
b16240a
test: ad test for properly setting default values
Hellgartner Mar 14, 2025
194a1ee
chore: add reducer for user config
Hellgartner Mar 18, 2025
b725fc8
chore: sync user settings on app start
Hellgartner Mar 18, 2025
bc4dee4
chore: keep state in sync with backend
Hellgartner Mar 18, 2025
93def01
chore: move show classifications and show criticality to new approach
Hellgartner Mar 18, 2025
769c623
chore: move the remaining usage of use user settings to the new way
Hellgartner Mar 18, 2025
0f3855f
test: fix e2e tests
Hellgartner Mar 19, 2025
2e33471
fix: fix updating multiple values
Hellgartner Mar 19, 2025
86a4850
test: add tests for user settings actions
Hellgartner Mar 19, 2025
25861ef
test: add e2e test
Hellgartner Mar 19, 2025
9c3af15
fix: review-comment: add a few empty lines to improve readability
Hellgartner Mar 20, 2025
5393b42
fix: review-comment: Move showing the project popup to the new mechanism
Hellgartner Mar 20, 2025
27149ef
fix: review-comment: Simplify IPC interface
Hellgartner Mar 20, 2025
ad933a3
Merge branch 'feat/make-classification-and-criticality-optional' into…
Hellgartner Mar 20, 2025
f03015d
refactor: review-column: rename UserSettingsProvider
Hellgartner Mar 20, 2025
8fd911b
refactor: review-comment: make update user settings accept a update f…
Hellgartner Mar 20, 2025
8314a57
refactor: review-comment: user the new capability to inline the toggl…
Hellgartner Mar 20, 2025
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
4 changes: 2 additions & 2 deletions src/ElectronBackend/main/__tests__/menu.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import electron, { BrowserWindow, MenuItemConstructorOptions } from 'electron';

import { createMenu } from '../menu';
import { UserSettings } from '../user-settings';
import { UserSettingsService } from '../user-settings-service';

jest.mock('electron', () => ({
BrowserWindow: class BrowserWindowMock {},
Expand Down Expand Up @@ -49,7 +49,7 @@ describe('create menu', () => {
];
testCases.forEach((testCase) => {
it(`evaluates ${testCase.darkMode ? 'dark' : 'light'} mode properly`, async () => {
await UserSettings.init();
await UserSettingsService.init();
const mainWindow = new BrowserWindow();

// Important to set this up only here and not in the mock setup
Expand Down
151 changes: 151 additions & 0 deletions src/ElectronBackend/main/__tests__/user-settings-provider.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
// SPDX-FileCopyrightText: Meta Platforms, Inc. and its affiliates
// SPDX-FileCopyrightText: TNG Technology Consulting GmbH <https://www.tngtech.com>
//
// SPDX-License-Identifier: Apache-2.0
import { BrowserWindow } from 'electron';
import settings from 'electron-settings';
import { rmSync } from 'node:fs';
import { mkdtemp } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join } from 'node:path';

import { AllowedFrontendChannels } from '../../../shared/ipc-channels';
import {
DEFAULT_PANEL_SIZES,
DEFAULT_USER_SETTINGS,
} from '../../../shared/shared-constants';
import { UserSettingsService } from '../user-settings-service';

type MockedBrowserWindow = BrowserWindow & {
sendFunction: (channel: string, ...args: Array<unknown>) => void;
};

jest.mock('electron', () => {
const sendFunction = jest.fn();
return {
BrowserWindow: {
getAllWindows: () => {
return [
{
webContents: {
send: sendFunction,
},
},
];
},
sendFunction,
},
};
});

describe('UserSettings', () => {
let temporaryDir: string | undefined = undefined;
beforeEach(async () => {
temporaryDir = await mkdtemp(join(tmpdir(), 'OpossumUiTesting-'));
settings.configure({ dir: temporaryDir, atomicSave: true });
});

afterEach(() => {
if (temporaryDir) {
rmSync(temporaryDir, { recursive: true, force: true });
}
});

describe('init', () => {
it('sets up the default values if empty', async () => {
await UserSettingsService.init();

const result = await settings.get();

expect(result).toEqual(DEFAULT_USER_SETTINGS);
});

it('overwrites only the non set values if there are already values set', async () => {
await settings.set('qaMode', true);

await UserSettingsService.init();

const result = await settings.get();

expect(result).toEqual({ ...DEFAULT_USER_SETTINGS, qaMode: true });
});

it('resets everything if reset is requested', async () => {
const oldEnvironment = process.env;
process.env = { ...oldEnvironment, RESET: 'TRUE' };

await settings.set('qaMode', true);

await UserSettingsService.init();

const result = await settings.get();

expect(result).toEqual({ ...DEFAULT_USER_SETTINGS });
process.env = oldEnvironment;
});
});

describe('get', () => {
it('gets a user setting from a predescribed path', async () => {
await UserSettingsService.init();

const panelSizes = await UserSettingsService.get('panelSizes');

expect(panelSizes).toEqual(DEFAULT_PANEL_SIZES);
});

it('gets the full user settings', async () => {
await UserSettingsService.init();

const userSettings = await UserSettingsService.get();

expect(userSettings).toEqual(DEFAULT_USER_SETTINGS);
});
});

describe('write operations', () => {
it('sets a value and communicates to the frontend', async () => {
await UserSettingsService.set('qaMode', true);

const qaMode = await UserSettingsService.get('qaMode');
expect(qaMode).toBe(true);
expect(
(BrowserWindow as unknown as MockedBrowserWindow).sendFunction,
).toHaveBeenCalledWith(AllowedFrontendChannels.UserSettingsChanged, {
qaMode: true,
});
});

it('sets a value and does not communicates to the frontend if disabled', async () => {
await UserSettingsService.set('qaMode', true, {
skipNotification: true,
});

const qaMode = await UserSettingsService.get('qaMode');
expect(qaMode).toBe(true);
expect(
(BrowserWindow as unknown as MockedBrowserWindow).sendFunction,
).not.toHaveBeenCalled();
});

it('allows to update multiple values at once', async () => {
await UserSettingsService.init();

await UserSettingsService.update({
qaMode: true,
showClassifications: false,
});

const userSettings = await UserSettingsService.get();

expect(
(BrowserWindow as unknown as MockedBrowserWindow).sendFunction,
).toHaveBeenCalledTimes(2);
expect(userSettings).toEqual({
...DEFAULT_USER_SETTINGS,
qaMode: true,
showClassifications: false,
});
});
});
});
8 changes: 5 additions & 3 deletions src/ElectronBackend/main/listeners.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ import {
} from './globalBackendState';
import logger from './logger';
import { createMenu } from './menu';
import { UserSettings } from './user-settings';
import { UserSettingsService } from './user-settings-service';

const MAX_NUMBER_OF_RECENTLY_OPENED_PATHS = 10;

Expand Down Expand Up @@ -361,8 +361,10 @@ export async function openFile(
}

async function updateRecentlyOpenedPaths(filePath: string): Promise<void> {
const recentlyOpenedPaths = await UserSettings.get('recentlyOpenedPaths');
await UserSettings.set(
const recentlyOpenedPaths = await UserSettingsService.get(
'recentlyOpenedPaths',
);
await UserSettingsService.set(
'recentlyOpenedPaths',
uniq([filePath, ...(recentlyOpenedPaths ?? [])]).slice(
0,
Expand Down
15 changes: 8 additions & 7 deletions src/ElectronBackend/main/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { dialog, ipcMain, systemPreferences } from 'electron';
import os from 'os';

import { AllowedFrontendChannels, IpcChannel } from '../../shared/ipc-channels';
import { UserSettings } from '../../shared/shared-types';
import { getMessageBoxContentForErrorsWrapper } from '../errorHandling/errorHandling';
import { createWindow, loadWebApp } from './createWindow';
import {
Expand All @@ -21,7 +22,7 @@ import {
import { createMenu } from './menu';
import { DisabledMenuItemHandler } from './menu/DisabledMenuItemHandler';
import { openFileFromCliOrEnvVariableIfProvided } from './openFileFromCliOrEnvVariableIfProvided';
import { UserSettings } from './user-settings';
import { UserSettingsService } from './user-settings-service';

export async function main(): Promise<void> {
try {
Expand All @@ -35,7 +36,7 @@ export async function main(): Promise<void> {

const mainWindow = createWindow();

await UserSettings.init();
await UserSettingsService.init();
await createMenu(mainWindow);

mainWindow.webContents.session.webRequest.onBeforeSendHeaders(
Expand Down Expand Up @@ -92,11 +93,11 @@ export async function main(): Promise<void> {
}),
);
ipcMain.handle(IpcChannel.OpenLink, openLinkListener);
ipcMain.handle(IpcChannel.GetUserSettings, (_, key) =>
UserSettings.get(key),
);
ipcMain.handle(IpcChannel.SetUserSettings, (_, { key, value }) =>
UserSettings.set(key, value, { skipNotification: true }),
ipcMain.handle(IpcChannel.GetUserSettings, () => UserSettingsService.get());
ipcMain.handle(
IpcChannel.SetUserSettings,
(_, userSettings: Partial<UserSettings>) =>
UserSettingsService.update(userSettings, true),
);

await loadWebApp(mainWindow);
Expand Down
8 changes: 5 additions & 3 deletions src/ElectronBackend/main/menu/fileMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
} from '../listeners';
import logger from '../logger';
import { createMenu } from '../menu';
import { UserSettings } from '../user-settings';
import { UserSettingsService } from '../user-settings-service';
import { DisabledMenuItemHandler } from './DisabledMenuItemHandler';

export const importFileFormats: Array<FileFormatInfo> = [
Expand Down Expand Up @@ -67,7 +67,9 @@ function getOpenFile(mainWindow: BrowserWindow): MenuItemConstructorOptions {
async function getOpenRecent(
mainWindow: BrowserWindow,
): Promise<MenuItemConstructorOptions> {
const recentlyOpenedPaths = await UserSettings.get('recentlyOpenedPaths');
const recentlyOpenedPaths = await UserSettingsService.get(
'recentlyOpenedPaths',
);

return {
icon: getIconBasedOnTheme('icons/open-white.png', 'icons/open-black.png'),
Expand Down Expand Up @@ -101,7 +103,7 @@ function getOpenRecentSubmenu(
id: 'clear-recent',
label: text.menu.fileSubmenu.clearRecent,
click: async () => {
await UserSettings.set('recentlyOpenedPaths', []);
await UserSettingsService.set('recentlyOpenedPaths', []);
await createMenu(mainWindow);
},
},
Expand Down
6 changes: 3 additions & 3 deletions src/ElectronBackend/main/menu/switchableMenuItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { BrowserWindow, MenuItemConstructorOptions } from 'electron';
import { UserSettings as IUserSettings } from '../../../shared/shared-types';
import { getCheckboxBasedOnThemeAndCheckState } from '../iconHelpers';
import { createMenu } from '../menu';
import { UserSettings } from '../user-settings';
import { UserSettingsService } from '../user-settings-service';

export interface SwitchableItemOptions {
id: string;
Expand All @@ -19,13 +19,13 @@ export async function switchableMenuItem(
mainWindow: BrowserWindow,
options: SwitchableItemOptions,
): Promise<MenuItemConstructorOptions> {
const state = !!(await UserSettings.get(options.userSettingsKey));
const state = !!(await UserSettingsService.get(options.userSettingsKey));
return {
icon: getCheckboxBasedOnThemeAndCheckState(state),
id: state ? `enabled-${options.id}` : `disabled-${options.id}`,
label: options.label,
click: async () => {
await UserSettings.set(options.userSettingsKey, !state);
await UserSettingsService.set(options.userSettingsKey, !state);
await createMenu(mainWindow);
},
};
Expand Down
74 changes: 74 additions & 0 deletions src/ElectronBackend/main/user-settings-service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// SPDX-FileCopyrightText: Meta Platforms, Inc. and its affiliates
// SPDX-FileCopyrightText: TNG Technology Consulting GmbH <https://www.tngtech.com>
//
// SPDX-License-Identifier: Apache-2.0
import { BrowserWindow } from 'electron';
import log from 'electron-log';
import settings from 'electron-settings';

import { AllowedFrontendChannels } from '../../shared/ipc-channels';
import { DEFAULT_USER_SETTINGS } from '../../shared/shared-constants';
import {
UserSettings as IUserSettings,
UserSettings,
} from '../../shared/shared-types';

export class UserSettingsService {
public static async init() {
if (process.argv.includes('--reset') || process.env.RESET) {
log.info('Resetting user settings');
await settings.set(DEFAULT_USER_SETTINGS as unknown as never);
} else {
const currentSettings = await settings.get();
await settings.set({
...DEFAULT_USER_SETTINGS,
...currentSettings,
});
}
}

public static get<T extends keyof IUserSettings>(
path: T,
): Promise<IUserSettings[T]>;
public static get(): Promise<IUserSettings>;
public static get<T extends keyof IUserSettings>(
path?: T,
): Promise<IUserSettings[T]> | Promise<IUserSettings> {
if (path) {
return settings.get(path) as Promise<IUserSettings[T]>;
}
return settings.get() as unknown as Promise<IUserSettings>;
}

public static async update(
userSettings: Partial<IUserSettings>,
skipNotification: boolean = false,
): Promise<void> {
for (const key of Object.keys(userSettings)) {
const properKey = key as keyof UserSettings;
if (userSettings[properKey] !== undefined) {
await UserSettingsService.set(properKey, userSettings[properKey], {
skipNotification,
});
}
}
}

public static async set<T extends keyof IUserSettings>(
path: T,
value: IUserSettings[T],
{ skipNotification }: { skipNotification?: boolean } = {},
): Promise<void> {
await settings.set(path, value);

if (!skipNotification) {
const partialSettingsToUpdate = Object.fromEntries([[path, value]]);
BrowserWindow.getAllWindows().forEach((window) => {
window.webContents.send(
AllowedFrontendChannels.UserSettingsChanged,
partialSettingsToUpdate,
);
});
}
}
}
Loading
Loading