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): Fixed bug where setting SearchBar "hintText" after window open crashes as of 7.0.0 #10848

Merged
merged 3 commits into from Apr 23, 2019

Conversation

jquick-axway
Copy link
Contributor

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

Test:

  1. Build and run the below code on Android.
  2. Verify that the app starts up without crashing.
  3. Verify that the top SearchBar reads "Hint Text".
var window = Ti.UI.createWindow();
var searchBar = Ti.UI.createSearchBar({
	top: 0,
	width: Ti.UI.FILL,
	height: "50dp",
});
window.add(searchBar);
window.add(Ti.UI.createLabel({ text: "SearchBar Test" }));
window.addEventListener("open", function(e) {
	searchBar.hintText = "Hint Text";
});
window.open();

@jquick-axway jquick-axway added this to the 8.1.0 milestone Apr 13, 2019
@build build requested a review from a team April 13, 2019 01:49
@build
Copy link
Contributor

build commented Apr 13, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3649 tests are passing.

Generated by 🚫 dangerJS against adaa7f7

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -347,7 +347,10 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP
} else if (key.equals(TiC.PROPERTY_COLOR)) {
tv.setTextColor(TiConvert.toColor((String) newValue));
} else if (key.equals(TiC.PROPERTY_HINT_TEXT)) {
int type = proxy.getProperties().getInt(TiC.PROPERTY_HINT_TYPE);
int type = UIModule.HINT_TYPE_STATIC;
if (proxy.getProperties() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

assign to a temp var to avoid grabbing it via getter twice?

Also, we should consider using a null object pattern here to avoid the null check altogether. If a KrollProxy has a null properties field, can't we return an empty KrollDict and get the same behavior typically? (i.e. returns null for everything, returns default values if passed in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to change this to use proxy.getProperty(TiC.PROPERTY_HINT_TYPE) instead which will return null if the properties dictionary is null, but I can see this PR is already merged. That said, I think the current code is fine anyways.

@@ -921,7 +924,11 @@ public void setAttributedStringHint(AttributedStringProxy attrString)
{
Spannable spannableText = AttributedStringProxy.toSpannable(attrString, TiApplication.getAppCurrentActivity());
if (spannableText != null) {
int type = getProxy().getProperties().getInt(TiC.PROPERTY_HINT_TYPE);
int type = UIModule.HINT_TYPE_STATIC;
KrollProxy proxy = getProxy();
Copy link
Contributor

Choose a reason for hiding this comment

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

another potential place we could return a null object variant? i.e. if a TiUIView's proxy is null, return a special NullProxy instance that has empty properties?

} catch (err) {
finish(err);
}
}, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to add any specific delay in milliseconds here, I wouldn't think. You could put 1 and it should basically allow the UI thread to dispatch before we fire. (also note that technically the delay arg is optional, and under the hood it enforces a minimum delay anyways - so maybe just omit the delay)

@keerthi1032
Copy link
Contributor

FR passed. Searchbar did not crash and works as expected.
Test Enviornment:
Operating System
Name = Mac OS X
Version = 10.13.6
Architecture = 64bit
Node.js
Node.js Version = 8.9.1
npm Version = 5.5.1
Titanium CLI
CLI Version = 5.1.1
APPC CLI =7.0.11-70X.1
Titanium SDK
SDK Version = local 8.0.1.v20190423101725 and local master 8.1.0.v20190412182932
Device=Oneplus 5t android 9,samsung s5 android 6
Emulator =pixel 2xl android 9,nexus android 6

@build
Copy link
Contributor

build commented Apr 23, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3795 tests are passing.
(There are 466 tests skipped)

Generated by 🚫 dangerJS against 55a7a38

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

4 participants