Mobile: Resolves #10883: Remove slider component module and replace integer settings with validated text inputs and ensure all validation error flows work correctly #11069
Conversation
…l, which allows entering the value accurately on the mobile app
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| @@ -114,6 +114,33 @@ const SettingComponent: React.FunctionComponent<Props> = props => { | |||
| </View> | |||
| </View> | |||
| ); | |||
| } else if (md.type === Setting.TYPE_BIGINT) { | |||
There was a problem hiding this comment.
Rather than adding a new type (Setting.TYPE_BIGINT), it could make sense to define a new subtype. See, for example, how the FontSearch text input type is implemented .
|
Thanks for creating this change. It needs to have the same quality of validation as the slider since we don't want invalid values to get in. As personalizedrefrigerator mentioned I'd also rather not have a new type. Why not detect what should be displayed depending on the min/max value? But also I'm not sure this is the right fix and that probably should be discussed before spending too much time on implementation and review. How do other mobile apps handle large number input? |
From my experience, apps either show a pre-defined list of options to select from, so that the user doesn't need to input anything manually, or they use the same input as in this PR. For example, the following screenshot comes from Google Calendar:
Sliders are usually only used for a small range of numbers, e.g. if you need to select from |
|
I don't personally like the idea of automatically changing the component based on the max size of a number setting, as whether it is needed also depends how long the bar is rendered in portrait mode on mobile, and that will depend on the length of the labels used. I think it is better to leave that in the control of the developer. But using a subtype instead is certainly a logical suggestion. Regarding the quality of the validation, what about if I gave the 2 settings a new subtype, and for that scenario on mobile, make it use the same component as an enum, with the values generated as a range between the min and max values? That way the validation stays solid, and although the list will be fairly big, the component supports variable scrolling speed, so you can scroll from min to max value in not that long. I have attached a video of a quick POC for an enum number range, demonstrating how this behaves in the UI (I scrolled through the list by swiping with the mouse, rather than using the scroll wheel on the mouse). Bandicam.2024-09-18.12-50-42-996.mp4 |
I've now come up with and drafted another solution for dealing with large numbers. We can use a slider component on mobile still, but additionally have decrement and increment buttons on either side of it. In order to cater for the extra space taken for the buttons (which further reduce the width of the slider), the selected value can be appended to the label's view on a new line, in brackets. I have added validation to the buttons to prevent surpassing the min and max values, and they work without any graphical defects while providing the same quality of validation as existing. You might consider this solution can be used for all slider components on mobile, or it can just be limited to settings which deal with large numbers like this. Bandicam.2024-09-18.18-51-06-348.mp4What do you think? |
Honestly, the previous version with the dropdown list seems way more usable. The slider in the current version with this range of numbers is completely useless, especially on phone screens. Maybe on larger tablets it may have some utility. |
Fair enough. I guess then it's a question of whether the pop-in you get when scrolling really fast is acceptable. This looks to be a trade-off of using a FlatList facebook/react-native#23442 , but at least the large amount of values is not going to be a performance issue, when using this component with a low initialNumToRender value. Another alternative would be to implement a searchable dropdown menu via a third party library such as this https://www.npmjs.com/package/react-native-element-dropdown . That also is common approach (combined with lazy loading) for handling fairly large lists I think. But it's more commonly used for multi selects, and frankly seems a bit overkill for an ordered list of numbers. |
Just for the record, by "current version" of the slider I meant what we've currently got in Joplin, not necessarily your implementation above. A slider with buttons would of course be much better than the current slider with no buttons, but still, you would likely have to press those buttons at least tens of times in order to actually get to the expected value, which doesn't sound like great user experience. |
|
@laurent22 could you clarify if by "quality of validation" you just mean that is needs to prevent invalid values getting in, or also the fact that for the TextInput solution you can see the invalid character briefly before being removed, which might give the impression of the app looking buggy? I'm guessing this bug / limitation is considered a low priority issue, but if you are not satisfied with the behaviour shown for my TextInput or DropDown solutions due to the latter point, would you consider my decrement and increment button solution, as an interim solution to at least solve the inability to enter an accurate value for the ttl settings on mobile, until a more long term solution is decided for a better user experience? With regards to using a sub type for large numbers vs automatically changing the widget depending on the range of numbers, I'd be confident to implement either of those solutions myself. I would prefer opting to use a subtype, but if you did have a stronger preference to go with dynamically using the modified widget based on the number range, I would propose to also move the selected value onto the label side of the row for normal slider widgets which don't need to include the decrement and increment buttons, as is shown in the latest video I attached. The reason for this would be that the slider width will then always be consistent, and wont vary in size, depending on the length of the unit text where specified |
Yes, to prevent invalid values to get in.
I'm not a fan of it to be honest as I don't feel it helps that much, and it doesn't exactly look good (we could probably fix this but I don't feel it's worth the time) |
Thanks for getting back on that. As your original concern is just preventing invalid values getting in, using the text input originally implemented in this PR should be sufficient, if I extend the onChangeText behaviour to replace the value with the min value or max value when the value is out of bounds (in a similar way to how the desktop app substitutes the value with 0 when removing all characters, which I replicated in this PR). Regarding not liking how the increment and decrement solution looks, ok fair enough. Although I could change the styling of the buttons if that helps (maybe remove the blue boxes and just show the plus and minus characters in black?) With regards to the final point about this being not worth fixing: there's at least 2 other people interested in a fix, based on likes on my bug report. But from my perspective I consider it worth fixing because the larger the note history ttl, the larger the number of objects are stored at the sync source. My use case is I use the android app daily, but I only use the desktop version rarely, so with a large note history, a sync after some months will have a lot to catch up on, because I make a lot of brief note edits every day (and I can only view note history on the desktop version in case of accidental data loss). On android the note history ttl value is basically limited to the default 90 days, 1 day, 730 days, or a completely random value in between, because you can't consistently move it to the same position using your finger (even with an external keyboard, using the arrow keys skip over many values). I'd like to be able to have more flexibility, to choose a good compromise between a large amount of objects to sync and a bit of leeway to retain note history more than a day. I understand you might not think the fix is necessary, but I am offering to fix the issue myself. Would you consider to be acceptable any of the solutions I have proposed on this PR thread, or some variation of them? |
|
@laurent22 I have yet another idea. If you absolutely don't want the aesthetics of the configuration screens to be changed, I could simply add an event handler when you tap the value (i.e. the displayed number and unit, where specified) of any slider component, which will increment the value except when the max value has been reached. That will be a minimal code change which will provide a means to solve the issue that is at least better than what we currently have |
|
It could alternatively make sense to remove all slider components for a similar UI to the desktop app. At present, we're patching the slider library to prevent a crash on certain Android devices when opening settings (see #7974). |
… item ttl, which allows entering the value accurately on the mobile app" This reverts commit e55db5a.
…with validation for exceeding the max value
I have pushed up this change now in case you would consider it. Here is a video which shows the behaviour: Bandicam.2024-10-02.13-47-28-743.mp4 |
|
@laurent22 Is there any chance of this cut down code change getting merged? Note history and trash ttl are pretty much set and forget, so not a big deal if the user experience isn't great (but still better than it is now anyway) and it would at least allow those settings to be set consistently across mobile and desktop. |
I wouldn't mind a text input only but it needs to be validate the value, like the slider currently does. I'm not sure how easy it is to do though |
…ted text inputs and ensure all validation error flows work correctly
… screen prompt so it will autosave
…ry-and-trash-ttl-text-input
mrjo118
changed the title
Thanks for making a decision on this. There is actually an existing validation mechanism on save of settings which I could work with. However it was a bit of a can of worms as there were a number of issues I found with it, which may or may not have ever surfaced depending on how it was possible to get the existing validation error currently. I have pushed up some changes to implement as described, which includes a number of other changes to facilitate the validation flows working properly as well. I've updated the original description of the PR with the new details |
This reverts commit 3d7e0c9.
…b.com/mrjo118/joplin into note-history-and-trash-ttl-text-input
| accessibilityHint={label} | ||
| style={styleSheet.settingControl} | ||
| value={value} | ||
| onChangeText={newValue => props.updateSettingValue(props.settingId, newValue?.replace(/[^0-9-]/g, ''))} |
There was a problem hiding this comment.
If there is already validation when saving, we don't need this real time change
|
|
||
| if (newValue === '' || isNaN(newValueInt) || newValueInt > maximum || newValueInt < minimum) { | ||
| return _('%s must be a valid integer between %s and %s', md.label(), minimum, maximum); |
There was a problem hiding this comment.
| return _('%s must be a valid integer between %s and %s', md.label(), minimum, maximum); | |
| return _('%s must be a valid number between %s and %s', md.label(), minimum, maximum); |
There was a problem hiding this comment.
If you don't like use of the word integer, would this be better?
%s must be a valid whole number
| const validateInteger = async (key: string, newValue: string) => { | ||
| const md = Setting.settingMetadata(key); | ||
| const minimum = 'minimum' in md ? md.minimum : 0; | ||
| const maximum = 'maximum' in md ? md.maximum : 10; |
There was a problem hiding this comment.
If maximum is not specified it should probably not be restricted
| 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)); |
There was a problem hiding this comment.
The value is apparently formatted here and the checks are done on that value, but then that's not what we're actually saving to the database? For me these tests are irrelevant if we're testing against one value and saving a different value in the end
There was a problem hiding this comment.
This comment it pivotal for most of your other comments, so I'll just answer this one.
The reason for formatting on validation is because in order to properly validate the input, you need to first store the input as a string. If for example you enter 1.2, if you used the integer type, then it will truncate the decimal in Setting.formatValue and store it as 1, rather than failing. So because we set the value on change, but validate on save, you kind of have to use a different type for set and validate to do a thorough validation.
I could set the value as an integer in both set and validate if that's what you would like. But bear in mind that is going to reduce the quality of the validation. As it will truncate a decimal instead of raising a validation error. As per your comment on the error message, the reason I used the word 'integer' instead of 'number', is because entering the value 1.2, then getting the error '1.2 must be a valid number...' is going to be confusing for the user, because that would be a valid number.
There was a problem hiding this comment.
Ok, I've managed to find a workaround for this. If I check if the value is an integer before updating the setting, I can force it to a null value in order to make it fail the validation and set the value on the component using useState in order to prevent invalid values changing as you enter them. It just means if you leave the screen and come back into it, you can't restore the original state value if it is not a parsable integer, but I think in that scenario it is fine to just set the state value in the field to an empty string
| const newValue = newValues[settingName]; | ||
|
|
||
| // Type based validations | ||
| if (md.type === SettingItemType.Int && !md.isEnum) { |
There was a problem hiding this comment.
Why doesn't the oldValue === newValue check not relevant for integer values? I'm not keen on these complicated and unexplained if/else statements with early returns.
|
@laurent22 All feedback should now be addressed. I've updated the testing scenarios in the description as well |
On mobile, a react native slider component is used where integer values are entered as a configuration setting. This is not user friendly where the value is a large number. The desktop app instead uses a text input with the number type, which allows restricted characters to be entered and it has increment and decrement buttons which appear when hovering over (which are capped at the min and max values for the setting). In order to improve usability and have consistency between mobile and desktop, I have removed the react-native-community/slider component entirely from the project, and used TextInput components to replace them.
As a integer type settings need to prevent invalid values from being entered, I have implemented validation of save. Additionally, where unitLabel is specified for a configuration setting, on desktop the default value plus the unit value is appended to the field label in brackets. I have mimicked this behaviour on mobile as well
In addition to validating that invalid values cannot be entered, where min / max values are specified for a setting, those limits will be enforced by the validation. The validation has been added on save for all integer type setting inputs, which is triggered either manually by pressing the button, exiting a screen and being prompted, or changing an auto save value. In all of these cases, an error message in the format "%s must be a valid whole number" / "%s cannot be less than %s" / "%s cannot be greater than %s", will be displayed for the first invalid value which was changed by the user, which indicates the name of the field, the min or max value. This adds the benefit of providing an indication to the user of the valid value range, where it is not made apparent to the user. This builds upon the existing validation mechanism which was implemented for changing the sync target. However, the existing implementation did not correctly handle the flows for preventing exit of the relevant screen if the user confirmed on a save prompt that they wanted to save pending changes, but that save failed. In order to facilitate the settings exit flows correctly handling error scenarios, a number of additional changes were required for both mobile and desktop, including using shim to create an alert instead of a standard javascript alert where the code is shared between mobile and desktop, to avoid this issue electron/electron#31917
Note that on the desktop app, number range validations are poorly handled currently, as they would allow saving a value outside of the min / max values if the value is entered as free text. In such cases, when you reload the settings config, the values are capped at the min / max value despite the user not receiving a warning that it was the case. The desktop flow has now been improved, to also show a validation error when the value is outside of the min / max range. Additionally on desktop, when the user removes the existing value of a number input using backspace, the value automatically changes to the value 0, which is misleading to the user. This has been changed to now allow a blank value to be displayed in the input when it is cleared out, which will throw a validation error if attempting to save with a blank value. The mobile app has been aligned to have this behaviour as well. On desktop, when entering the value as free text there is no limit to the number of characters you can enter. This turned out to be difficult to fix, due to limitations of the React number type text inputs, so I have not attempted to change this behaviour. However on mobile, I have set number inputs to have a hard limit of 15 characters, to avoid the integer limit being exceeded, which causes a change to the input value in real time.
Testing
These are scenarios which I have covered with manual testing on mobile and desktop:
-For Settings > Appearance > Editor font size, ensure the label without any unit or brackets are set as the label
-For Settings > Note History > Keep note history for, ensure the label plus a space followed by the unit in brackets is set as the label
-The user should be able to clear out a number setting input using backspace, without the value being replaced with 0 when doing so on mobile and desktop. On desktop, any characters which cannot be used as part of a number of prevented from being entered, while on mobile, a number type on screen keyboard will pop up, but any character can be entered into the input, which will be validated on save
-Verify after changing a value of number setting, that the value can be used correctly by the code the uses it. An easy example to test is that changing editor font size is reflected when opening a note in edit mode
-Verify after changing a value of a non number setting, such as a checkbox and dropdown, it saves without issues in combination with changing the value of a number setting
-On mobile, the user should not be able to enter more than 15 characters in a number setting input
-When saving configuration settings, where any number setting has the value set to blank, or a non parsable integer value (eg. '5-5' or '5-' or 'aaa' or any decimal value outside of all trailing zeros eg. '5.0') set as it's value, the save should be prevented with an error alert "%s must be a valid whole number". Where any non parsable integer value is entered by the user, if they navigate to a different screen and then return to the same on, the value will be cleared to prevent a NaN string literal being shown in the inputs, and will instead be a blank value which will continue to fail validation
-When saving configuration settings, where any number setting has the value lower than the minimum value where the setting has one, the save should be prevented with an error alert "%s cannot be less than %s"
-When saving configuration settings, where any number setting has the value higher than the maximum value where the setting has one, the save should be prevented with an error alert "%s cannot be greater than %s"
-In a number setting input, you should be able to enter a negative number. On Desktop, in Settings, Appearance, Editor maximum width, this field allows entering negative values and there should be no restriction for the lower and upper limit
-If there are multiple invalid values, only one error alert should display (this seems to be the first value which was changed to an invalid value). After resolving one value, attempting to save again should raise an error alert for the other value, and once that is resolved then save should work. On desktop, confirming the alert should not leave text inputs in a disabled state (however, I did find the last input has lost focus in my testing)
-On desktop, clear out the value of a number setting input, then click switch to a setting screen which prompts the switching screen prompt (eg. Web Clipper and Encryption screens). Clicking cancel should prevent the screen from switching (existing behaviour for this is it switches screen on clicking cancel, I have fixed this), clicking ok should raise an error alert about the field which was cleared and prevent the screen from switching. Clicking ok or cancel with and without an error should not leave text inputs in a disabled state. After setting a valid value in that number setting input, switching screen again and clicking ok should result in the screen switching to the new one and the new value being successfully saved.
NOTE: the plugins screen has been added to the screens which trigger the switching screen prompt, in order to ensure changes are saved before entering this screen. Otherwise when toggling a plugin, the message to restart for changes to take effect will be misleading if the user has not successfully saved the plugin change
-On desktop if you navigate around settings screens which don't trigger the switch screen prompt while there are unsaved changes, the back button should say cancel, and clicking it should discard unsaved changed without a prompt. This is the existing behaviour and should remain unchanged
-On mobile, clear out the value of a number setting input, then attempt to exit the top level settings screen (verify using both the back button in the top bar of the app, and using the device back button). Choosing discard changes should prevent the screen from switching and choosing save changes should raise an error alert about the field which was cleared and prevent the screen from switching. After setting a valid value in that number setting input, switching screen again and clicking save changes should result in the screen switching to the new one and the new value being successfully saved. The behaviour should be the same when going to Settings > Tools > Manage Profiles, or Settings > Synchronisation > Encryption Config, which prompts a switching screen prompt in desktop
-On mobile, verify that the save is blocked by a validation error where the value of a number setting input has an invalid value and then an auto save field has the value changed. Examples of this are going to Settings > Plugins > Enable / disable plugin support, and enabling / disabling biometrics in Settings > General, after setting up biometric data on the device. In both cases, toggling the option will trigger a save immediately, and the validation error should be displayed. This should leave the settings screen in a state where there are unsaved changes pending. On mobile you will still be prompted to save unsaved changes when trying the exit the top level settings screen and have the option to save and discard after failing to save when the auto save triggered, nor is there any prompt to restart the app for setting changes which require a restart, so this would not cause flow issues that would happen on the desktop app if the auto save was unable to complete.
Videos are attached showing a number of these test scenarios, following the code changes:
Mobile (Android):
https://github.com/user-attachments/assets/5cad51fb-a6c6-4f75-8b3e-eb406787f805
Desktop (Windows):
https://github.com/user-attachments/assets/0a08d6fe-fb49-4af6-9661-833898d74d67