Skip to content

Conversation

javieranton-zz
Copy link
Contributor

@javieranton-zz javieranton-zz commented Dec 2, 2020

I know this is very specific to Android, but that's because:

  1. Android is the only platform where the keyboard can't currently be kept open while clearing text. On iOS, it's as simple as setText("")
  2. Android's InPlaceEditView static method public static void stopEdit(), which stops editing without closing the keyboard, isn't exposed in the interface implementation so calling it from Display (to avoid having to call it natively) isn't currently possible

Feel free to create a wrapper to call the 4 points in the comments (and then remove the comments). I tried doing this but then as I went along I felt very confident that you would object to my implementation, so it's best CN1 does this

For now, can we please have this merged so I can call this from my side? It will fix a big source of pain

@javieranton-zz
Copy link
Contributor Author

@shai-almog hi... just wondering, will this be merged before Friday? Thx

@shai-almog
Copy link
Collaborator

This looks identical to #3318

@javieranton-zz
Copy link
Contributor Author

javieranton-zz commented Dec 3, 2020

I added the curly braces as requested. Can you please ask me what isn't understood of this change? I will repeat again the purpose of this: to provide a way to clear TextAreas on Android that doesn't fold/toggle the keyboard. If CN1 wants it can create a wrapper that does what the 4 points in the change's comments mention, but without this small change keeping the keyboard on while clearing is currently impossible. I have tested this on my end by replicating an edited version of InPaceEditView and it works fine. But the change needs to go into CN1's InPaceEditView. It's a super-small change that won't affect regression in any way because the flag is false by default. Please, In know you might be busy, but spend a couple minutes on this as forcing to toggle the keyboard is a death blow to many applications (chat applications have no hope without this)

@shai-almog
Copy link
Collaborator

You addressed the minor part of my comment. My problem with the previous PR is that it's a PR strictly to enable a hack. If this isn't exposed in a couple of weeks we'll forget this hack exists and will write code that breaks it.
To make sure a feature works it needs to be exposed to users so they can use it. If this is a better way to "clear" without folding the keyboard then why isn't this the default for the clear method?

I think you might have assumed the keyboard folding is something we wanted?

@javieranton-zz
Copy link
Contributor Author

javieranton-zz commented Dec 3, 2020

I can expose the new "better?" way if you want, I can create a PR for that. Does that sound good? It will strictly be for Android/iOS as I haven't dealt with any other platforms before (nor can I build into them I think)
I'm not sure how to call native code from Display (never done this before) but I shall try

@shai-almog
Copy link
Collaborator

Sure, but why not add it to this PR?
you can commit to the branch and push to this PR.

@javieranton-zz
Copy link
Contributor Author

ok, will do

@shannah
Copy link
Collaborator

shannah commented Dec 3, 2020

I think the right way to do this would be to "bind" the native text field contents to the "CN1 text field" contents, perhaps using a DataChangedListener so that if setText(sometext) is called on the CN1 textfield while the native editor is opened, it will automatically update the value in the native editor. Care just needs to be take to prevent cycles since.

Right now it is only a one-way binding (Native -> CN1 - i.e. when the Native textfield is edited, it propagates changes to the Cn1 text field, but not vice versa).

Creating a method to stopEditing without closing the keyboard is a can of worms.

@javieranton-zz
Copy link
Contributor Author

Ok, I have added the new TextField clear(hideKeyboard).

I know this looks hacky, and it is. The current implementations is also hacky, so in the interest of not affecting regression this might be the safest way to achieve this.

I have thoroughly tested this (I've spent the last week just trying to get on top of this). As @shannah says changing the TextField's content from CN1 is very buggy. Right now the only way to achieve this is this way, and it works. It calls stopEditing in a way that doesn't close the keyboard (which was already available, although not exposed) so that the underlying Android logic is cleared and it starts editing again without re-opening the keyboard. It works, and as I said I tested this a lot

@shai-almog
Copy link
Collaborator

@javieranton-zz I'm moving this issue to @shannah as he has strong strong feelings about this and a lot of experience in this specific area.

@javieranton-zz
Copy link
Contributor Author

Sure, makes sense

@javieranton-zz
Copy link
Contributor Author

javieranton-zz commented Dec 3, 2020

@shannah pls note that TextField isn't gaining a new stopEditing method that keeps the keyboard open, only a clear(toggleKeyboard) that just re-sets both CN1's and native textfield's text boxes to empty in a way that the keyboard isn't toggled. On Android, the keyboard is actually refreshed (for example, if it is currently showing emojis it goes back to text after calling this new method) but it doesn't fly up and down the screen. This is the same behavior as for example in Whatsapp

@javieranton-zz
Copy link
Contributor Author

I know we're all super busy, but it would be great if we could start using this with today's update. I understand if that's not possible, but asking is free :)

@javieranton-zz
Copy link
Contributor Author

@shannah hate to put pressure on anyone, but this is a bit of a blocker for me. Can the PR be reviewed/accepted? Or alternatively, can an estimate be given for how long this will take? Thanks

shannah added a commit that referenced this pull request Jan 4, 2021
…e native android editor while editing is in progress so that changes to the textarea contents will be reflected during editing. This is an alternative solution to the one presented in #3319 which added a special method to clear the textfield contents without closing the keyboard.

This solution is more generalized (as you can clear the text field, or set its contents to any other string), and it doesn't require any API changes. It improves consistency with iOS since iOS already behaves this way.
@shannah
Copy link
Collaborator

shannah commented Jan 4, 2021

I have just committed an alternate fix for this issue.
cdaaf8e

This will be available in the next update on Friday.

Summary of change (pasted from commit comments):

Added DataChangedListener to bind text in lightweight text area to the native android editor while editing is in progress so that changes to the textarea contents will be reflected during editing. This is an alternative solution to the one presented in #3319 which added a special method to clear the textfield contents without closing the keyboard.

This solution is more generalized (as you can clear the text field, or set its contents to any other string), and it doesn't require any API changes. It improves consistency with iOS since iOS already behaves this way.

@shannah shannah closed this Jan 4, 2021
@javieranton-zz
Copy link
Contributor Author

Nice one, I will thoroughly test it come Friday. Thanks a lot :)

@javieranton-zz
Copy link
Contributor Author

javieranton-zz commented Jan 8, 2021

Works well, just one thing:

On Android, with the new code, if the following is done:

txtField.setText("A");
txtField.setText("B");

The text field isn't guaranteed to show "B". The listener gets executed asynchronously and sometimes the first instruction will come after the second, resulting in "A"

It's not a huge deal but it might break existing code. I know I will have to change mine to prevent issues

@shai-almog
Copy link
Collaborator

@all-contributors please add @javieranton-zz for code

@allcontributors
Copy link
Contributor

@shai-almog

I've put up a pull request to add @javieranton-zz! 🎉

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.

3 participants