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

[TIMOB-25655] Fix Android: Android: Set textfield value by coding will make cursor to the beginning of textfield #9710

Merged
merged 7 commits into from Feb 6, 2018

Conversation

maggieaxway
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-25655

Test case:

var window = Ti.UI.createWindow({ layout: "vertical" });
var textField = Ti.UI.createTextField(
{
   value: "Hello World!",
   width: "90%",
   top: "5%",
});
textField.addEventListener("change", function(e) {
   var message = "TextField 'change' event received.\n\"" + textField.value + "\"";
   Ti.API.info(message);
   if ((Ti.Platform.name === "android") || (Ti.Platform.name === "windows")) {
      var notification = Ti.UI.createNotification(
      {
         message: message,
         duration: Ti.UI.NOTIFICATION_DURATION_SHORT,
      }).show();
   }
});
window.add(textField);
var changeValueButton = Ti.UI.createButton(
{
   title: "Change Value",
   top: "5%",
});
changeValueButton.addEventListener("click", function(e) {
   textField.value = "Hello World";
});
window.add(changeValueButton);
var changeAttributedStringButton = Ti.UI.createButton(
{
   title: "Change Attributed String",
   top: "5%",
});
changeAttributedStringButton.addEventListener("click", function(e) {
   textField.attributedString = Ti.UI.createAttributedString(
   {
      text: "Appcelerator Titanium rocks!",
      attributes:
      [
         {
            type: Titanium.UI.ATTRIBUTE_UNDERLINES_STYLE,
            value: Ti.UI.ATTRIBUTE_UNDERLINE_STYLE_SINGLE,
            range: [0, 2]
         },
         {
            type: Titanium.UI.ATTRIBUTE_BACKGROUND_COLOR,
            value: "red",
            range: [0, 2]
         },
         {
            type: Titanium.UI.ATTRIBUTE_BACKGROUND_COLOR,
            value: "blue",
            range: [2, 4]
         },
      ],
   });
});
window.add(changeAttributedStringButton);
window.open();

@build
Copy link
Contributor

build commented Jan 9, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn hansemannn added this to the 7.1.0 milestone Jan 9, 2018
* @param type TextView.BufferType - characteristics of the text such as static, styleable, or editable.
*/
@Override
public void setText(CharSequence text, BufferType type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Jenkins now checks our code changes to see if it conforms to our code formatting rules. This causes a failure because a method's starting curly brace '{' needs to be below the method.

@Override
public void setText(CharSequence text, BufferType type) {
super.setText(text, type);
setSelection(text.length());
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested iOS and noticed that the cursor is only moved to the end of the TextField if the text actually changes. It won't change the cursor position (or text selection) if setting the value property to the same text it already has. So, we need to do an equality check like this...

@Override
public void setText(CharSequence text, BufferType type)
{
	// Do not continue if the text is unchanged.
	CharSequence oldText = getText();
	if (text == null) {
		if ((oldText == null) || oldText.equals("")) {
			return;
		}
	} else if (text.equals(oldText)) {
		return;
	}

	// Update the field's text.
	super.setText(text, type);

	// Move the cursor to the end of the field. (Matches iOS' behavior.)
	setSelection(length());
}

Also note that your setSelection() call wasn't checking if the text argument was null. My setSelection() method call above fetches length from the EditText field instead, avoiding the null pointer exception case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the thoughtful reviews! I tied it out and had this error message:

Caused by: java.lang.ClassCastException: java.lang.String cannot be cast to android.text.Editable
at android.widget.EditText.getText(EditText.java:84)
at ti.modules.titanium.ui.widget.TiUIEditText.setText(TiUIEditText.java:112)

EditText.java line 84 is

    public Editable getText() {
        return (Editable) super.getText();
    }

TiUIEditText.java line 112 is

CharSequence oldText = getText();

I've tried sth like getText().toString() and SpannableStringBuilder etc. It seems come from EditText/TextView source code. At the time generating this error, it looks like doing initialisation, the super.getText() returns a String "" and also the text to be set is "". Any idea how could I avoid the error? @jquick-axway

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh bummer. This is a bug on Google's end. The returned Editable type does implement the CharSequence interface. So, what we're doing on our end is correct. The issue is that Google's getText() method blindly casts the returned value from super.getText() which defaults to an empty string in Google's TextView constructor. The Java String type does not implement the Editable interface, hence the class cast exception. :(

We can work-around the issue mentioned above, but I don't want to add extra complexity to the code. So, let's change the code back to what you had before (sorry), but use the EditText.length() method to set the cursor position instead to avoid the null exception case.

@Override
public void setText(CharSequence text, BufferType type)
{
	super.setText(text, type);
	setSelection(length());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted with thanks!

@build build added the android label Jan 11, 2018
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@longton95 longton95 self-requested a review January 16, 2018 19:49
Copy link
Contributor

@longton95 longton95 left a comment

Choose a reason for hiding this comment

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

FR passed waiting for Jenkins build to pass.

Tested using the test case the ticket and above, the cursor is now at the end of the textfield in all cases.

ENV

SDK Version : 7.0.1.GA, Local 7.1.0
macOS Sierra 10.13.2
Google Pixel 2 XL (8.1.0)
android emulator (7.1.1)
Appc CLI : 7.0.2-master.5
Appc NPM : 4.2.11
Node : v8.9.1

@build build added the android label Jan 16, 2018
@longton95 longton95 merged commit 3272543 into tidev:master Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants