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(android): reduce tableView updates #13526

Merged
merged 21 commits into from
Aug 19, 2023
Merged

fix(android): reduce tableView updates #13526

merged 21 commits into from
Aug 19, 2023

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Aug 4, 2022

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 😄 (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.

@hansemannn
Copy link
Collaborator

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

@m1ga
Copy link
Contributor Author

m1ga commented Aug 6, 2022

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.

@hansemannn
Copy link
Collaborator

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

@m1ga
Copy link
Contributor Author

m1ga commented Mar 11, 2023

Finally have a client app with some tableviews 😄

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

👍 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!

@hansemannn
Copy link
Collaborator

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

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

Looks good! APPROVED

@cb1kenobi
Copy link
Contributor

Tested, didn't crash, lets go!

@cb1kenobi cb1kenobi merged commit 911fa84 into master Aug 19, 2023
@m1ga m1ga deleted the androidTableUpdate branch September 23, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants