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-26104] Android: Picker inside Tableview resets value #10211

Merged
merged 20 commits into from Oct 2, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Jul 25, 2018

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

Description:
Persist the selection in a Picker in case it has to be refreshed\added again after being removed.

Note: This may not be the best experience if during the absence of the Picker from the screen it's adapter has been changed. Do you think it will be fine to reset the persisted index in that case?

Test case:
Removing and adding the picker again
app.js

var window = Ti.UI.createWindow();
var picker = Ti.UI.createPicker({ width: '50%' });
var rows = [];

for (var index = 0; index < 20; index++) {
	rows.push(Ti.UI.createPickerRow({ title: 'Item ' + (index + 1).toString() }));
}

picker.add(rows);

window.add(picker);

function createButtonText() {
	return window.children.includes(picker) ? 'Remove Picker' : 'Add Picker';
}

var button = Ti.UI.createButton({
	title: createButtonText(),
	bottom: '20dp',
});

button.addEventListener('click', function(e) {
	if (window.children.includes(picker)) {
		window.remove(picker);
	} else {
		window.add(picker);
	}

	button.title = createButtonText();
});

window.add(button);
window.open();

Using a Picker in a TableView
app.js

var currentwindow = Ti.UI.createWindow();

var tabledata = [];

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

var data = [];
data[0]=Ti.UI.createPickerRow({title:'Bananas'});
data[1]=Ti.UI.createPickerRow({title:'Strawberries'});
data[2]=Ti.UI.createPickerRow({title:'Mangoes'});

picker.add(data);

var myrow=Ti.UI.createTableViewRow({ 'height' : '50sp' });

myrow.add(picker);
tabledata.push(myrow);

var infotableview = Titanium.UI.createTableView({ data:tabledata });

currentwindow.add(infotableview);
currentwindow.open();

Unit test added.

@ypbnv ypbnv added the android label Jul 25, 2018
@ypbnv ypbnv added this to the 7.4.0 milestone Jul 25, 2018
@ypbnv ypbnv requested a review from jquick-axway July 25, 2018 12:40
@ypbnv ypbnv added the bug label Jul 25, 2018
@build
Copy link
Contributor

build commented Jul 25, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@jquick-axway
Copy link
Contributor

jquick-axway commented Jul 25, 2018

Question:
Do we need to store the last selection for each column as well? I'm okay with us writing that up as a separate ticket in order to resolve the community's issue with this today. Single selection pickers are the most common case.

This may not be the best experience if during the absence of the Picker from the screen it's adapter has been changed. Do you think it will be fine to reset the persisted index in that case?

To support this, our proxy's getter would have to return the proxy's lastSelectedIndex member variable instead of returning the index from native picker.

@garymathews
Copy link
Contributor

@hansemannn @ypbnv iOS failed ios.Ti.UI.Picker | Selected index persistance

Is there an issue here?

@hansemannn
Copy link
Collaborator

@garymathews postlayout on iOS is only triggered if the layout actually changed. If the frame / origin of the view remains (like in this case), it is not fired. Can we change the test to not use the picker's postlayout event but the window's open / focus event? I rather would not change the iOS side to trigger a postevent on row-changes, since it seems Android-specific.

@ypbnv
Copy link
Contributor Author

ypbnv commented Jul 31, 2018

@hansemannn I will see if the test can use something different. I tried at first with events from the window, but got some mismatched timings.
And BTW is the disparity of when the postlayout is fired on Android and iOS something we should address? I have used it in a number of unit tests and it sounds like I may have to revisit that.

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 2, 2018

@hansemannn I am not particularly proud with the workaround for the unit test, but if I got it right now it should be fine on iOS too.

@jquick-axway Let's separate the multiple column persistence in another ticket then. I think it can be put together with the persistence across different data sets for both single and multiple column pickers.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@jquick-axway
Copy link
Contributor

jquick-axway commented Aug 3, 2018

@hansemannn, @ypbnv,

I just tested add/removing the following picker types:

  • For multicolumn spinner, both Android and iOS forget last selection.
  • For date picker, iOS resets to current time. (Android remembers last selection.)
  • For time picker, iOS resets to current time. (Android remembers last selection.)

I think the above are minor and low priority. I've written up the following tickets:

Edit:
Actually, I just discovered that iOS resets selection for single column picker. iOS fails this ticket's test case.

@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
@keerthi1032
Copy link
Contributor

keerthi1032 commented Sep 18, 2018

FR Passed. Works fine. Some test are failing in jenkins. Not able to merge PR .Can somebody please look at it.
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

SDK =7.5.0.v20180901082336 local build
Studio = 5.1.1.201809051655
Device = samsung galaxy s5 -Android 6
Pixel emulator -Android 7

@sgtcoolguy sgtcoolguy merged commit fbb3803 into tidev:master Oct 2, 2018
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

10 participants