titanium-sdk icon indicating copy to clipboard operation
titanium-sdk copied to clipboard

fix(android): reduce tableView updates

Open m1ga opened this issue 3 years ago • 3 comments

During older TableView tests I saw that this update function https://github.com/tidev/titanium_mobile/blob/918388a75a98c3f5458d27f6ed035f11883b303e/android/modules/ui/src/java/ti/modules/titanium/ui/TableViewProxy.java#L532 is called multiple times. The shouldUpdate around it doesn't catch all loops.

Putting a log in the update function showed:

  • Without the PR: 43 update logs
  • With this PR: 23 update logs for the example below.

Change: Calling appendRow from Ti doesn't change. Only the internal appendRow calls won't trigger the update() during the loop. It is called at the end of the loop anyways: https://github.com/tidev/titanium_mobile/blob/918388a75a98c3f5458d27f6ed035f11883b303e/android/modules/ui/src/java/ti/modules/titanium/ui/TableViewProxy.java#L532

Also fixed a potential null pointer exception.

Demo

const win = Ti.UI.createWindow();
const tableView = Ti.UI.createTableView({});
var tableData = [];

for (var i = 1; i <= 20; i++) {
	var entryRow = Ti.UI.createTableViewRow({height: 40});
	var entryNameLabel = Ti.UI.createLabel({text: "test"});
	entryRow.add(entryNameLabel);
	tableData.push(entryRow);
}
tableView.data = tableData;

win.add(tableView);
win.open();

Note I've only tested some demo tableViews since I don't have an app with an advanced TableView :smile: (ListView FTW!) So if you have an app with some tables where you add and remove data it would be nice to hear if the updates are quicker now and everything still works.

m1ga avatar Aug 04 '22 18:08 m1ga

Pretty smart! Could this also be an issue with list views or are those not affected?

hansemannn avatar Aug 06 '22 11:08 hansemannn

Pretty smart! Could this also be an issue with list views or are those not affected?

no, the update in the ListView isn't called in a loop as far as I see.

m1ga avatar Aug 06 '22 13:08 m1ga

Any community users that can test this with a more advanced case? We also only use list views

hansemannn avatar Aug 09 '22 21:08 hansemannn

Finally have a client app with some tableviews :smile:

Added the log again and could see:

[INFO]  I/----    : (main) [10343,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip
[INFO]  I/----    : (main) [0,12280] would run update, but skip

:+1: tableview didn't look different compared to 12.0.0. Sadly not that much data so couldn't test if it will improve the performance much. But at least 10 less updates!

m1ga avatar Mar 11 '23 13:03 m1ga

Can we get this tested by someone with more complex table views? We also only use it for a maximum of like 12 cells

hansemannn avatar Mar 11 '23 14:03 hansemannn

Tested, didn't crash, lets go!

cb1kenobi avatar Aug 19 '23 15:08 cb1kenobi