Desktop: Resolves #12714: Make more settings per-profile (application layout, note list style, and note list order) (#12825)

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) {

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'));

packages/lib/models/Setting.test.ts

@@ -12,6 +12,10 @@ async function loadSettingsFromFile(): Promise<any> {
 	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);
 	});
 

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[] {

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,