All: Added mechanism to migrate default settings to new values

packages/app-mobile/root.tsx

@@ -487,6 +487,18 @@ async function initialize(dispatch: Function) {
 		await loadKeychainServiceAndSettings(KeychainServiceDriverMobile);
 		await migrateMasterPassword();
 
+		if (!Setting.value('clientId')) Setting.setValue('clientId', uuid.create());
+
+		if (Setting.value('firstStart')) {
+			let locale = NativeModules.I18nManager.localeIdentifier;
+			if (!locale) locale = defaultLocale();
+			Setting.setValue('locale', closestSupportedLocale(locale));
+			Setting.skipDefaultMigrations();
+			Setting.setValue('firstStart', 0);
+		} else {
+			Setting.applyDefaultMigrations();
+		}
+
 		if (Setting.value('env') === Env.Dev) {
 			// Setting.setValue('sync.10.path', 'https://api.joplincloud.com');
 			// Setting.setValue('sync.10.userContentPath', 'https://joplinusercontent.com');
@@ -498,16 +510,6 @@ async function initialize(dispatch: Function) {
 			Setting.setValue('sync.10.password', 'hunter1hunter2hunter3');
 		}
 
-		if (!Setting.value('clientId')) Setting.setValue('clientId', uuid.create());
-
-		if (Setting.value('firstStart')) {
-			let locale = NativeModules.I18nManager.localeIdentifier;
-			if (!locale) locale = defaultLocale();
-			Setting.setValue('locale', closestSupportedLocale(locale));
-			Setting.setValue('sync.target', 0);
-			Setting.setValue('firstStart', 0);
-		}
-
 		if (Setting.value('db.ftsEnabled') === -1) {
 			const ftsEnabled = await db.ftsEnabled();
 			Setting.setValue('db.ftsEnabled', ftsEnabled ? 1 : 0);

packages/lib/BaseApplication.ts

@@ -787,15 +787,17 @@ export default class BaseApplication {
 			const locale = shim.detectAndSetLocale(Setting);
 			reg.logger().info(`First start: detected locale as ${locale}`);
 
+			Setting.skipDefaultMigrations();
+
 			if (Setting.value('env') === 'dev') {
 				Setting.setValue('showTrayIcon', 0);
 				Setting.setValue('autoUpdateEnabled', 0);
 				Setting.setValue('sync.interval', 3600);
 			}
 
-			Setting.setValue('sync.target', 0);
 			Setting.setValue('firstStart', 0);
 		} else {
+			Setting.applyDefaultMigrations();
 			setLocale(Setting.value('locale'));
 		}
 

packages/lib/models/Setting.test.ts

@@ -216,4 +216,36 @@ describe('models/Setting', function() {
 		expect(await fs.readFile(`${Setting.value('profileDir')}/${files[0]}`, 'utf8')).toBe(badContent);
 	}));
 
+	it('should allow applying default migrations', (async () => {
+		await Setting.reset();
+
+		expect(Setting.value('sync.target')).toBe(0);
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(0);
+		Setting.applyDefaultMigrations();
+		expect(Setting.value('sync.target')).toBe(7); // Changed
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(600); // Changed
+	}));
+
+	it('should allow skipping default migrations', (async () => {
+		await Setting.reset();
+
+		expect(Setting.value('sync.target')).toBe(0);
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(0);
+		Setting.skipDefaultMigrations();
+		Setting.applyDefaultMigrations();
+		expect(Setting.value('sync.target')).toBe(0); // Not changed
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(0); // Not changed
+	}));
+
+	it('should not apply migrations that have already been applied', (async () => {
+		await Setting.reset();
+
+		Setting.setValue('lastSettingDefaultMigration', 0);
+		expect(Setting.value('sync.target')).toBe(0);
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(0);
+		Setting.applyDefaultMigrations();
+		expect(Setting.value('sync.target')).toBe(0); // Not changed
+		expect(Setting.value('style.editor.contentMaxWidth')).toBe(600); // Changed
+	}));
+
 });

packages/lib/models/Setting.ts

@@ -6,11 +6,14 @@ import Database from '../database';
 import SyncTargetRegistry from '../SyncTargetRegistry';
 import time from '../time';
 import FileHandler, { SettingValues } from './settings/FileHandler';
+import Logger from '../Logger';
 const { sprintf } = require('sprintf-js');
 const ObjectUtils = require('../ObjectUtils');
 const { toTitleCase } = require('../string-utils.js');
 const { rtrimSlashes, toSystemSlashes } = require('../path-utils');
 
+const logger = Logger.create('models/Setting');
+
 export enum SettingItemType {
 	Int = 1,
 	String = 2,
@@ -122,6 +125,54 @@ interface SettingSections {
 	[key: string]: SettingSection;
 }
 
+// "Default migrations" are used to migrate previous setting defaults to new
+// values. If we simply change the default in the metadata, it might cause
+// problems if the user has never previously set the value.
+//
+// It happened for example when changing the "sync.target" from 7 (Dropbox) to 0
+// (None). Users who had never explicitly set the sync target and were using
+// Dropbox would suddenly have their sync target set to "none".
+//
+// So the technique is like this:
+//
+// - If the app has previously been executed, we run the migrations, which do
+//   something like this:
+//     - If the setting has never been set, set it to the previous default
+//       value. For example, for sync.target, it would set it to "7".
+//     - If the setting has been explicitly set, keep the current value.
+// - If the app runs for the first time, skip all the migrations. So
+//   "sync.target" would be set to 0.
+//
+// A default migration runs only once (or never, if it is skipped).
+//
+// The handlers to either apply or skip the migrations must be called from the
+// application, in the initialization code.
+
+interface DefaultMigration {
+	name: string;
+	previousDefault: any;
+}
+
+// To create a default migration:
+//
+// - Set the new default value in the setting metadata
+// - Add an entry below with the name of the setting and the **previous**
+//   default value.
+//
+// **Never** removes an item from this array, as the array index is essentially
+// the migration ID.
+
+const defaultMigrations: DefaultMigration[] = [
+	{
+		name: 'sync.target',
+		previousDefault: 7,
+	},
+	{
+		name: 'style.editor.contentMaxWidth',
+		previousDefault: 600,
+	},
+];
+
 class Setting extends BaseModel {
 
 	public static schemaUrl = 'https://joplinapp.org/schema/settings.json';
@@ -319,7 +370,7 @@ class Setting extends BaseModel {
 			},
 
 			'sync.target': {
-				value: 7, // Dropbox
+				value: 0,
 				type: SettingItemType.Int,
 				isEnum: true,
 				public: true,
@@ -985,7 +1036,7 @@ class Setting extends BaseModel {
 				storage: SettingStorage.File,
 			},
 
-			'style.editor.contentMaxWidth': { value: 600, type: SettingItemType.Int, public: true, storage: SettingStorage.File, appTypes: [AppType.Desktop], section: 'appearance', label: () => _('Editor maximum width'), description: () => _('Set it to 0 to make it take the complete available space.') },
+			'style.editor.contentMaxWidth': { value: 0, type: SettingItemType.Int, public: true, storage: SettingStorage.File, appTypes: [AppType.Desktop], section: 'appearance', label: () => _('Editor maximum width'), description: () => _('Set it to 0 to make it take the complete available space. Recommended width is 600.') },
 
 			'ui.layout': { value: {}, type: SettingItemType.Object, storage: SettingStorage.File, public: false, appTypes: [AppType.Desktop] },
 
@@ -1289,6 +1340,12 @@ class Setting extends BaseModel {
 				storage: SettingStorage.Database,
 			},
 
+			lastSettingDefaultMigration: {
+				value: -1,
+				type: SettingItemType.Int,
+				public: false,
+			},
+
 			// 'featureFlag.syncAccurateTimestamps': {
 			// 	value: false,
 			// 	type: SettingItemType.Bool,
@@ -1310,6 +1367,35 @@ class Setting extends BaseModel {
 		return this.metadata_;
 	}
 
+	public static skipDefaultMigrations() {
+		logger.info('Skipping all default migrations...');
+
+		this.setValue('lastSettingDefaultMigration', defaultMigrations.length - 1);
+	}
+
+	public static applyDefaultMigrations() {
+		logger.info('Applying default migrations...');
+		const lastSettingDefaultMigration: number = this.value('lastSettingDefaultMigration');
+
+		for (let i = 0; i < defaultMigrations.length; i++) {
+			if (i <= lastSettingDefaultMigration) continue;
+
+			const migration = defaultMigrations[i];
+
+			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);
+			}
+		}
+
+		this.setValue('lastSettingDefaultMigration', defaultMigrations.length - 1);
+	}
+
 	public static featureFlagKeys(appType: AppType): string[] {
 		const keys = this.keys(false, appType);
 		return keys.filter(k => k.indexOf('featureFlag.') === 0);
@@ -1370,6 +1456,10 @@ class Setting extends BaseModel {
 		return key in this.metadata();
 	}
 
+	public static isSet(key: string) {
+		return key in this.cache_;
+	}
+
 	static keyDescription(key: string, appType: AppType = null) {
 		const md = this.settingMetadata(key);
 		if (!md.description) return null;
@@ -1545,7 +1635,7 @@ class Setting extends BaseModel {
 				this.changedKeys_.push(key);
 
 				// Don't log this to prevent sensitive info (passwords, auth tokens...) to end up in logs
-				// this.logger().info('Setting: ' + key + ' = ' + c.value + ' => ' + value);
+				// logger.info('Setting: ' + key + ' = ' + c.value + ' => ' + value);
 
 				if ('minimum' in md && value < md.minimum) value = md.minimum;
 				if ('maximum' in md && value > md.maximum) value = md.maximum;
@@ -1774,7 +1864,7 @@ class Setting extends BaseModel {
 	public static async saveAll() {
 		if (Setting.autoSaveEnabled && !this.saveTimeoutId_) return Promise.resolve();
 
-		this.logger().debug('Saving settings...');
+		logger.debug('Saving settings...');
 		shim.clearTimeout(this.saveTimeoutId_);
 		this.saveTimeoutId_ = null;
 
@@ -1814,7 +1904,7 @@ class Setting extends BaseModel {
 						continue;
 					}
 				} catch (error) {
-					this.logger().error(`Could not set setting on the keychain. Will be saved to database instead: ${s.key}:`, error);
+					logger.error(`Could not set setting on the keychain. Will be saved to database instead: ${s.key}:`, error);
 				}
 			}
 
@@ -1832,7 +1922,7 @@ class Setting extends BaseModel {
 
 		if (this.canUseFileStorage()) await this.fileHandler.save(valuesForFile);
 
-		this.logger().debug('Settings have been saved.');
+		logger.debug('Settings have been saved.');
 	}
 
 	static scheduleChangeEvent() {
@@ -1856,7 +1946,7 @@ class Setting extends BaseModel {
 
 		if (!this.changedKeys_.length) {
 			// Sanity check - shouldn't happen
-			this.logger().warn('Trying to dispatch a change event without any changed keys');
+			logger.warn('Trying to dispatch a change event without any changed keys');
 			return;
 		}
 
@@ -1874,7 +1964,7 @@ class Setting extends BaseModel {
 			try {
 				await this.saveAll();
 			} catch (error) {
-				this.logger().error('Could not save settings', error);
+				logger.error('Could not save settings', error);
 			}
 		}, 500);
 	}