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

[iOS] ListView with UnevenRows and Cell Heights will no longer be slow to load #994

Merged
merged 3 commits into from Jun 23, 2017

Conversation

Projects
None yet
4 participants
@samhouts
Member

samhouts commented Jun 15, 2017

Description of Change

#454 turned off row height estimation for ListViews enabling HasUnevenRows when the first ViewCell had a non-zero Height. This requires every row to be measured on initial load to prepare the scroll area, so large item sources developed a significant load time.

Re-enabling estimation is ideal for performance; however, it will regress the issue resolved by #454. As a compromise, this change will cache known ViewCell heights to be used by EstimateHeight, which will be called once when the ListView loads and allow the scroll area to be accurately estimated (preserving the resolved status of Bugzilla 43313) while still providing a value for estimation quick enough to restore performance.

Users may still see that the ViewCell constructors will be called more times than they were before #454, since we still need to query each ViewCell for its Height. However, this should be a value closer to 1x instead of 3-4x.

If users are not concerned about adding rows and scrolling to them with smooth animation, users may now also specify a RowHeight on the ListView. This value will be used for initial estimation, and the actual measured heights of the rows will be used when they are displayed. This, in effect, returns the behavior back to pre-#454, reducing the constructor calls to just those necessary for initial display.

Bugs Fixed

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@rmarinho

Seems correct.. A little more memory i think but i think is a good tradeoff

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/ListViewRenderer.cs
// We want to make sure we reset the cached defined row heights whenever this is called.
// Failing to do this will regress Bugzilla 43313 (strange animation when adding rows with uneven heights)
source.CacheDefinedRowHeights();

This comment has been minimized.

@rmarinho

rmarinho Jun 22, 2017

Member

we are using source without checking if it's null.. not sure ?.will be more correct

@rmarinho

rmarinho Jun 22, 2017

Member

we are using source without checking if it's null.. not sure ?.will be more correct

This comment has been minimized.

@samhouts

samhouts Jun 22, 2017

Member

Good call! Fixed.

@samhouts

samhouts Jun 22, 2017

Member

Good call! Fixed.

@rmarinho rmarinho merged commit a9f6acb into master Jun 23, 2017

6 checks passed

OSX-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3795, ignored: 10
Details
Windows-Release-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Release Unit Tests : Tests passed: 3795, ignored: 10
Details
iOS10-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

rmarinho added a commit that referenced this pull request Jun 23, 2017

[iOS] ListView with UnevenRows and Cell Heights will no longer be slo…
…w to load (#994)

* Add repro for 56896

* [iOS] Cached defined row heights for estimation

* [iOS] Null check source

@rmarinho rmarinho deleted the fix-bugzilla56896 branch Jun 26, 2017

assemhakmeh added a commit to assemhakmeh/Xamarin.Forms that referenced this pull request Jul 28, 2017

Merge branch '2.3.5' into ios-fastrenderers
* 2.3.5:
  [UWP] Fixes for usage of XF with .net native toolchain (#1024)
  [UWP] Make sure to update HitTestVisible when IsEnable changes (#1015)
  [Android] Dispose check before setting properties on Button (#1013)
  Add missing member variable to FormsApplicationActivity
  Fix NRE when background color of button set in FormsApplicationActivity (#1010)
  Fix border on android buttons  (#941)
  [iOS] ListView with UnevenRows and Cell Heights will no longer be slow to load (#994)
  Set the Id field for Android Views created by Forms #1004
  Fix build
  Fix possible crash on API 21+ at launch when using Holo theme and FormsApplicationActivity (#961)
  [Android] Remove the ". " on empty labels (Accessibility) on Fastrenderers (#915)
  Remove debug outputs (#1008)
  Add check for instance of UITableView (#885)
  [XamlC] fix release builds of Xaml Unit Tests
  Dispose check on ButtonRenderer (#975)
  [previewer] make sure we do not crash even if the previewer doesn't s… (#946)
  [XamlC] fix build
  Remove VisualElement finalizer (#918)
  [XamlC] process symbols if DebugType is set (#925)
@fschwiet

This comment has been minimized.

Show comment
Hide comment
@fschwiet

fschwiet Jul 28, 2017

Will there be an update to the documentation? https://developer.xamarin.com/api/property/Xamarin.Forms.ListView.HasUnevenRows/#Remarks states:

When the app developer sets the ListView.HasUnevenRows property to true, the behavior of the list view still depends on the ListView.RowHeight property. First, if the developer either does not set the ListView.RowHeight property or sets it to -1, list view items are autosized to fit their contents. This is the desired behavior and the intended use case for a ListView.HasUnevenRows value of true, as noted above. Second, if the developer sets the ListView.RowHeight property to 0 or to a negative value other than -1, then all rows in the ListView will, irrespective of the height of their content, have the default height for the system. Third, and finally, if the developer sets ListView.RowHeight to a positive value, then all rows in the ListView will, irrespective of the height of their content, be as tall as ListView.RowHeight, as if ListView.HasUnevenRows had been set to false.

fschwiet commented Jul 28, 2017

Will there be an update to the documentation? https://developer.xamarin.com/api/property/Xamarin.Forms.ListView.HasUnevenRows/#Remarks states:

When the app developer sets the ListView.HasUnevenRows property to true, the behavior of the list view still depends on the ListView.RowHeight property. First, if the developer either does not set the ListView.RowHeight property or sets it to -1, list view items are autosized to fit their contents. This is the desired behavior and the intended use case for a ListView.HasUnevenRows value of true, as noted above. Second, if the developer sets the ListView.RowHeight property to 0 or to a negative value other than -1, then all rows in the ListView will, irrespective of the height of their content, have the default height for the system. Third, and finally, if the developer sets ListView.RowHeight to a positive value, then all rows in the ListView will, irrespective of the height of their content, be as tall as ListView.RowHeight, as if ListView.HasUnevenRows had been set to false.

@samhouts samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment