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(ios): allow TableViewRow proxy to return correct view #12225

Merged
merged 7 commits into from Nov 3, 2020

Conversation

garymathews
Copy link
Contributor

  • Amend TableViewRowProxy to return correct view for rect calculation
    • Tested via row#rect in ti.ui.tableview.test.js
  • Returns valid UIView to prevent TIMOB-28148
    • Tested via TIMOB-28148 : adding view on row causing crash in ti.ui.tableview.test.js
TEST CASE
const win = Ti.UI.createWindow({
    backgroundColor: '#AAAAFF',
    layout: 'vertical'
});
const top = Ti.UI.createView({
    backgroundColor: '#FFAAAA',
    layout: 'horizontal',
    height: Ti.UI.SIZE,
    width: Ti.UI.FILL
});
const btnPlus = Ti.UI.createButton({
    title: '+',
    top: 50,
    color: 'white',
    borderWidth: 2
});
const btnMinus = Ti.UI.createButton({
    title: '-',
    top: 50,
    color: 'white',
    borderWidth: 2
});
const label = Ti.UI.createLabel();

btnPlus.addEventListener('click', () => {
    console.log('plus click');
    view.height += 50;
});
btnMinus.addEventListener('click', () => {
    console.log('minus click');
    view.height -= 50;
});

const row = Ti.UI.createTableViewRow({
    height: Ti.UI.SIZE,
    width: Ti.UI.FILL
});
const view = Ti.UI.createView({
    height: 150,
    backgroundColor: 'blue'
});
row.add(view);

const tableView = Ti.UI.createTableView({ data: [ row ] });

tableView.addEventListener('postlayout', () => {
    console.log('postlayout', row.rect.height, view.rect.height);
    label.text = `${row.rect.height} | ${view.rect.height}`;
});

top.add([ btnPlus, btnMinus, label ]);
win.add([ top, tableView ]);
win.open();

JIRA Ticket

@@ -528,7 +528,7 @@ - (TiProxy *)parentForBubbling

- (UIView *)view
{
return nil;
return rowContainerView;
Copy link
Contributor

Choose a reason for hiding this comment

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

@garymathews TiUITableViewRowContainer is subclass of UIView. Ideally it should be subclass of TiUIView. It may crash at some point because all functions and properties available in TiUIView is not available with TiUITableViewRowContainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should view enforce TiUIView?

- (TiUIView *)view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR, TiUITableViewRowContainer is now a subclass of TiUIView

Copy link
Contributor

Choose a reason for hiding this comment

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

@garymathews I am not sure if it will work. I guess it may cause layout issues. 1. Can you test long tableview (at least some tableview rows goes out of screen) with different contents (to verify reusability of cells).
2. Need to test a bit more around TableViewRow properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing, everything works as it should with no layout or event issues. I don't think using TiUIView is necessary, but there's no harm in maintaining the change.

All of the tests are passing, including the problematic tests for TIMOB-28148 and TIMOB-27935.

TEST CASE
NOTE: I ran through more tests than this, but this provides a nice base to test more properties and complex views.
const win = Ti.UI.createWindow();
const tableData = [];

for (let i = 1; i <= 100; i++) {
	const row = Ti.UI.createTableViewRow();
	const label = Ti.UI.createLabel({
		color: 'black',
		text: `Row #${i}`,
		height: 50
	});

	row.add(label);
	tableData.push(row);
}

const tableView = Ti.UI.createTableView({ data: tableData });

tableView.addEventListener('click', e => {
	const row = tableData[e.index];
	const message = `Clicked row ${e.index + 1}\n\nrect.width: ${row.rect.width}\nrect.height: ${row.rect.height}`;

	// NOTE: `borderRadius` on TableViewRow has no effect on iOS.
	row.borderRadius = 20;
	row.backgroundColor = 'red';

	Ti.API.info(message);
	alert(message);
});
tableView.addEventListener('longpress', e => {
	const message = `Long-pressed row ${e.index + 1}`;

	Ti.API.info(message);
	alert(message);
});

const restartButton = Ti.UI.createButton({
	title: 'Restart',
	bottom: 5,
	right: 5
});
restartButton.addEventListener('click', _ => {
	Ti.App._restart();
});

win.add([ tableView, restartButton ]);
win.open();

We can notify QE to run through more TableView tests.

@build
Copy link
Contributor

build commented Oct 27, 2020

Fails
🚫 Tests have failed, see below for more information.
Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 971 skipped out of 12585 total tests.

Tests:

ClassnameNameTimeError
ios.macos.Titanium.UI.iOS.CollisionBehavior.exampleworks (10.15.5)15.014
Error: timeout of 15000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-12225/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27

Generated by 🚫 dangerJS against 209e6e0

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

Changes looks good.
@qe Can you verify other layout properties and memory release.

@ssekhri
Copy link

ssekhri commented Nov 3, 2020

FR Passed.
Correct values for the view returned and getRect() method on tableViewRow returns correct value.
No crash seen when adding/updating tableView.
Other tests on tableView and its performance working fine.

Verified on:
Mac OS: 10.15.4
SDK: 9.3.0.v20201029122835
Appc CLI: 8.1.1
JDK: 11.0.4
Node: 10.17.0
Studio: 6.0.0.202005141803
Xcode: 12.0.1
iPhone 7Plus(v14.0), iOS simulator v14.0

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

5 participants