diff --git a/packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx b/packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx index 875a36194..cdd732327 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx +++ b/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 => { const inputId = useId(); const descriptionId = useId(); const descriptionComp = ; + const [valueState, setValueState] = useState(props.value?.toString()); if (key in settingKeyToControl) { const CustomSettingComponent = settingKeyToControl[key]; @@ -321,10 +322,9 @@ const SettingComponent: React.FC = props => { ); } } else if (md.type === Setting.TYPE_INT) { - const value = props.value as number; - const onNumChange: React.ChangeEventHandler = (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 => { const containerStyle = props.styles.getContainerStyle(!!settingDescription); const labelId = useId(); + const [valueState, setValueState] = useState(props.value?.toString()); if (md.isEnum) { const value = props.value?.toString(); @@ -91,7 +92,6 @@ const SettingComponent: React.FunctionComponent = props => { /> ); } else if (md.type === Setting.TYPE_INT) { - const value = props.value?.toString(); const label = md.unitLabel?.toString() !== undefined ? `${md.label()} (${md.unitLabel(md.value)})` : `${md.label()}`; return ( @@ -109,8 +109,11 @@ const SettingComponent: React.FunctionComponent = props => { autoCapitalize="none" key="control" style={styleSheet.settingControl} - value={value} - onChangeText={newValue => props.updateSettingValue(props.settingId, newValue?.replace(/[^0-9-]/g, ''))} + 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} /> diff --git a/packages/lib/models/Setting.test.ts b/packages/lib/models/Setting.test.ts index 083b60cd7..772cdbdf2 100644 --- a/packages/lib/models/Setting.test.ts +++ b/packages/lib/models/Setting.test.ts @@ -459,37 +459,16 @@ describe('models/Setting', () => { test.each([ ['1', 1], - ['1.5', 1], ['-1', -1], ['+1', 1], - ['', ''], - ['aaa', 'aaa'], - ['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 will fail max value validation on save - ])('should format integer type setting as an integer if it is a number that can be parsed, otherwise retain a string value for validation on save', (async (input, expectedOutput) => { + ['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); })); - - test.each([ - ['1', 1], - ['1.5', 1], - ['-1', -1], - ['', 0], - ])('should always format integer type setting as an integer when it is an enum', (async (input, expectedOutput) => { - const value = Setting.formatValue('sync.target', input); - expect(value).toBe(expectedOutput); - })); - - test.each([ - ['1', 1], - ['1.5', 1], - ['-1', -1], - ['', 0], - ])('should always format integer type setting as an integer key is a SettingItemType', (async (input, expectedOutput) => { - const value = Setting.formatValue(SettingItemType.Int, input); - expect(value).toBe(expectedOutput); - })); }); diff --git a/packages/lib/models/Setting.ts b/packages/lib/models/Setting.ts index 83bd5fa47..0e1928470 100644 --- a/packages/lib/models/Setting.ts +++ b/packages/lib/models/Setting.ts @@ -811,9 +811,9 @@ class Setting extends BaseModel { const type = typeof key === 'string' ? this.settingMetadata(key).type : key; if (type === SettingItemType.Int) { - if (typeof key === 'string' && !this.settingMetadata(key).isEnum) { - // Retain invalid values as a string, so they can be validated on save without modifying the user input as the user types - return value !== '' && !isNaN(value) ? Math.floor(Number(value)) : value; + 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)); } diff --git a/packages/lib/models/settings/settingValidations.test.ts b/packages/lib/models/settings/settingValidations.test.ts index c8fd5f9f0..b6f522213 100644 --- a/packages/lib/models/settings/settingValidations.test.ts +++ b/packages/lib/models/settings/settingValidations.test.ts @@ -33,16 +33,40 @@ 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, '1-1', 'aaa', 731, 1e20], - )('should return error message for invalid integer values', async (input) => { + [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 must be a valid integer between 1 and 730'); + 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', async (input) => { + )('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(''); }); diff --git a/packages/lib/models/settings/settingValidations.ts b/packages/lib/models/settings/settingValidations.ts index eaae11977..7728b5b33 100644 --- a/packages/lib/models/settings/settingValidations.ts +++ b/packages/lib/models/settings/settingValidations.ts @@ -40,14 +40,21 @@ const validations: Record = { }, }; -const validateInteger = async (key: string, newValue: string) => { +const validateInteger = async (key: string, newValue: number) => { const md = Setting.settingMetadata(key); - const minimum = 'minimum' in md ? md.minimum : 0; - const maximum = 'maximum' in md ? md.maximum : 10; - const newValueInt = Math.floor(Number(newValue)); + const minimum = 'minimum' in md ? md.minimum : null; + const maximum = 'maximum' in md ? md.maximum : null; - if (newValue === '' || isNaN(newValueInt) || newValueInt > maximum || newValueInt < minimum) { - return _('%s must be a valid integer between %s and %s', md.label(), minimum, maximum); + if (newValue === null || isNaN(newValue)) { + return _('%s must be a valid whole number', md.label()); + } + + if (maximum !== null && newValue > maximum) { + return _('%s cannot be greater than %s', md.label(), maximum); + } + + if (minimum !== null && newValue < minimum) { + return _('%s cannot be less than %s', md.label(), minimum); } return ''; @@ -58,14 +65,12 @@ const validateSetting = async (settingName: string, newValues: Record