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.
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 a hybrid approach of restricting input and validating the input. When entering text in the input, any characters which are not 0-9 or - are removed from the input as you type, as any other character is not required for entering an integer value. Leading zero's are also removed from the value as you type, which is a consequence of mapping any valid value to the model as an integer on change, but this would not be disruptive for the user to enter a valid value. 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 order to ensure values outside of the min/max range cannot be saved, 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 integer between %s and %s" will be displayed for the first invalid value which was changed by the using, which indicates the name of the field and the min and max values. 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 realtime.
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, ensure any characters other than 0-9 and - are prevented from being entered in a number setting input (on desktop +, . and e can be entered as well, but upon entering a number after a decimal point results is the decimal being immediately removed, this is existing behaviour). You should be able to enter a negative number, should there be need for a setting with a negative value in future. On mobile, you see the character briefly because it gets removed, which is a limitation of react native
-Verify after changing a value of number setting, that the value can be used correctly but 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, saved 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, a non parsable integer value (eg. 5-5 or 5-), or a number outside the range of the settings min / max values (which defaults to 0 / 10 if not specified) set as it's value, the save should be prevented with an error alert "%s must be a valid integer between %s and %s". 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