search

SimpleMobileTools / Simple-Notes

A simple textfield for adding quick notes without ads.
https://www.simplemobiletools.com
GNU General Public License v3.0
826 stars 271 forks source link

Arrow Keys jump to URLs and do not move cursor #621

Closed Zocker1999NET closed 1 year ago

Zocker1999NET commented 1 year ago

This might be a regression of #136 & #178 solved by commit 696858e2bc345ed55d4d717cfade33123d774477.

I use LineageOS+microG and AnySoftKeyboard. Using the arrow buttons of AnySoftKeyboard or the feature of LineageOS to use the hardware volume keys as arrow keys do jump between the URLs available and do not move to cursor as expected.

App Version: 6.15.4 (at time of writing, the newest) LineageOS Version: 20-20230410-microG-FP4

tibbi commented 1 year ago

Im pretty sure that is the expected behaviour. Anyway, such things are handled by the system, not us.

Zocker1999NET commented 1 year ago

I wouldn't call that expected behavior. If I'm inserting text & having a blinking "text" cursor, I'm expecting that my arrow keys do move the "text" cursor & not jump in between elements.

However, I found out the exact conditions the issue is appearing (tested with version 6.15.5):

  1. Enable "Make links and emails clickable" in the app settings
  2. Creating a note with a link for testing the issue
  3. Fully restart the app to ensure that said link is recognized as such
  4. Reopen the note & verify that arrow keys are working as expected (moving text cursor)
  5. Now long press anything to trigger Android's sub menu
    • EDIT: It does not need to be the link which has to be selected, anything selecting works, making the app barely usable with this setting. However, the setting from point 1 needs to be enabled.
  6. Press anywhere in the text to give focus back to the input
  7. Now verify that the arrow keys (IMO falsely) do not move the text cursor but jump between/to the links

This is a screen record where I reproduce the issue:

https://github.com/SimpleMobileTools/Simple-Notes/assets/1645646/dd62f541-5981-4c92-91ba-37c6329d914d

I have no real experience in Android development specific, but to me it looks like the focus isn't correctly set to be only on the text input after the link was selected once. I cannot say if this might be an issue with the Android system, but this is the only app where I experience this. Anyway, I will keep my eyes open to find an app with a similar simple text UI & link recognizing to see if it experiences the same error.

tibbi commented 1 year ago

that is all done by the system, not us.

Zocker1999NET commented 1 year ago

Sorry, I value the huge work you put in this app, but I disagree with you. I've looked in-depth into the code & I found out the hotfix added in 1741070686d83745762bf285455aac985f10fca1 is causing that, with "Make links and emails clickable" enabled, the fix you introduced in 696858e2bc345ed55d4d717cfade33123d774477 for #136 is reverted at runtime in the case I presented above.

Your fix for #136 wants to use MyMovementMethod instead of LinkMovementMethod with linksClickable=true to solve it:

https://github.com/SimpleMobileTools/Simple-Notes/blob/148c9e58b822573d1b3649c1af7d17da76ca0398/app/src/main/kotlin/com/simplemobiletools/notes/pro/fragments/TextFragment.kt#L55-L61

And it does in my case, after a fresh launch, the arrow keys work fine.

But when selecting a text, this hotfix comes in to solve an issue in Android <=6 Google "couldn't" fix by enabling ArrowKeyMovementMethod when a selection is triggered to avoid a crash. It would eventually revert the hotfix when the text is deselected, but it reverts it to LinkMovementMethod, ultimately reverting your work in 696858e2bc345ed55d4d717cfade33123d774477 at runtime:

https://github.com/SimpleMobileTools/Simple-Notes/blob/148c9e58b822573d1b3649c1af7d17da76ca0398/app/src/main/kotlin/com/simplemobiletools/notes/pro/activities/MainActivity.kt#L256-L258

So I concluded that changing line 257 to use MyMovementMethod and not LinkMovementMethod should fix the issue I have, and after testing I see that it does fix my issue. I've created PR #634 to propose this change.

Alternatively, or rather additionally, the hotfix may be guarded to be only applied on Android <=6 devices (like proposed here), to avoid further hassle with this workaround where it is not needed, as it feels kind of "hacky" and hopefully is only required on such old Android versions (However, that requires further testing with older devices at least I cannot do, that's why I didn't add this to my PR).