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 crash caused by reading View's "backgroundDisabledColor" if background/border properties are set #10744

Closed
wants to merge 1 commit into from

Conversation

jquick-axway
Copy link
Contributor

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

Summary:
Fixed bug where if both a background color property and a border property was set, then reading the "backgroundDisabledColor" property would cause a crash.

Note:
@garymathews , @ypbnv , I added a new TiCast.java class used to provide a C#/Swift as keyword like functionality where if it's unable to cast to the given type, then it'll return null. It allows us to do the following. What do you think?

BitmapDrawable result = TiCast.as(getDrawable(), BitmapDrawable.class);
if (result != null) {
	// Got it!
}

Note that I'd prefer to use generics more heavily, but Java generics cannot provide type information at runtime, making the below impossible. The only viable solution that I know of is to pass in a Class<T> type like I'm doing in this PR's code.

public static <T> T as(Object value)
{
	if (value instanceof T) { // <- This won't work.
		return (T) value;
	}
	return null;
}

Test:

  1. Build and run the below code on Android.
  2. Verify that the app does not crash on startup.
  3. Uncomment the 2 lines of code below.
  4. Rebuild and rerun the app on Android.
  5. Verify that the displayed TextField now has a gray background.
  6. In the log, verify that you see the following message. ("#FF888888" is the color "gray".)
    [INFO] : ### textField.backgroundDisabledColor: #FF888888
var window = Ti.UI.createWindow();
var textField = Ti.UI.createTextField({
	value: "Hello World",
	width: "80%",
	color: "white",
	backgroundColor: "black",
	borderColor: "white",
//	backgroundDisabledColor: "gray",
//	enabled: false,
});
window.add(textField);
window.open();
Ti.API.info("### textField.backgroundDisabledColor: " + textField.backgroundDisabledColor);
Ti.API.info("### JSON.stringify(textField): " + JSON.stringify(textField));

…ndDisableColor" if background/border properties are set

- Also added new "TiCast" Java class which provides C#/Switft "as" keyword like support.
@jquick-axway jquick-axway added this to the 8.1.0 milestone Mar 2, 2019
@build build requested a review from a team March 2, 2019 04:00
@build
Copy link
Contributor

build commented Mar 2, 2019

Fails
🚫

😥 npm test failed. See below for details.

Warnings
⚠️

🔍 Can't find junit reports at ./junit.*.xml, skipping generating JUnit Report.

Messages
📖
> titanium-mobile@8.1.0 test /Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744
> npm run ios-sanity-check && grunt


> titanium-mobile@8.1.0 ios-sanity-check /Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744
> ./build/scons check-ios-toplevel

Running "appcJs:src:lintOnly" (appcJs) task

Running "eslint:src" (eslint) task

/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744/build/scons-xcode-test.js
27:3  warning  Each then() should return a value or throw  promise/always-return

/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744/build/utils.js
154:3   warning  Avoid using promises inside of callbacks    promise/no-promise-in-callback
154:3   warning  Avoid using promises inside of callbacks    promise/no-promise-in-callback
154:63  warning  Each then() should return a value or throw  promise/always-return
155:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
157:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
175:59  warning  Each then() should return a value or throw  promise/always-return
176:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
178:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
189:3   warning  Avoid using promises inside of callbacks    promise/no-promise-in-callback
189:3   warning  Avoid using promises inside of callbacks    promise/no-promise-in-callback
189:51  warning  Each then() should return a value or throw  promise/always-return
190:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
192:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
203:71  warning  Each then() should return a value or throw  promise/always-return
205:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
206:17  warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
218:71  warning  Each then() should return a value or throw  promise/always-return
220:4   warning  Avoid calling back inside of a promise      promise/no-callback-in-promise

/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744/common/Resources/ti.internal/bootstrap.loader.js
122:17  warning  Found non-literal argument in require  security/detect-non-literal-require

/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744/iphone/cli/commands/_build.js
6410:79  warning  'out' is defined but never used  no-unused-vars

/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-10744/iphone/cli/hooks/frameworks.js
 61:5   warning  Expected catch() or return                  promise/catch-or-return
 61:34  warning  Avoid calling back inside of a promise      promise/no-callback-in-promise
428:5   warning  Each then() should return a value or throw  promise/always-return

✖ 24 problems (0 errors, 24 warnings)


Running "checkFormat:ios" (checkFormat) task

Running "checkFormat:android" (checkFormat) task
Fatal error: Error: Formatting incorrect on "android/titanium/src/java/org/appcelerator/titanium/proxy/TiViewProxy.java", proposed changes: <?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='28132' length='10'>&#10;								   </replacement>
<replacement offset='28313' length='9'>&#10;							   </replacement>
</replacements>
�
npm ERR! Test failed.  See above for more details.

Generated by 🚫 dangerJS against 35c26ef

Copy link
Contributor

@ypbnv ypbnv left a comment

Choose a reason for hiding this comment

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

LGTM!

@ypbnv
Copy link
Contributor

ypbnv commented Mar 25, 2019

For the bug - besides the complaint for formatting from Danger it looks good to me.
And seeing that I introduced it I would like to know what do you think about getting properties this way. On one hand I am not sure if it is worth it to make reflections like this for every place we have drawables, but on the other this could affect our unit tests in a good way - making sure we do not regress straightforward properties. Recently I have stumbled upon similar situations on a few occasions and I am starting to think if it will be good to try to apply it wherever possible. Of course other concern I have about such a move is the one mentioned in here: #10393 ( Long story short - backwards compatibility with human readable color names ).

I am OK with the introduction of the as method. I see myself taking advantage of it.

@jquick-axway
Copy link
Contributor Author

@ypbnv, I think I should close this PR in favor of your PR #10393. Thanks for bringing that up.

I'll see about introducing TiCast later. I'm not sure if I'm 100% settled on it yet. I may just get rid of its instance methods and only keep the static as() method instead.

@garymathews
Copy link
Contributor

Closing in favor of #10393

NOTE: You should make another PR containing TiCast

@garymathews garymathews closed this May 1, 2019
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