Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mobile: Resolves #10883: Allow accurate value to be set for note history and trashed item TTL #11069

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

mrjo118
Copy link

@mrjo118 mrjo118 commented Sep 17, 2024

The note history and trashed item TTL settings currently utilise a slider UI component to set their values on the mobile apps. As the value for these settings have quite large maximum values (730 and 300 respectively), it is not intuitive, nor is it possible to set an accurate value for these settings using a slider UI component for such a large value. In order to remedy this, I have created a new SettingItemType called BigInt, which is used exclusively for these 2 settings, in order to use a separate UI component on the mobile app.

On the desktop app, the UI component currently used for integer type settings is a text input, with the optional use of step up and step down buttons. It currently does not have the best validation, as it will allow you to enter 0 or a value greater than the maximum (and deleting the value immediately replaces it with a 0, instead of staying blank), and after saving and returning to the window, you find that it has been capped at the min or max value without warning. It does however prevent entering non-numeric characters in the input, including values pasted from the clipboard, which automatically get removed.

The new UI component has been implemented as a text input, similar to the desktop app. By setting the keyboardType on the input to numeric, the on-screen keyboard on mobile will use a layout catered to entering numbers, when tapping on the field. However, on Android the 'numeric' keyboard still includes a fair few non numeric characters as options as well (see https://davidl.fr/blog/keyboard-react-native-ios-android?utm_campaign=thisweekinreact&utm_medium=email&utm_source=Revue%20newsletter). Therefore it is also necessary to strip out non numeric characters, which can be done via an onChange handler. Unfortunately the onChange handler does not invoke instantly, so there is a small delay in which you see the character you entered being removed. But it does however serve it's purpose, and allow an accurate value to be entered while also sanitising the input to the same degree at which the desktop app does so.

I have attached 2 videos, one which demonstrates the new behaviour working on the mobile app, and the other which shows the existing behaviour on the desktop app (but it's demonstrated using this build of the code). I've also attached screenshots for both of the changed fields on the mobile app, showing what they look like in portrait and landscape.

I have therefore manual tested the change on both the desktop and mobile app, using Windows 10 and Android emulator. I don't have a Mac, so I have not tested the change on IOS, but everything on the new UI component should be supported on IOS as well from what I have read. I unfortunately don't have the know-how to write automated tests, as this is my first time working with TypeScript, but I worked on this as the fix itself looked simple enough to implement with barely any knowledge of TypeScript.

Link to the issue: #10883

Video (mobile):
https:/user-attachments/assets/bef8d15f-09f4-4de6-b87a-0cc39e2c7afd
Video (desktop):
https:/user-attachments/assets/fd92720c-d3da-4a1a-8b02-0a3de1a2f511

Screenshots:
history1
history2
trash1
trash2

…l, which allows entering the value accurately on the mobile app
Copy link
Contributor

github-actions bot commented Sep 17, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mrjo118
Copy link
Author

mrjo118 commented Sep 17, 2024

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Sep 17, 2024
@@ -114,6 +114,33 @@ const SettingComponent: React.FunctionComponent<Props> = props => {
</View>
</View>
);
} else if (md.type === Setting.TYPE_BIGINT) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

@laurent22
Copy link
Owner

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?

@tomasz1986
Copy link

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:

Untitled

Sliders are usually only used for a small range of numbers, e.g. if you need to select from 1 to 7 (e.g. days) or similar.

@mrjo118
Copy link
Author

mrjo118 commented Sep 18, 2024

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

@mrjo118
Copy link
Author

mrjo118 commented Sep 18, 2024

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?

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.mp4

What do you think?

@tomasz1986
Copy link

What 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.

@mrjo118
Copy link
Author

mrjo118 commented Sep 18, 2024

What 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.

@tomasz1986
Copy link

tomasz1986 commented Sep 18, 2024

Fair enough.

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.

@mrjo118
Copy link
Author

mrjo118 commented Sep 21, 2024

@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

@laurent22
Copy link
Owner

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

Yes, to prevent invalid values to get in.

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?

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)

@mrjo118
Copy link
Author

mrjo118 commented Sep 23, 2024

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

Yes, to prevent invalid values to get in.

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?

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?

@mrjo118
Copy link
Author

mrjo118 commented Sep 26, 2024

@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

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Sep 26, 2024

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). Removing all slider components would mean fewer patches to maintain. Edit: The issue seems to be fixed upstream! The fix was merged 3 days ago and hasn't yet been included in a release.

… item ttl, which allows entering the value accurately on the mobile app"

This reverts commit e55db5a.
@mrjo118
Copy link
Author

mrjo118 commented Oct 2, 2024

@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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants