[TIMOB-25687] Android: Picker change listener doesn't work the first time #9763

ypbnv · 2018-01-23T15:35:57Z

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

Description:
Set a default preselectedRows pair for NativePicker in order to trigger first time onItemSelect guard.
This would match the default selected item when a index is not set.
That is necessary to not lose the first onItemSelected call in case the adapter is filled for the first time from thread different from the UI one.

Test case:
app.js

var win = Ti.UI.createWindow();
var type2 = [];
type2[0]= Ti.UI.createPickerRow({title: 'Row 1'});
type2[1]= Ti.UI.createPickerRow({title: 'Row 2'});

pickerType = Ti.UI.createPicker();
win.add(pickerType);
setTimeout(loadTypes, 1000);
pickerType.selectionIndicator = true;
pickerType.addEventListener('change', pickerChange);

function loadTypes() {
    pickerType.add(type2);
}

function pickerChange(e){
    alert('Change listener triggered');
}

win.open();

An Alloy test case can be found in the JIRA ticket.

…he onItemSelected guard.

jquick-axway

CR: Pass

jquick-axway · 2018-01-23T22:23:05Z

@ypbnv,
Quick question. Will this also be an issue for pickers with multiple columns?

ypbnv · 2018-01-24T13:38:24Z

@jquick-axway Just tested it. It works fine with multiple columns. But I found something else I have missed. I will update once I have it figured out.

Clean some unnecessary code.

ypbnv · 2018-01-24T16:50:09Z

@jquick-axway I updated the code. If you could go through the recent changes that would be great.

jquick-axway

Just a few minor change requests.
The rest looks good to me.

jquick-axway · 2018-01-25T02:44:20Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUINativePicker.java

-	public void onItemSelected(AdapterView<?> parent, View view, int position, long itemId)
-	{
-		if (!firstSelectedFired) {
+	final OnItemSelectedListener onItemSelectedListener = new OnItemSelectedListener() {


Can you make this listener member variable private please?

jquick-axway · 2018-01-25T02:45:12Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUINativePicker.java

 			// swallow the first selected event that gets fired after the adapter gets set, so as to avoid
 			// firing our change event in that case.
 			firstSelectedFired = true;
 			return;
-		}
-		fireSelectionChange(0, position);
+		}*/


Since you've removed the firstSelectedFired member variable, we might as well as delete this commented out code block too.

jquick-axway · 2018-01-25T02:47:00Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUINativePicker.java

-			if (p instanceof View) {
-				((View) p).invalidate();
+			// Invalidate the parent view after the item is selected (TIMOB-13540).
+			if (Build.VERSION.SDK_INT >= TiC.API_LEVEL_HONEYCOMB) {


Can you remove the HONEYCOMB version check please? Since API Level 16 is the min version we support now, we should always run the below parent invalidate code.

Remove comments left from older code. Remove outdated platform version check.

ypbnv · 2018-01-25T11:13:50Z

@jquick-axway Updated the PR. Also the docs change was made for the previous take on the problem. But it is still relevant because:

NativePicker is always used with one column only - the index will always be 0.
By default in the Android sources if we do not have preselected index it uses 0 for the dropdown picker.

Should we keep them that way or maybe reword/revert the docs change?

lokeshchdhry · 2018-02-13T22:30:23Z

@ypbnv , Can you please fix the lint issues https://jenkins.appcelerator.org/job/titanium-sdk/job/titanium_mobile/job/PR-9763/11/console

lokeshchdhry · 2018-02-14T00:03:53Z

FR Passed.

The change event is successfully fired for the first time.
Checked with plain picker & multi column picker.

Studio Ver: 5.0.0.201712081732
SDK Ver: 7.1.0 local build
OS Ver: 10.13.2
Xcode Ver: Xcode 9.2
Appc NPM: 4.2.12
Appc CLI: 7.0.2
Daemon Ver: 1.0.1
Ti CLI Ver: 5.0.14
Alloy Ver: 1.11.0
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 6P --- Android 8.0.0
⇨ google Nexus 5 --- Android 6.0.1

hansemannn · 2018-02-17T09:28:58Z

@ypbnv Backport please

build · 2018-02-19T11:17:27Z

	Messages
📖	💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

ypbnv added 2 commits January 23, 2018 17:19

Set a default preselcted index for NativePicker in order to trigger t…

e49d358

…he onItemSelected guard.

Add docs change and unit test.

ccf7d0f

ypbnv added android bug labels Jan 23, 2018

ypbnv added this to the 7.1.0 milestone Jan 23, 2018

ypbnv requested a review from jquick-axway January 23, 2018 15:35

build added android docs labels Jan 23, 2018

jquick-axway approved these changes Jan 23, 2018

View reviewed changes

jquick-axway added the in-qe-testing 🕵 label Jan 23, 2018

ypbnv removed the in-qe-testing 🕵 label Jan 24, 2018

Rework handling of the first "fake" itemSelected event handling.

8f25bb7

Clean some unnecessary code.

build added android docs labels Jan 24, 2018

Merge branch 'master' into TIMOB-25687

55429c6

build added android docs labels Jan 24, 2018

jquick-axway suggested changes Jan 25, 2018

View reviewed changes

ypbnv added 2 commits January 25, 2018 13:06

Merge branch 'master' into TIMOB-25687

65603ae

Add access modifier for the new listener.

c80d4bb

Remove comments left from older code. Remove outdated platform version check.

build added android docs labels Jan 25, 2018

Merge branch 'master' into TIMOB-25687

e970070

build added android docs labels Feb 13, 2018

Merge branch 'master' into TIMOB-25687

48d45ee

build added android docs labels Feb 14, 2018

Lint unit test.

90091d1

build added android docs labels Feb 15, 2018

Merge branch 'master' into TIMOB-25687

136e984

build added android docs labels Feb 15, 2018

Merge branch 'master' into TIMOB-25687

745befc

build added android docs labels Feb 16, 2018

Merge branch 'master' into TIMOB-25687

52b2fbd

ypbnv modified the milestones: 7.1.0, 7.2.0 Feb 19, 2018

ypbnv mentioned this pull request Feb 19, 2018

[7_1_X][TIMOB-25687] Android: Picker change listener doesn't work the first time #9857

Merged

build added android docs labels Feb 19, 2018

hansemannn merged commit 65f161d into tidev:master Feb 19, 2018

sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-25687] Android: Picker change listener doesn't work the first time #9763

[TIMOB-25687] Android: Picker change listener doesn't work the first time #9763

ypbnv commented Jan 23, 2018 •

edited

jquick-axway left a comment

jquick-axway commented Jan 23, 2018

ypbnv commented Jan 24, 2018 •

edited

ypbnv commented Jan 24, 2018 •

edited

jquick-axway left a comment

jquick-axway Jan 25, 2018

jquick-axway Jan 25, 2018

jquick-axway Jan 25, 2018

ypbnv commented Jan 25, 2018 •

edited

lokeshchdhry commented Feb 13, 2018

lokeshchdhry commented Feb 14, 2018

hansemannn commented Feb 17, 2018

build commented Feb 19, 2018

[TIMOB-25687] Android: Picker change listener doesn't work the first time #9763

[TIMOB-25687] Android: Picker change listener doesn't work the first time #9763

Conversation

ypbnv commented Jan 23, 2018 • edited

jquick-axway left a comment

Choose a reason for hiding this comment

jquick-axway commented Jan 23, 2018

ypbnv commented Jan 24, 2018 • edited

ypbnv commented Jan 24, 2018 • edited

jquick-axway left a comment

Choose a reason for hiding this comment

jquick-axway Jan 25, 2018

Choose a reason for hiding this comment

jquick-axway Jan 25, 2018

Choose a reason for hiding this comment

jquick-axway Jan 25, 2018

Choose a reason for hiding this comment

ypbnv commented Jan 25, 2018 • edited

lokeshchdhry commented Feb 13, 2018

lokeshchdhry commented Feb 14, 2018

hansemannn commented Feb 17, 2018

build commented Feb 19, 2018

ypbnv commented Jan 23, 2018 •

edited

ypbnv commented Jan 24, 2018 •

edited

ypbnv commented Jan 24, 2018 •

edited

ypbnv commented Jan 25, 2018 •

edited