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-18053] Android: fixed default text color value changing #8661

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

frankieandone
Copy link
Contributor

CHANGED

  • default color value kept getting changed so let default color be set only once

TEST CASE
Comment and uncomment colors in each picker row.

var win = Ti.UI.createWindow({
    exitOnClose: true,
    layout: 'vertical',
    backgroundColor: 'black'
});

var picker = Ti.UI.createPicker({
    top: 50
});

var data = [];
data[0] = Ti.UI.createPickerRow({
    title: 'Bananas',
    color: 'yellow'
});
data[1] = Ti.UI.createPickerRow({
    title: 'Strawberries',
    // color: 'red'
});
data[2] = Ti.UI.createPickerRow({
    title: 'Mangos',
    color: 'orange'
});
data[3] = Ti.UI.createPickerRow({
    title: 'Grapes',
    // color: 'purple'
});

picker.add(data);
picker.selectionIndicator = true;

win.add(picker);
win.open();

EXPECTED
A picker row's text color should only be set if specified otherwise should be default color.

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

@@ -82,8 +83,9 @@ private void styleTextView(int position, TextView tv) {
}
if (rowProxy.hasProperty(TiC.PROPERTY_COLOR)) {
final int color = TiConvert.toColor(rowProxy.getProperties(), TiC.PROPERTY_COLOR);
if (defaultTextColor != color) {
if (defaultTextColor != color && !setDefaultColor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private static int defaultTextColor = 0;
...
if (!defaultTextColor) {

That should be sufficient, like my original example 👍

@@ -41,7 +41,8 @@
{
private static final String TAG = "TiUINativePicker";
private boolean firstSelectedFired = false;
private static int defaultTextColor = 0;
private static int defaultTextColor = -1;
private static boolean setDefaultColor = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

the boolean isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would throw a compile error: you can't use boolean operators on integer types in Java.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could do this instead

if (defaultTextColor == 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean setdefaultTextColor to 0 and using your snippet then it behaves weird initially as text is not visible. Also, this if block would get called x2 for every item with text color set to 0. Seems inefficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see a problem. We should be setting the defaultColor regardless if PROPERTY_COLOR is defined. Otherwise, the initial text wont be set to the default text color unless an item with PROPERTY_COLOR is set first. And yes, good catch keep the Boolean 👍

See if this works?

private static int defaultTextColor;
private static boolean setDefaultTextColor = false;
...
private void styleTextView(int position, TextView tv) {
...
if (!setDefaultTextColor) {
    defaultTextColor = tv.getCurrentTextColor();
    setDefaultTextColor = true;
}
if (rowProxy.hasProperty(TiC.PROPERTY_COLOR)) {
    final int color = TiConvert.toColor(rowProxy.getProperties(), TiC.PROPERTY_COLOR);
    tv.setTextColor(color);
} else {
    tv.setTextColor(defaultTextColor);
}
...

@frankieandone frankieandone force-pushed the timob-18053 branch 3 times, most recently from 9e405e1 to 5930608 Compare December 7, 2016 08:13
@@ -41,7 +41,8 @@
{
private static final String TAG = "TiUINativePicker";
private boolean firstSelectedFired = false;
private static int defaultTextColor = 0;
private static int defaultTextColor;
private static boolean setDefaultTextColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! But lets initialise this setDefaultTextColor = false

@frankieandone frankieandone changed the title timob-18053 fixed default value changing [TIMOB-18053] Android: fixed default text color value changing Dec 7, 2016
@garymathews
Copy link
Contributor

CR: PASS
FT: PASS

@garymathews garymathews merged commit 340aefe into tidev:master Dec 7, 2016
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

2 participants