self-hosted/joplin
1
0
Fork 0
mirror of https://github.com/laurent22/joplin.git synced 2024-12-24 10:27:10 +02:00
Code Issues Releases Activity

Change formatting of values back to always using integer type and address other PR feedback

Browse Source
This commit is contained in:
Josh C 2024-12-10 01:18:48 +00:00
parent 5428a45d84
commit 2595f61ec8
6 changed files with 63 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx
View File

@ -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}

11
packages/app-mobile/components/screens/ConfigScreen/SettingComponent.tsx
View File

@ -9,7 +9,7 @@ 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;
@ -40,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();
@ -91,7 +92,6 @@ const SettingComponent: React.FunctionComponent<Props> = 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> = 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}
/>

31
packages/lib/models/Setting.test.ts
View File

@ -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);
}));
});

6
packages/lib/models/Setting.ts
View File

@ -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));
}

32
packages/lib/models/settings/settingValidations.test.ts
View File

@ -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('');
});

25
packages/lib/models/settings/settingValidations.ts
View File

@ -40,14 +40,21 @@ const validations: Record<string, ValidationHandler> = {
},
};
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<string, an
const md = Setting.settingMetadata(settingName);
const oldValue = Setting.value(settingName); // Needs to be set this way, rather than from Setting.settingMetadata
const newValue = newValues[settingName];
if (oldValue === newValue) return '';
// Type based validations
if (md.type === SettingItemType.Int && !md.isEnum) {
if (oldValue?.toString() === newValue?.toString()) return '';
const message = await validateInteger(settingName, newValue);
const message = await validateInteger(settingName, newValue as number);
if (message !== '') return message;
} else {
if (oldValue === newValue) return '';
}
// Custom validations