[TIMOB-26390] Android: Connect getters with background drawables for Unit test purposes. #10393

ypbnv · 2018-10-23T12:21:48Z

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

Description:
Connect the getters for different color states for Ti.UI.View with the Android object representing it.
Extract the method for getting colors for state in Ti.UIHelper.
Expose the states we assign for custom background drawables.
Guard for backwards compatibility.
Replace the String properties in Ti.UI.View with the proper TiC constants.

NOTE: This introduces behavior that may be classified as "breaking". Prior to this PR color state properties that have not been defined would return undefined and now that is replaced with null. Also colors set with predefined words ( like "green", "cyan", "yellow" ) will now return the hexadecimal value with alpha channel. For instance setting a backgroundColor to blue will afterwards return the value #FF0000FF. Please, let me know what do you think about these changes and if they need reworking, mention in the docs or anything that this PR is missing.
The failed unit tests show pretty well what I had in mind with "breaking" change. Having a list with predefined color words for specific color values does solve the problem ( I personally don't like this approach because there may be a difference in naming specific color values across platforms** ). I see we have a defined color map, but it may need a bit of a rework to help in this case.
** A good example is Android - they are supporting both words "green" and "lime" according to their docs:
https://developer.android.com/reference/android/graphics/Color.html#parseColor(java.lang.String)
Which is great, but the result of the call of Color.parseColor() with both parameters is the same. Judging by the code it is more likely a bug, because there is no point in having two keys with the same value in their color map.

Test case:
The added unit test should suffice, but here is a code sample
app.js

var win = Ti.UI.createWindow(),
	view = Ti.UI.createView({
		width: 100,
		height: 100,
		borderColor: 'green',
		borderWidth: 5,
		backgroundColor: 'red',
		backgroundSelectedColor: 'green',
		backgroundFocusedColor: 'blue',
		backgroundDisabledColor: 'gray'
	}),
	button = Ti.UI.createButton();

win.add(view);
win.add(button);

button.addEventListener('click', function () {
	Ti.API.info('backgroundColor is ' + view.getBackgroundColor());
	Ti.API.info('backgroundSelectedColor is ' + view.getBackgroundSelectedColor());
	Ti.API.info('backgroundFocusedColor is ' + view.backgroundFocusedColor);
	Ti.API.info('backgroundDisabledColor is ' + view.backgroundDisabledColor);
});
win.open();

keerthi1032 · 2018-10-23T18:05:59Z

FR Passed. Waiting for CR and Merge. Hexadecimal values displayed for colors.
Test Environment:
Operating System
Name = Mac OS X
Version = 10.13.6
Architecture = 64bit
Node.js
Node.js Version = 8.12.0
npm Version = 6.4.1
Titanium CLI
CLI Version = 5.1.1
Titanium SDK
SDK Version = Local 7.5.0.v20181023053605 build
Device =Samsung s5 Android 6
Emulator = pixel android 7, Sony Xperia S android 4.1.1

lokeshchdhry · 2018-10-24T20:35:59Z

@ypbnv , Can you please fix the unit test failures. Thanks.

ypbnv · 2018-10-25T08:40:09Z

@lokeshchdhry Removing this out of 7.5.0. Will schedule it for 8.0.0, since we need to clarify what approach will be the most suitable for the PR's purpose.

lokeshchdhry · 2018-11-30T23:12:56Z

@ypbnv , Did we finalize any approach for this PR ? Is it good to merge ?

ypbnv · 2018-12-03T13:58:10Z

@lokeshchdhry Not really.
@garymathews, @jquick-axway When you get the chance can you take a look at the note in the description?

jquick-axway · 2019-03-25T19:23:15Z

android/titanium/src/java/org/appcelerator/titanium/proxy/TiViewProxy.java

-	"softKeyboardOnFocus",
-	"transform",
-	"elevation",
-	"touchTestId",


This change is removing "touchTestId", but that's probably fine since it's not referenced anywhere else in the code. Our iOS code doesn't have any references to it either.

jquick-axway · 2019-03-25T19:29:42Z

android/titanium/src/java/org/appcelerator/titanium/util/TiUIHelper.java

+				return transcriptColorIntToString(((PaintDrawable) simpleDrawable).getPaint().getColor());
+			}
+			// Get the backgroundDrawable background as a StateListDrawable.
+			StateListDrawable stateListDrawable = ((StateListDrawable) simpleDrawable);


We need to do an instanceof check on the StateListDrawable to avoid the issue mentioned here...
#10744

jquick-axway · 2019-03-25T19:30:19Z

android/titanium/src/java/org/appcelerator/titanium/util/TiUIHelper.java

+				if (state != TiUIHelper.BACKGROUND_DEFAULT_STATE_1) {
+					return null;
+				}
+				return transcriptColorIntToString(((PaintDrawable) simpleDrawable).getPaint().getColor());


We need to do an instanceof check on PaintDrawable.

jquick-axway · 2019-03-25T19:31:40Z

android/titanium/src/java/org/appcelerator/titanium/util/TiUIHelper.java

+			Log.w(TAG, "Unable to access a method for reflection.");
+		} catch (InvocationTargetException e) {
+			Log.w(TAG, "Unable to invoke a method for reflection.");
+		}


Let's catch all exceptions here as shown in my other PR.
I'll close my PR once this PR resolves the same issue.

jquick-axway · 2019-03-25T19:46:21Z

android/titanium/src/java/org/appcelerator/titanium/util/TiUIHelper.java

+	public static String transcriptColorIntToString(int colorInt)
+	{
+		return String.format("#%08X", 0xFFFFFFFF & colorInt);
+	}


I think this function should be moved to our TiColorHelper class.

And would you mind renaming the function from transcriptColorIntToString(int) to hexStringFrom(int) please?
I say this because Titanium also supports an "rgba(0,0,0,0)" color string as well, which if we were to support it in the future, I would name it rgbaStringFrom(int).

…for TiView. Replace Strings in property accessors list for Ti.View with proper constants.

garymathews · 2019-05-01T21:12:20Z

CR: PASS

TEST CASE from #10744

Build and run the below code on Android.
Verify that the app does not crash on startup.
Uncomment the 2 lines of code below.
Rebuild and rerun the app on Android.
Verify that the displayed TextField now has a gray background.
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));

Addressed review comments.

build · 2019-05-01T21:19:26Z

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	✅ All tests are passing Nice one! All 3727 tests are passing. (There are 466 tests skipped)

Generated by 🚫 dangerJS against ca28331

garymathews · 2019-05-01T22:33:11Z

Reverted to returning the property string by default for parity and to satisfy our test cases.

keerthi1032 · 2019-05-02T18:20:08Z

FR Passed.Works as expected.
Test Environment:
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
Titanium SDK
SDK Version = local sdk 8.1.0.v20190501153539
CLI =7.0.11-1
Studio =5.1.2.201903111843
Device =Samsung s5 android 6,pixel android 9
Emulator =pixel 3xl android 9

…Unit test purposes. (tidev#10393) * Connect different color states with the colors in the Android object for TiView. Replace Strings in property accessors list for Ti.View with proper constants. * fix(android): rename methods and add validation * fix(android): formatting * fix(android): return property by default

ypbnv added android bug labels Oct 23, 2018

ypbnv added this to the 7.5.0 milestone Oct 23, 2018

ypbnv requested a review from garymathews October 23, 2018 12:21

ypbnv modified the milestones: 7.5.0, 8.0.0 Oct 25, 2018

ypbnv mentioned this pull request Nov 16, 2018

[TIMOB-26480] Android: Titanium.UI.ProgressBar 'color' setting portion of progressBar - should be text only #10478

Merged

mukherjee2 modified the milestones: 8.0.0, 8.1.0 Jan 16, 2019

ypbnv mentioned this pull request Feb 11, 2019

feat(android): Add "barColor" property of Ti.UI.Window to Android. #10693

Merged

ypbnv mentioned this pull request Mar 25, 2019

fix(android): Fixed crash caused by reading View's "backgroundDisabledColor" if background/border properties are set #10744

Closed

jquick-axway previously requested changes Mar 25, 2019

View reviewed changes

ypbnv and others added 2 commits May 1, 2019 13:50

Connect different color states with the colors in the Android object …

86e626f

…for TiView. Replace Strings in property accessors list for Ti.View with proper constants.

fix(android): rename methods and add validation

f0317ce

garymathews force-pushed the TIMOB-26390 branch from c963e87 to f0317ce Compare May 1, 2019 21:04

fix(android): formatting

16bb56b

garymathews approved these changes May 1, 2019

View reviewed changes

garymathews added the in-qe-testing 🕵 label May 1, 2019

jquick-axway approved these changes May 1, 2019

View reviewed changes

tidev deleted a comment from build May 1, 2019

fix(android): return property by default

ca28331

keerthi1032 merged commit 3d1569d into tidev:master May 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-26390] Android: Connect getters with background drawables for Unit test purposes. #10393

[TIMOB-26390] Android: Connect getters with background drawables for Unit test purposes. #10393

ypbnv commented Oct 23, 2018 •

edited

keerthi1032 commented Oct 23, 2018

lokeshchdhry commented Oct 24, 2018

ypbnv commented Oct 25, 2018 •

edited

lokeshchdhry commented Nov 30, 2018

ypbnv commented Dec 3, 2018

jquick-axway Mar 25, 2019

jquick-axway Mar 25, 2019

jquick-axway Mar 25, 2019

jquick-axway Mar 25, 2019

jquick-axway Mar 25, 2019

garymathews commented May 1, 2019

build commented May 1, 2019 •

edited

garymathews commented May 1, 2019

keerthi1032 commented May 2, 2019

[TIMOB-26390] Android: Connect getters with background drawables for Unit test purposes. #10393

[TIMOB-26390] Android: Connect getters with background drawables for Unit test purposes. #10393

Conversation

ypbnv commented Oct 23, 2018 • edited

keerthi1032 commented Oct 23, 2018

lokeshchdhry commented Oct 24, 2018

ypbnv commented Oct 25, 2018 • edited

lokeshchdhry commented Nov 30, 2018

ypbnv commented Dec 3, 2018

jquick-axway Mar 25, 2019

Choose a reason for hiding this comment

jquick-axway Mar 25, 2019

Choose a reason for hiding this comment

jquick-axway Mar 25, 2019

Choose a reason for hiding this comment

jquick-axway Mar 25, 2019

Choose a reason for hiding this comment

jquick-axway Mar 25, 2019

Choose a reason for hiding this comment

garymathews commented May 1, 2019

TEST CASE from #10744

build commented May 1, 2019 • edited

garymathews commented May 1, 2019

keerthi1032 commented May 2, 2019

ypbnv commented Oct 23, 2018 •

edited

ypbnv commented Oct 25, 2018 •

edited

build commented May 1, 2019 •

edited