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

Reduce updateItem calls #4012

Merged
merged 18 commits into from
Jul 11, 2023
Merged

Conversation

SMQuazi
Copy link
Contributor

@SMQuazi SMQuazi commented Jul 2, 2023

Description:

Currently, placing a print in the updateItem callback for a list shows that the function is being called for all items that are visible for every change made. This makes expensive tasks repeat whenever there is any sort of change to the list, and locks up the screen for a long time. This change aims to only call the function for a given item only once, when the item becomes visible.

The unit test shows a slight difference, however placing a print statement in the updateItem function seems to show a drastically more apparent change, as show below.

also, Fixes

#3826 (partially - for lists only)

Print out of list IDs - on loading of a list

currently

7.9.0.6.3.4.5.8.10.1.2.5.8.9.3.4.6.7.10.0.1.2.1.3.5.8.7.9.10.0.2.4.6.0.1.2.4.10.3.5.6.7.8.9.3.4.7.8.10.0.2.6.9.1.5.0.3.5.7.10.1.2.4.6.8.9.7.8.9.0.3.4.5.6.10.1.2.6.7.8.2.3.4.9.10.0.1.5.6.7.8.9.0.2.5.10.1.3.4.8.9.10.1.2.4.5.7.0.3.6.1.2.3.4.5.6.7.9.10.0.8.0.5.8.10.1.2.3.4.6.7.9.10.0.1.2.4.5.6.7.3.8.9.1.3.5.10.9.0.2.4.6.7.8.3.5.7.8.9.0.2.4.6.10.1.3.4.8.10.0.2.5.6.7.9.1.8.10.2.7.3.4.5.6.9.0.1.1.2.3.10.9.0.4.5.6.7.8.1.2.5.8.0.3.4.6.7.9.10.8.9.4.5.7.3.6.10.0.1.2.0.1.2.4.8.3.5.6.7.9.10.3.7.0.2.5.6.8.9.10.1.4.1.3.7.9.10.0.2.4.5.6.8.5.6.8.10.0.2.4.9.1.3.7.10.0.2.8.9.6.7.1.3.4.5.1.3.5.8.10.0.2.4.6.7.9.

with the change

7.8.10.9.0.1.2.3.4.5.6.2.3.4.10.0.1.5.6.7.8.9.

Print out of list IDs - scrolling down 2 items

current

1.4.6.7.8.9.0.2.3.5.10.10.0.1.5.7.8.2.3.4.6.9.3.5.6.7.8.10.0.1.2.4.9.8.9.10.0.4.5.6.7.1.2.3.4.5.7.8.9.11.3.2.6.10.1.2.3.6.8.9.11.1.5.7.10.4.11.2.6.7.8.9.1.3.4.5.10.8.4.6.7.5.9.10.11.1.2.3.5.7.9.1.2.6.8.10.11.3.4.10.11.1.3.5.7.8.2.4.6.9.6.10.1.2.3.4.5.7.8.9.11.7.11.3.4.5.6.8.9.1.2.10.6.7.8.9.1.2.3.4.10.5.11.7.9.10.11.4.5.6.8.1.2.3.10.11.3.5.4.6.7.8.9.1.2.2.4.6.8.9.10.1.3.5.7.11.3.5.7.11.1.2.8.9.10.4.6.2.3.5.6.7.9.11.1.4.8.10.8.11.3.4.6.7.10.1.2.5.9.1.2.6.7.8.10.11.3.4.5.9.11.1.3.4.7.8.9.10.2.5.6.11.4.6.8.10.7.9.1.2.3.5.5.6.7.8.9.10.1.3.11.2.4.6.7.9.11.3.5.4.8.10.1.2.2.5.8.10.1.3.4.6.7.9.11.3.5.6.8.11.1.4.7.9.10.2.9.10.11.1.2.3.5.8.4.6.7.6.8.10.1.2.3.9.11.4.5.7.3.4.6.9.10.11.1.2.5.7.8.9.11.1.3.6.7.8.2.4.5.10.4.5.11.1.3.6.7.8.9.10.2.3.4.5.10.12.2.7.8.9.11.6.2.5.7.10.11.12.3.4.6.8.9.2.7.8.9.10.3.4.5.6.11.12.7.8.9.10.11.2.3.4.5.6.12.7.9.11.10.12.2.3.4.5.6.8.4.5.8.11.12.10.2.3.6.7.9.8.11.2.3.4.5.12.6.7.9.10.2.3.4.9.10.5.6.7.8.11.12.3.4.7.10.11.12.2.6.8.9.5.6.10.9.11.2.3.4.5.7.8.12.12.5.8.10.11.7.9.2.3.4.6.7.9.4.5.6.8.10.11.2.3.12.3.5.6.8.10.11.12.2.4.7.9.5.6.8.9.11.12.3.4.7.10.2.4.5.6.8.2.3.7.9.10.11.12.2.4.5.12.11.3.6.7.8.9.10.10.11.5.7.8.6.9.12.2.3.4.2.3.4.5.10.12.6.7.8.9.11.5.6.7.8.9.2.3.4.10.11.12.7.8.9.12.2.4.5.6.3.10.11.4.8.10.11.12.3.5.6.7.9.2.3.5.6.7.8.9.10.2.11.12.4.5.6.9.10.2.3.8.11.12.4.7.

with changes

11.12.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@andydotxyz
Copy link
Member

Thanks for looking at this. However every item must also update if Refresh is call. I think that was missed or not tested in the unit test you added.

@SMQuazi
Copy link
Contributor Author

SMQuazi commented Jul 2, 2023

Changes made and tests passed (locally atleast. waiting on PR checks). It's not where I want it to be performance wise. Still getting less calls (description updated) but would ideally like it only to call updateItem 1x when it becomes visible. Still looking.

@SMQuazi
Copy link
Contributor Author

SMQuazi commented Jul 3, 2023

Looks like it's working as desired and all green.

@coveralls
Copy link

coveralls commented Jul 3, 2023

Coverage Status

coverage: 66.18% (+0.03%) from 66.149% when pulling c7bafe1 on SMQuazi:reduce-list-item-updates into e050d79 on fyne-io:develop.

widget/list.go Outdated Show resolved Hide resolved
widget/list.go Outdated Show resolved Hide resolved
@SMQuazi
Copy link
Contributor Author

SMQuazi commented Jul 8, 2023

Reworked to adhere to the current flow with minimal changes.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. That does look better. I didn't have time to test the code locally but I added some comments inline.

widget/list.go Outdated Show resolved Hide resolved
widget/list.go Show resolved Hide resolved
widget/list.go Outdated Show resolved Hide resolved
@SMQuazi SMQuazi requested review from Jacalz and andydotxyz July 9, 2023 05:17
widget/list.go Outdated Show resolved Hide resolved
Co-authored-by: Jacob Alzén <[email protected]>
@SMQuazi SMQuazi requested a review from Jacalz July 9, 2023 13:32
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Looks like a great improvement, thanks for helping with this.

I guess we need to match RefreshItem with table and tree equivalents too, but that could be a follow up PR.

@SMQuazi
Copy link
Contributor Author

SMQuazi commented Jul 11, 2023

Looks like a great improvement, thanks for helping with this.

I guess we need to match RefreshItem with table and tree equivalents too, but that could be a follow up PR.

Thanks for guiding me through my first PR here. Hopefully more to come.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This looks good to me as well. Congratulations on landing your first contribution to Fyne. We appreciate your work. Don't hesitate to reach out if you have any questions or want to discuss anything. A lot of discussions around Fyne take place in the #fyne and #fyne-contributors channels on Gophers Slack if you wish to join :)

@Jacalz Jacalz merged commit d4b95e1 into fyne-io:develop Jul 11, 2023
11 checks passed
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.

4 participants