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

ListView Android performance refactor #3235

Open
wants to merge 16 commits into
base: ucr
Choose a base branch
from

Conversation

SusanRatiLane
Copy link
Contributor

@SusanRatiLane SusanRatiLane commented Sep 9, 2024

General items:

  • ant tests passes on my machine

  • I branched from ucr

  • My pull request has ucr as the base

Further, if you've changed the blocks language or another user-facing designer/blocks API (added a SimpleProperty, etc.):

  • I have updated the corresponding version number in YaVersion.java
  • I have updated the corresponding upgrader in YoungAndroidFormUpgrader.java (components only)
  • I have updated the corresponding entries in versioning.js

^^ I have actually never done this, so we'll need to talk.

Description

This is a performance refactor submitted by @patryk84a in February 2024. He also included some methods for adding and removing items that the class should probably have. I included implementations for these in iOS.

He also included one aesthetic property for which I included a stub for iOS.

Fixes #2857

Original PR (now closed): #3099

Co-author: @patryk84a

@SusanRatiLane SusanRatiLane requested a review from a team September 9, 2024 18:23
@SusanRatiLane SusanRatiLane marked this pull request as draft September 10, 2024 15:23
@SusanRatiLane
Copy link
Contributor Author

SusanRatiLane commented Sep 10, 2024

Hmm. I thought the tests were passing, but two ListView ones are not. Patryk's changes have broken an assumption the tests are making, but I'm trying to sort out what it is.

ETA: Tests now passing.

@SusanRatiLane SusanRatiLane marked this pull request as ready for review September 12, 2024 15:51
@SusanRatiLane SusanRatiLane added this to the nb199 milestone Sep 12, 2024
@patryk84a
Copy link
Contributor

The HintSearchText property has been removed. Maybe instead for Android add a fixed hint text but from the Android resources "@android:string/search_go", then we should get the string in the language set in the phone, without the possibility of defining your own, which would be compatible with IOS

@SusanRatiLane
Copy link
Contributor Author

The HintSearchText property has been removed. Maybe instead for Android add a fixed hint text but from the Android resources "@android:string/search_go", then we should get the string in the language set in the phone, without the possibility of defining your own, which would be compatible with IOS

Actually, that was removed because someone else wrote a PR that adds the property for both Android and iOS. I should probably have mentioned that in the PR description.

Copy link
Member

@ewpatton ewpatton left a comment

Choose a reason for hiding this comment

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

In addition to the file level and inline comments, the LISTVIEW_COMPONENT_VERSION value in YaVersion needs to be bumped to the change in the properties, and the appropriate code paths in YoungAndroidFormUpgrader.java and versioning.js need to be added.

@patryk84a
Copy link
Contributor

I guess I can't make any changes here, right?

@SusanRatiLane
Copy link
Contributor Author

I guess I can't make any changes here, right?

I should be able to give you access. I'll take a look.

Change-Id: I6d9d69a09b18aaf32d908be32bd2b84609b597cb
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.

3 participants