From 00e4657a391d6cd5aa6fa940a034205b72fc5083 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Thu, 31 Jul 2025 09:12:31 -0700 Subject: [PATCH] Desktop: Resolves #12714: Make more settings per-profile (application layout, note list style, and note list order) (#12825) --- .../app-mobile/utils/buildStartupTasks.ts | 4 +- packages/lib/BaseApplication.ts | 5 +- packages/lib/models/Setting.test.ts | 71 +++++++++-- packages/lib/models/Setting.ts | 120 ++++++++++++++---- .../lib/models/settings/builtInMetadata.ts | 30 ++++- 5 files changed, 186 insertions(+), 44 deletions(-) diff --git a/packages/app-mobile/utils/buildStartupTasks.ts b/packages/app-mobile/utils/buildStartupTasks.ts index d6c1390c85..69e81b610a 100644 --- a/packages/app-mobile/utils/buildStartupTasks.ts +++ b/packages/app-mobile/utils/buildStartupTasks.ts @@ -286,10 +286,10 @@ const buildStartupTasks = ( logger.info('First start on web: Set resource download mode to auto and disabled location tracking.'); } - Setting.skipDefaultMigrations(); + Setting.skipMigrations(); Setting.setValue('firstStart', false); } else { - Setting.applyDefaultMigrations(); + await Setting.applyMigrations(); } if (Setting.value('env') === Env.Dev) { diff --git a/packages/lib/BaseApplication.ts b/packages/lib/BaseApplication.ts index 6843027e4d..0d1aa2cce6 100644 --- a/packages/lib/BaseApplication.ts +++ b/packages/lib/BaseApplication.ts @@ -809,7 +809,7 @@ export default class BaseApplication { const locale = shim.detectAndSetLocale(Setting); reg.logger().info(`First start: detected locale as ${locale}`); } - Setting.skipDefaultMigrations(); + Setting.skipMigrations(); if (Setting.value('env') === 'dev') { Setting.setValue('showTrayIcon', false); @@ -819,8 +819,7 @@ export default class BaseApplication { Setting.setValue('firstStart', false); } else { - Setting.applyDefaultMigrations(); - Setting.applyUserSettingMigration(); + await Setting.applyMigrations(); } setLocale(Setting.value('locale')); diff --git a/packages/lib/models/Setting.test.ts b/packages/lib/models/Setting.test.ts index b5fa3bff09..424b9ccce5 100644 --- a/packages/lib/models/Setting.test.ts +++ b/packages/lib/models/Setting.test.ts @@ -12,6 +12,10 @@ async function loadSettingsFromFile(): Promise { return JSON.parse(await readFile(Setting.settingFilePath, 'utf8')); } +const loadDefaultProfileSettings = async () => { + return JSON.parse(await readFile(`${Setting.value('rootProfileDir')}/settings-1.json`, 'utf8')); +}; + const switchToSubProfileSettings = async () => { await Setting.reset(); const rootProfileDir = Setting.value('profileDir'); @@ -274,7 +278,7 @@ describe('models/Setting', () => { expect(Setting.value('sync.target')).toBe(0); expect(Setting.value('style.editor.contentMaxWidth')).toBe(0); - Setting.applyDefaultMigrations(); + await Setting.applyMigrations(); expect(Setting.value('sync.target')).toBe(7); // Changed expect(Setting.value('style.editor.contentMaxWidth')).toBe(600); // Changed })); @@ -283,7 +287,7 @@ describe('models/Setting', () => { await Setting.reset(); Setting.setValue('sync.target', 9); - Setting.applyDefaultMigrations(); + await Setting.applyMigrations(); expect(Setting.value('sync.target')).toBe(9); // Not changed })); @@ -292,8 +296,8 @@ describe('models/Setting', () => { expect(Setting.value('sync.target')).toBe(0); expect(Setting.value('style.editor.contentMaxWidth')).toBe(0); - Setting.skipDefaultMigrations(); - Setting.applyDefaultMigrations(); + Setting.skipMigrations(); + await Setting.applyMigrations(); expect(Setting.value('sync.target')).toBe(0); // Not changed expect(Setting.value('style.editor.contentMaxWidth')).toBe(0); // Not changed })); @@ -304,7 +308,7 @@ describe('models/Setting', () => { Setting.setValue('lastSettingDefaultMigration', 0); expect(Setting.value('sync.target')).toBe(0); expect(Setting.value('style.editor.contentMaxWidth')).toBe(0); - Setting.applyDefaultMigrations(); + await Setting.applyMigrations(); expect(Setting.value('sync.target')).toBe(0); // Not changed expect(Setting.value('style.editor.contentMaxWidth')).toBe(600); // Changed })); @@ -313,7 +317,7 @@ describe('models/Setting', () => { await Setting.reset(); Setting.setValue('spellChecker.language', 'fr-FR'); - Setting.applyUserSettingMigration(); + await Setting.applyMigrations(); expect(Setting.value('spellChecker.languages')).toStrictEqual(['fr-FR']); })); @@ -322,7 +326,7 @@ describe('models/Setting', () => { Setting.setValue('spellChecker.languages', ['fr-FR', 'en-US']); Setting.setValue('spellChecker.language', 'fr-FR'); - Setting.applyUserSettingMigration(); + await Setting.applyMigrations(); expect(Setting.value('spellChecker.languages')).toStrictEqual(['fr-FR', 'en-US']); })); @@ -330,10 +334,57 @@ describe('models/Setting', () => { await Setting.reset(); expect(Setting.isSet('spellChecker.language')).toBe(false); - Setting.applyUserSettingMigration(); + await Setting.applyMigrations(); expect(Setting.isSet('spellChecker.languages')).toBe(false); })); + it('should migrate global to local settings', (async () => { + await Setting.reset(); + + // Set an initial value -- should store in the global settings + const initialLayout = { key: 'test' }; + Setting.setValue('ui.layout', initialLayout); + await Setting.saveAll(); + + await switchToSubProfileSettings(); + + // The migrations should fetch the previous initial layout from the global settings + expect(Setting.value('ui.layout')).toEqual({}); + await Setting.applyMigrations(); + expect(Setting.value('ui.layout')).toEqual(initialLayout); + + Setting.setValue('ui.layout', { key: 'test 2' }); + await Setting.saveAll(); + + // Should not overwrite parent settings + const globalSettings = await loadDefaultProfileSettings(); + expect(globalSettings['ui.layout']).toEqual(initialLayout); + })); + + it('migrated settings should still be local even if global->local migrations are skipped', async () => { + await Setting.reset(); + const defaultSettingValue = Setting.value('notes.listRendererId'); + + // Set an initial value -- should store in the global settings + Setting.setValue('notes.listRendererId', 'global-setting-value'); + await Setting.saveAll(); + + await switchToSubProfileSettings(); + expect(Setting.value('notes.listRendererId')).toBe(defaultSettingValue); + + // .applyMigrations should not apply skipped migrations + Setting.skipMigrations(); + await Setting.applyMigrations(); + expect(Setting.value('notes.listRendererId')).toBe(defaultSettingValue); + + // The setting should be local -- the parent setting should not be overwritten + Setting.setValue('notes.listRendererId', 'some-other-value'); + await Setting.saveAll(); + + const globalSettings = await loadDefaultProfileSettings(); + expect(globalSettings['notes.listRendererId']).toBe('global-setting-value'); + }); + it('should load sub-profile settings', async () => { await Setting.reset(); @@ -388,7 +439,7 @@ describe('models/Setting', () => { // Double-check that actual file content is correct - const globalSettings = JSON.parse(await readFile(`${Setting.value('rootProfileDir')}/settings-1.json`, 'utf8')); + const globalSettings = await loadDefaultProfileSettings(); const localSettings = JSON.parse(await readFile(`${Setting.value('profileDir')}/settings-1.json`, 'utf8')); expect(globalSettings).toEqual({ @@ -418,7 +469,7 @@ describe('models/Setting', () => { Setting.setValue('sync.target', 2); // Local setting (Sub-profile) await Setting.saveAll(); - const globalSettings = JSON.parse(await readFile(`${Setting.value('rootProfileDir')}/settings-1.json`, 'utf8')); + const globalSettings = await loadDefaultProfileSettings(); expect(globalSettings['sync.target']).toBe(9); }); diff --git a/packages/lib/models/Setting.ts b/packages/lib/models/Setting.ts index 84b2d1b335..fb62b55d17 100644 --- a/packages/lib/models/Setting.ts +++ b/packages/lib/models/Setting.ts @@ -123,6 +123,35 @@ const defaultMigrations: DefaultMigration[] = [ }, ]; +// Global migrations migrate a setting from a global (all-profile) setting to a +// local (per-profile) setting. When adding a new global migration, the setting +// should be set to "isGlobal: true" in "builtInMetadata.ts". +interface GlobalMigration { + name: string; + // At present, this should always be true: + wasGlobal: true; +} + +// The array index is the migration ID -- items should not be removed from this array. +const globalMigrations: GlobalMigration[] = [ + { + name: 'ui.layout', + wasGlobal: true, + }, + { + name: 'notes.sortOrder.field', + wasGlobal: true, + }, + { + name: 'notes.sortOrder.reverse', + wasGlobal: true, + }, + { + name: 'notes.listRendererId', + wasGlobal: true, + }, +]; + // "UserSettingMigration" are used to migrate existing user setting to a new setting. With a way // to transform existing value of the old setting to value and type of the new setting. interface UserSettingMigration { @@ -342,44 +371,85 @@ class Setting extends BaseModel { return `${this.value('rootProfileDir')}/${filename}`; } - public static skipDefaultMigrations() { + public static skipMigrations() { logger.info('Skipping all default migrations...'); this.setValue('lastSettingDefaultMigration', defaultMigrations.length - 1); + this.setValue('lastSettingGlobalMigration', globalMigrations.length - 1); } - public static applyDefaultMigrations() { - logger.info('Applying default migrations...'); - const lastSettingDefaultMigration: number = this.value('lastSettingDefaultMigration'); + public static async applyMigrations() { + const applyDefaultMigrations = () => { + logger.info('Applying default migrations...'); + const lastSettingDefaultMigration: number = this.value('lastSettingDefaultMigration'); - for (let i = 0; i < defaultMigrations.length; i++) { - if (i <= lastSettingDefaultMigration) continue; + for (let i = 0; i < defaultMigrations.length; i++) { + if (i <= lastSettingDefaultMigration) continue; - const migration = defaultMigrations[i]; + const migration = defaultMigrations[i]; - logger.info(`Applying default migration: ${migration.name}`); + logger.info(`Applying default migration: ${migration.name}`); - if (this.isSet(migration.name)) { - logger.info('Skipping because value is already set'); - continue; - } else { - logger.info(`Applying previous default: ${migration.previousDefault}`); - this.setValue(migration.name, migration.previousDefault); + if (this.isSet(migration.name)) { + logger.info('Skipping because value is already set'); + continue; + } else { + logger.info(`Applying previous default: ${migration.previousDefault}`); + this.setValue(migration.name, migration.previousDefault); + } } - } - this.setValue('lastSettingDefaultMigration', defaultMigrations.length - 1); - } + this.setValue('lastSettingDefaultMigration', defaultMigrations.length - 1); + }; - public static applyUserSettingMigration() { - // Function to translate existing user settings to new setting. - // eslint-disable-next-line github/array-foreach -- Old code before rule was applied - userSettingMigration.forEach(userMigration => { - if (!this.isSet(userMigration.newName) && this.isSet(userMigration.oldName)) { - this.setValue(userMigration.newName, userMigration.transformValue(this.value(userMigration.oldName))); - logger.info(`Migrating ${userMigration.oldName} to ${userMigration.newName}`); + const applyGlobalMigrations = async () => { + const lastGlobalMigration = this.value('lastSettingGlobalMigration'); + let rootFileSettings_: SettingValues|null = null; + const rootFileSettings = async () => { + rootFileSettings_ ??= await this.rootFileHandler.load(); + return rootFileSettings_; + }; + + for (let i = 0; i < globalMigrations.length; i++) { + if (i <= lastGlobalMigration) continue; + const migration = globalMigrations[i]; + + // Skip migrations if the setting is stored in the database and thus + // probably can't be fetched from the root profile. This is, for example, + // the case on mobile. + if (this.keyStorage(migration.name) !== SettingStorage.File) { + logger.info('Skipped global value migration -- setting is not stored as a file.'); + continue; + } + + logger.info(`Applying global migration: ${migration.name}`); + if (!migration.wasGlobal) { + throw new Error('Converting a non-global setting to a global setting is not supported.'); + } + + const rootSettings = await rootFileSettings(); + if (Object.prototype.hasOwnProperty.call(rootSettings, migration.name)) { + this.setValue(migration.name, rootSettings[migration.name]); + } } - }); + + this.setValue('lastSettingGlobalMigration', globalMigrations.length - 1); + }; + + const applyUserSettingMigrations = () => { + // Function to translate existing user settings to new setting. + // eslint-disable-next-line github/array-foreach -- Old code before rule was applied + userSettingMigration.forEach(userMigration => { + if (!this.isSet(userMigration.newName) && this.isSet(userMigration.oldName)) { + this.setValue(userMigration.newName, userMigration.transformValue(this.value(userMigration.oldName))); + logger.info(`Migrating ${userMigration.oldName} to ${userMigration.newName}`); + } + }); + }; + + applyDefaultMigrations(); + await applyGlobalMigrations(); + applyUserSettingMigrations(); } public static featureFlagKeys(appType: AppType): string[] { diff --git a/packages/lib/models/settings/builtInMetadata.ts b/packages/lib/models/settings/builtInMetadata.ts index 850e493086..46e3a5ba9f 100644 --- a/packages/lib/models/settings/builtInMetadata.ts +++ b/packages/lib/models/settings/builtInMetadata.ts @@ -697,7 +697,7 @@ const builtInMetadata = (Setting: typeof SettingType) => { return options; }, storage: SettingStorage.File, - isGlobal: true, + isGlobal: false, }, 'editor.autoMatchingBraces': { value: true, @@ -769,7 +769,16 @@ const builtInMetadata = (Setting: typeof SettingType) => { isGlobal: false, }, - 'notes.sortOrder.reverse': { value: true, type: SettingItemType.Bool, storage: SettingStorage.File, isGlobal: true, section: 'note', public: true, label: () => _('Reverse sort order'), appTypes: [AppType.Cli] }, + 'notes.sortOrder.reverse': { + value: true, + type: SettingItemType.Bool, + storage: SettingStorage.File, + isGlobal: false, + section: 'note', + public: true, + label: () => _('Reverse sort order'), + appTypes: [AppType.Cli], + }, // NOTE: A setting whose name starts with 'notes.sortOrder' is special, // which implies changing the setting automatically triggers the refresh of notes. // See lib/BaseApplication.ts/generalMiddleware() for details. @@ -946,7 +955,7 @@ const builtInMetadata = (Setting: typeof SettingType) => { public: false, appTypes: [AppType.Desktop], storage: SettingStorage.File, - isGlobal: true, + isGlobal: false, }, 'plugins.states': { @@ -1233,7 +1242,14 @@ const builtInMetadata = (Setting: typeof SettingType) => { isGlobal: true, }, - 'ui.layout': { value: {}, type: SettingItemType.Object, storage: SettingStorage.File, isGlobal: true, public: false, appTypes: [AppType.Desktop] }, + 'ui.layout': { + value: {}, + type: SettingItemType.Object, + storage: SettingStorage.File, + isGlobal: false, + public: false, + appTypes: [AppType.Desktop], + }, 'ui.lastSelectedPluginPanel': { value: '', @@ -1690,6 +1706,12 @@ const builtInMetadata = (Setting: typeof SettingType) => { public: false, }, + lastSettingGlobalMigration: { + value: -1, + type: SettingItemType.Int, + public: false, + }, + wasClosedSuccessfully: { value: true, type: SettingItemType.Bool,