Mobile: Resolves #10883: Remove slider component module and replace integer settings with validated text inputs and ensure all validation error flows work correctly

package.json

@@ -108,7 +108,6 @@
     "[email protected]": "patch:app-builder-lib@npm%3A24.4.0#./.yarn/patches/app-builder-lib-npm-24.4.0-05322ff057.patch",
     "nanoid": "patch:nanoid@npm%3A3.3.7#./.yarn/patches/nanoid-npm-3.3.7-98824ba130.patch",
     "pdfjs-dist": "patch:pdfjs-dist@npm%3A3.11.174#./.yarn/patches/pdfjs-dist-npm-3.11.174-67f2fee6d6.patch",
-    "@react-native-community/slider": "patch:@react-native-community/slider@npm%3A4.4.4#./.yarn/patches/@react-native-community-slider-npm-4.4.4-d78e472f48.patch",
     "husky": "patch:husky@npm%3A3.1.0#./.yarn/patches/husky-npm-3.1.0-5cc13e4e34.patch",
     "chokidar@^2.0.0": "3.5.3",
     "[email protected]": "patch:react-native@npm%3A0.74.1#./.yarn/patches/react-native-npm-0.74.1-754c02ae9e.patch",

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

@@ -140,18 +140,24 @@ class ConfigScreenComponent extends React.Component<any, any> {
 	public async switchSection(name: string) {
 		const section = this.sectionByName(name);
 		let screenName = '';
-		if (section.isScreen) {
-			screenName = section.name;
+		if (section.isScreen || name === 'plugins') {
+			if (section.isScreen) {
+				screenName = section.name;
+			}
 
 			if (this.hasChanges()) {
-				const ok = confirm(_('This will open a new screen. Save your current changes?'));
+				const ok = bridge().showConfirmMessageBox(_('This will open a new screen. Save your current changes?'));
 				if (ok) {
-					await shared.saveSettings(this);
+					const result = await shared.saveSettings(this);
+					if (result === false) return false;
+				} else {
+					return false;
 				}
 			}
 		}
 
 		this.setState({ selectedSectionName: section.name, screenName: screenName });
+		return true;
 	}
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied

packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx

@@ -1,7 +1,7 @@
 import Setting, { AppType, SettingItemSubType } from '@joplin/lib/models/Setting';
 import { themeStyle } from '@joplin/lib/theme';
 import * as React from 'react';
-import { useCallback, useId } from 'react';
+import { useCallback, useId, useState } from 'react';
 import control_PluginsStates from './plugins/PluginsStates';
 import bridge from '../../../services/bridge';
 import { _ } from '@joplin/lib/locale';
@@ -70,6 +70,7 @@ const SettingComponent: React.FC<Props> = props => {
 	const inputId = useId();
 	const descriptionId = useId();
 	const descriptionComp = <SettingDescription id={descriptionId} text={descriptionText}/>;
+	const [valueState, setValueState] = useState(props.value?.toString());
 
 	if (key in settingKeyToControl) {
 		const CustomSettingComponent = settingKeyToControl[key];
@@ -321,10 +322,9 @@ const SettingComponent: React.FC<Props> = props => {
 			);
 		}
 	} else if (md.type === Setting.TYPE_INT) {
-		const value = props.value as number;
-
 		const onNumChange: React.ChangeEventHandler<HTMLInputElement> = (event) => {
-			updateSettingValue(key, event.target.value);
+			setValueState(event.target.value);
+			updateSettingValue(key, Number.isInteger(Number(event.target.value)) ? event.target.value : ''); // Prevent invalid values being mapped to 0
 		};
 
 		const label = [md.label()];
@@ -336,7 +336,7 @@ const SettingComponent: React.FC<Props> = props => {
 				<input
 					type="number"
 					style={textInputBaseStyle}
-					value={value}
+					value={valueState}
 					onChange={onNumChange}
 					min={md.minimum}
 					max={md.maximum}

packages/app-mobile/components/screens/ConfigScreen/ConfigScreen.tsx

@@ -130,11 +130,13 @@ class ConfigScreenComponent extends BaseScreenComponent<ConfigScreenProps, Confi
 		const shouldSetIgnoreTlsErrors = this.state.changedSettingKeys.includes('net.ignoreTlsErrors');
 
 		const done = await shared.saveSettings(this);
-		if (!done) return;
+		if (!done) return false;
 
 		if (shouldSetIgnoreTlsErrors) {
 			await setIgnoreTlsErrors(Setting.value('net.ignoreTlsErrors'));
 		}
+
+		return true;
 	};
 
 	private syncStatusButtonPress_ = () => {
@@ -262,22 +264,21 @@ class ConfigScreenComponent extends BaseScreenComponent<ConfigScreenProps, Confi
 		return this.state.changedSettingKeys.length > 0;
 	}
 
-	private async promptSaveChanges(): Promise<void> {
+	private async promptSaveChanges(): Promise<boolean> {
 		if (this.hasUnsavedChanges()) {
 			const response = await shim.showMessageBox(_('There are unsaved changes.'), {
 				buttons: [_('Save changes'), _('Discard changes')],
 			});
 			if (response === 0) {
-				await this.saveButton_press();
+				return await this.saveButton_press();
 			}
 		}
+
+		return true;
 	}
 
 	private handleNavigateToNewScreen = async (): Promise<boolean> => {
-		await this.promptSaveChanges();
-
-		// Continue navigation
-		return false;
+		return !(await this.promptSaveChanges());
 	};
 
 	private handleBackButtonPress = (): boolean => {
@@ -300,8 +301,10 @@ class ConfigScreenComponent extends BaseScreenComponent<ConfigScreenProps, Confi
 
 		if (this.hasUnsavedChanges()) {
 			void (async () => {
-				await this.promptSaveChanges();
-				await goBack();
+				const result = await this.promptSaveChanges();
+				if (result) {
+					await goBack();
+				}
 			})();
 			return true;
 		}

packages/app-mobile/components/screens/ConfigScreen/SettingComponent.tsx

@@ -5,12 +5,11 @@ import { View, Text, TextInput } from 'react-native';
 import Setting, { AppType } from '@joplin/lib/models/Setting';
 import Dropdown from '../../Dropdown';
 import { ConfigScreenStyles } from './configScreenStyles';
-import Slider from '@react-native-community/slider';
 import SettingsToggle from './SettingsToggle';
 import FileSystemPathSelector from './FileSystemPathSelector';
 import shim from '@joplin/lib/shim';
 import { themeStyle } from '../../global-style';
-import { useId } from 'react';
+import { useId, useState } from 'react';
 
 interface Props {
 	settingId: string;
@@ -41,6 +40,7 @@ const SettingComponent: React.FunctionComponent<Props> = props => {
 	const containerStyle = props.styles.getContainerStyle(!!settingDescription);
 
 	const labelId = useId();
+	const [valueState, setValueState] = useState(props.value?.toString());
 
 	if (md.isEnum) {
 		const value = props.value?.toString();
@@ -92,33 +92,33 @@ const SettingComponent: React.FunctionComponent<Props> = props => {
 			/>
 		);
 	} else if (md.type === Setting.TYPE_INT) {
-		const unitLabel = md.unitLabel ? md.unitLabel(props.value) : props.value;
-		const minimum = 'minimum' in md ? md.minimum : 0;
-		const maximum = 'maximum' in md ? md.maximum : 10;
-		const label = md.label();
+		const label = md.unitLabel?.toString() !== undefined ? `${md.label()} (${md.unitLabel(md.value)})` : `${md.label()}`;
 
-		// Note: Do NOT add the minimumTrackTintColor and maximumTrackTintColor props
-		// on the Slider as they are buggy and can crash the app on certain devices.
-		// https://github.com/laurent22/joplin/issues/2733
-		// https://github.com/react-native-community/react-native-slider/issues/161
 		return (
-			<View key={props.settingId} style={styleSheet.settingContainer}>
-				<Text key="label" style={styleSheet.settingText} nativeID={labelId}>
-					{label}
-				</Text>
-				<View style={{ display: 'flex', flexDirection: 'row', alignItems: 'center', flex: 1 }}>
-					<Text style={styleSheet.sliderUnits}>{unitLabel}</Text>
-					<Slider
+			<View key={props.settingId} style={{ flexDirection: 'column', borderBottomWidth: 1, borderBottomColor: theme.dividerColor }}>
+				<View key={props.settingId} style={containerStyle}>
+					<Text key="label" style={styleSheet.settingText}>
+						{label}
+					</Text>
+					<TextInput
+						keyboardType="numeric"
+						autoCorrect={false}
+						autoComplete="off"
+						selectionColor={theme.textSelectionColor}
+						keyboardAppearance={theme.keyboardAppearance}
+						autoCapitalize="none"
 						key="control"
-						style={{ flex: 1 }}
-						step={md.step}
-						minimumValue={minimum}
-						maximumValue={maximum}
-						value={props.value}
-						onValueChange={newValue => void props.updateSettingValue(props.settingId, newValue)}
-						accessibilityHint={label}
+						style={styleSheet.settingControl}
+						value={valueState}
+						onChangeText={newValue => {
+							setValueState(newValue);
+							void props.updateSettingValue(props.settingId, Number.isInteger(Number(newValue)) ? newValue : ''); // Prevent invalid values being mapped to 0
+						}}
+						maxLength={15}
+						secureTextEntry={!!md.secure}
 					/>
 				</View>
+				{descriptionComp}
 			</View>
 		);
 	} else if (md.type === Setting.TYPE_STRING) {

packages/app-mobile/ios/Podfile.lock

@@ -1442,7 +1442,6 @@ DEPENDENCIES:
   - react-native-rsa-native (from `../node_modules/react-native-rsa-native`)
   - "react-native-saf-x (from `../node_modules/@joplin/react-native-saf-x`)"
   - react-native-safe-area-context (from `../node_modules/react-native-safe-area-context`)
-  - "react-native-slider (from `../node_modules/@react-native-community/slider`)"
   - react-native-sqlite-storage (from `../node_modules/react-native-sqlite-storage`)
   - react-native-version-info (from `../node_modules/react-native-version-info`)
   - react-native-webview (from `../node_modules/react-native-webview`)
@@ -1594,8 +1593,6 @@ EXTERNAL SOURCES:
     :path: "../node_modules/@joplin/react-native-saf-x"
   react-native-safe-area-context:
     :path: "../node_modules/react-native-safe-area-context"
-  react-native-slider:
-    :path: "../node_modules/@react-native-community/slider"
   react-native-sqlite-storage:
     :path: "../node_modules/react-native-sqlite-storage"
   react-native-version-info:

packages/app-mobile/package.json

@@ -32,7 +32,6 @@
     "@react-native-community/geolocation": "3.3.0",
     "@react-native-community/netinfo": "11.3.2",
     "@react-native-community/push-notification-ios": "1.11.0",
-    "@react-native-community/slider": "4.5.2",
     "assert-browserify": "2.0.0",
     "buffer": "6.0.3",
     "color": "3.2.1",

packages/lib/components/shared/config/config-shared.ts

@@ -4,7 +4,7 @@ const ObjectUtils = require('../../../ObjectUtils');
 const { _ } = require('../../../locale');
 import { createSelector } from 'reselect';
 import Logger from '@joplin/utils/Logger';
-
+import shim, { MessageBoxType } from '../../../shim';
 import { type ReactNode } from 'react';
 import { type Registry } from '../../../registry';
 import settingValidations from '../../../models/settings/settingValidations';
@@ -155,7 +155,11 @@ export const saveSettings = async (comp: ConfigScreenComponent) => {
 
 	const validationMessage = await settingValidations(savedSettingKeys, comp.state.settings);
 	if (validationMessage) {
-		alert(validationMessage);
+		await shim.showMessageBox(validationMessage, {
+			type: MessageBoxType.Error,
+			buttons: [_('OK')],
+		});
+
 		return false;
 	}
 

packages/lib/models/Setting.test.ts

@@ -456,4 +456,19 @@ describe('models/Setting', () => {
 			await Setting.saveAll();
 		}
 	});
+
+	test.each([
+		['1', 1],
+		['-1', -1],
+		['+1', 1],
+		['', null],
+		[null, 0],
+		[undefined, 0],
+		['1e3', 1000],
+		['1e20', 1e20],
+		['99999999999999999999', 1e20], // Value exceeds integer limit, output exhibits a rounding issue but still retains integer type
+	])('should format integer type setting as an integer or null', (async (input, expectedOutput) => {
+		const value = Setting.formatValue('revisionService.ttlDays', input);
+		expect(value).toBe(expectedOutput);
+	}));
 });

packages/lib/models/Setting.ts

@@ -810,7 +810,14 @@ class Setting extends BaseModel {
 	public static formatValue(key: string | SettingItemType, value: any) {
 		const type = typeof key === 'string' ? this.settingMetadata(key).type : key;
 
-		if (type === SettingItemType.Int) return !value ? 0 : Math.floor(Number(value));
+		if (type === SettingItemType.Int) {
+			if (value === '') {
+				// Set empty string as null instead of zero, so that validations will fail
+				return null;
+			} else {
+				return !value ? 0 : Math.floor(Number(value));
+			}
+		}
 
 		if (type === SettingItemType.Bool) {
 			if (typeof value === 'string') {

packages/lib/models/settings/settingValidations.test.ts

@@ -32,4 +32,42 @@ describe('settingValidations', () => {
 		}
 		expect(await settingValidations(['sync.target'], { 'sync.target': newSyncTargetId })).toBe('');
 	});
+
+	test('should return error message for null value for setting without range', async () => {
+		const value = await settingValidations(['style.editor.contentMaxWidth'], { 'style.editor.contentMaxWidth': null });
+		expect(value).toBe('Editor maximum width must be a valid whole number');
+	});
+
+	test('should return error message for null value for setting with range', async () => {
+		const value = await settingValidations(['revisionService.ttlDays'], { 'revisionService.ttlDays': null });
+		expect(value).toBe('Keep note history for must be a valid whole number');
+	});
+
+	test.each(
+		[0, -1],
+	)('should return error message for too low integer values', async (input) => {
+		const value = await settingValidations(['revisionService.ttlDays'], { 'revisionService.ttlDays': input });
+		expect(value).toBe('Keep note history for cannot be less than 1');
+	});
+
+	test.each(
+		[731, 1e20],
+	)('should return error message for too high integer values', async (input) => {
+		const value = await settingValidations(['revisionService.ttlDays'], { 'revisionService.ttlDays': input });
+		expect(value).toBe('Keep note history for cannot be greater than 730');
+	});
+
+	test.each(
+		[-999999999999999, 0, 999999999999999],
+	)('should return empty string for valid integer values for setting without range', async (input) => {
+		const value = await settingValidations(['style.editor.contentMaxWidth'], { 'style.editor.contentMaxWidth': input });
+		expect(value).toBe('');
+	});
+
+	test.each(
+		[1, 300, 730],
+	)('should return empty string for valid integer values for setting with range', async (input) => {
+		const value = await settingValidations(['revisionService.ttlDays'], { 'revisionService.ttlDays': input });
+		expect(value).toBe('');
+	});
 });