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

Fix Android LineEdit editing bugs #38309

Merged
merged 1 commit into from
May 20, 2020

Conversation

SkyLucilfer
Copy link
Contributor

@SkyLucilfer SkyLucilfer commented Apr 28, 2020

Supersedes #38104

Fixes #27065
Fixes #38158

  • The first bug is solved by sending cursor_pos in LineEdit to the Android Java side, so that we consider only text until that cursor position. Fix virtual keyboard text not in sync with cursor position on LineEdit #38104 contains more explanation.

  • The second bug is solved by sending selection.begin and selection.end to the Android Java side, so that text selection in LineEdit is taken into account.

There are still some problems with updates in GodotEditText.java and GodotTextInputWrapper.java that I found during testing in some cases. Sometimes edit.setText(""); doesn't set the text, sometimes the pCharSequence in beforeTextChanged() gets out of synced, and I believe InputFilter sometimes doesn't get set too.
However during my tests this only happens when we have reached max length in LineEdit and we keep typing randomly, or when we spam clicking too fast on the LineEdit. I tried a few things but I couldn't solve this.

@SkyLucilfer SkyLucilfer requested a review from vnen as a code owner April 28, 2020 18:47
@YeldhamDev YeldhamDev added bug platform:android topic:gui topic:porting cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 28, 2020
@YeldhamDev YeldhamDev added this to the 4.0 milestone Apr 28, 2020
@akien-mga akien-mga requested a review from m4gr3d April 29, 2020 13:57
@akien-mga
Copy link
Member

CC @m4gr3d @pouleyKetchoupp

@pouleyKetchoupp
Copy link
Contributor

Nice work! It looks like a good improvement on the existing system.

I was able to consistently reproduce a bug which could be the one you mentioned, and I'm not sure if it should be considered as a separate issue or if there's a problem with this new code in GodotEditText::showKeyboard, so I think it would be worth checking with prints or something.

Here are my repro steps:

  • Make a LineEdit with max length 5
  • Type "Abcde"
  • Touch in the middle to move the cursor between 'c' and 'd'
  • Touch at the end to move the cursor back to the end
  • Type "ff", it shouldn't allow adding more letters but the result is "Abcff"

@SkyLucilfer
Copy link
Contributor Author

@pouleyKetchoupp Thank you very much for taking the time to test the code !!

The problem that you reproduced was due to the same bug that I mentioned - edit.setText("") doesn't reset EditText back to "abced".

After some more testing, I finally found out that it was due to setMaxInputLength() not being placed at the right location.
When we move cursor back to the end, the InputFilter in the EditText is still the InputFilter we used from last time. Hence it prevents us from setting the new text properly, and causing all the other problems afterwards.

Setting setMaxInputLength() at the right place will normally solve all of the problems I mentioned in the PR description.

I still notice some problems with InputFilter. I will push another PR soon.

@SkyLucilfer
Copy link
Contributor Author

SkyLucilfer commented Apr 30, 2020

TLDR : The LineEdit on Android should be working fine now.

===

There's still one last minor problem with the virtual keyboard.

In the following scenarios, after reaching max length on LineEdit, we will still be able to continue typing on the virtual keyboard (but the LineEdit is not affected) :

  • after a space. For example, "Ab |cde"
  • at the start of the line. "|Ab cde"
  • inserting words with virtual keyboard suggestions at the furthest end of the text

As far as I know, TextWatcher is not triggered when we type on the vk in these scenarios. I couldn't find out why. And I think because of this, the InputFilter doesn't filter the text on the vk in these cases.

Other than that, the LineEdit should be working well now.

@SkyLucilfer SkyLucilfer force-pushed the AndroidLineEdit branch 2 times, most recently from 3cbdb4b to 86834bd Compare May 1, 2020 00:47
@pouleyKetchoupp
Copy link
Contributor

Good job! This looks fine to me.

For the remaining issue with spaces and suggestions, it seems it's a general problem with the way the length filter works. We might be better off implementing it ourselves in the text watcher, as suggested here:
https://stackoverflow.com/questions/48869190/android-edittext-length-filter-not-working-as-it-should

Would you mind opening a new issue with your repro cases? I think this PR is already a great improvement but we'll have to solve these other problems eventually.

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved
scene/gui/line_edit.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit aec0753 into godotengine:master May 20, 2020
@akien-mga
Copy link
Member

Thanks!

@SkyLucilfer SkyLucilfer deleted the AndroidLineEdit branch May 20, 2020 10:05
@akien-mga
Copy link
Member

Cherry-picked for 3.2.2.

It's in #38832 as 2197ef0, I did my best to resolve conflicts but I didn't test the Android build/export, so I'd welcome some testing to ensure that it's working as intended.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 20, 2020
@SkyLucilfer
Copy link
Contributor Author

SkyLucilfer commented May 20, 2020

@akien-mga I will test it sometime around tommorow or Friday (if it's not too urgent).

@ondesic
Copy link

ondesic commented Oct 3, 2020

It should be noted that this same problem occurs with the TextEdit and needs to be fixed as well.

@ondesic
Copy link

ondesic commented Dec 19, 2020

Can this fix for the LineEdit be applied to the TextEdit please? In 3.2.4 beta 4 this same bug still exists.

@pouleyKetchoupp
Copy link
Contributor

Can this fix for the LineEdit be applied to the TextEdit please? In 3.2.4 beta 4 this same bug still exists.

The same fix was done for TextEdit in #43560, it will be part of the next beta.

@ondesic
Copy link

ondesic commented Apr 1, 2021

I can confirm that this bug still exists in the TextEdit node in 3.3 RC7.

@akien-mga
Copy link
Member

Please open a dedicated bug report then, this PR is merged, cherry-picked, and related to another node.

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