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-17210] Android: Fixed TextField/TextArea to not trigger "change" event when setting properties #9700

Merged
merged 7 commits into from Feb 16, 2018

Conversation

maggieaxway
Copy link
Contributor

@maggieaxway maggieaxway commented Jan 8, 2018

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

Please test in both Android and iOS.

Test case:

var window = Ti.UI.createWindow({ layout: "vertical" });
var textField = Ti.UI.createTextField(
//var textField = Ti.UI.createTextArea(
{
	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 8, 2018

Messages
📖

🎉 Another contribution from our awesome community member, maggieaxway! Thanks again for helping us make Titanium SDK better. 👍

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

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.

@maggieaxway, can you also fix this for the attributedString property too please? I've confirmed that changing the attributedString property for TextField and TextArea fires a change event as well.

Side Note:
I've confirmed that iOS TextFields and TextAreas do not fire a "change" event for value and attributedString property changes.

@@ -825,10 +828,13 @@ public void setAttributedStringText(AttributedStringProxy attrString)
Bundle bundleText =
AttributedStringProxy.toSpannableInBundle(attrString, TiApplication.getAppCurrentActivity());
if (bundleText.containsKey(TiC.PROPERTY_ATTRIBUTED_STRING)) {
//TIMOB-17210 Android: A textfield change listener is wrongly triggered also if the value is programmatically set before creation
disableChangeEvent = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this code block's disableChangeEvent handling to the following please?

boolean wasDisabled = disableChangeEvent;
disableChangeEvent = true;
/* ... */
disableChangeEvent = wasDisabled;

The reason is because the processProperties() method already sets the disableChangeEvent to true and this method call will end up changing it back to false too soon. The above code will change it back to its original state. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I was still thinking why it's using wasDisabled. Thanks for the explanation :)

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

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.

Tested using the test case the ticket and above, the event is only triggered when the text is changed by the user and not programmatically set.

It is now the same as iOS

ENV

SDK Version : 7.0.2.GA, Local 7.1.0
macOS Sierra 10.13.3
Google Pixel 2 XL (8.1.0)
android emulator (7.1.1)
iOS simulator (11.2)
Appc CLI : 7.0.3-master.3
Appc NPM : 4.2.12
Node : v8.9.1

@longton95
Copy link
Contributor

@maggieaxway What release is this ticket scheduled for?

@jquick-axway
Copy link
Contributor

Let's get it into 7.1.0. (I just updated the ticket.)

@jquick-axway jquick-axway added this to the 7.1.0 milestone Feb 13, 2018
@longton95
Copy link
Contributor

@jquick-axway Would you be able to create a backport for 7_1_X

@jquick-axway
Copy link
Contributor

@longton95, here is the back-port: #9834

@jquick-axway jquick-axway changed the title [TIMOB-17210] Fix Android: A textfield change listener is wrongly triggered also if the value is programmatically set before creation [TIMOB-17210] Android: Fixed TextField/TextArea to not trigger "change" event when setting properties Feb 15, 2018
@jquick-axway jquick-axway modified the milestones: 7.1.0, 7.2.0 Feb 15, 2018
@longton95 longton95 merged commit fa3540b into tidev:master Feb 16, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 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

5 participants