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 TableView rendering on Android #19473

Closed
wants to merge 3 commits into from
Closed

Conversation

janusw
Copy link
Contributor

@janusw janusw commented Dec 18, 2023

Description of Change

This PR overrides the method TableViewRenderer.OnMeasure, in order to fix bad rendering of TableViews on Android (when custom cells are involved, e.g. with a Picker).

Issues Fixed

Fixes #5924

@janusw janusw requested a review from a team as a code owner December 18, 2023 12:52
@ghost ghost added the community ✨ Community Contribution label Dec 18, 2023
@ghost
Copy link

ghost commented Dec 18, 2023

Hey there @janusw! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@janusw janusw mentioned this pull request Dec 18, 2023
@Eilon Eilon added the area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Dec 18, 2023
* issue dotnet#5924 / dotnet#19321
* note: the VerticalStackLayout is not necessary to reproduce the bug
  (but the proposed fix does not work when it's present)
* proposed by @Bennynation at:
  dotnet#5924 (comment)
* only seems to work if the TableView is not encompassed by a VerticalStackLayout
@janusw
Copy link
Contributor Author

janusw commented Jan 3, 2024

@hartez @StephaneDelcroix Ping! This is a pretty bad blocker for me. The tests mostly look fine (except for one which apparently hasn't finished properly for some reason).

@PureWeen
Copy link
Member

PureWeen commented Jan 4, 2024

@hartez is currently working on a variation based on this PR

@PureWeen PureWeen marked this pull request as draft January 4, 2024 15:43
@janusw
Copy link
Contributor Author

janusw commented Jan 4, 2024

@hartez is currently working on a variation based on this PR

Awesome. 🥳 Thanks for the update!

@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2024

#19723

@hartez hartez removed their request for review January 10, 2024 21:09
@jsuarezruiz
Copy link
Contributor

@PureWeen With #19723 already merged, should we close this one?
@janusw Thanks so much for all your efforts!

@janusw
Copy link
Contributor Author

janusw commented Jan 18, 2024

@PureWeen With #19723 already merged, should we close this one?

Most probably yes, but actually I couldn't really test the fix yet. I'm pretty surprised and disappointed that #19723 is not part of release 8.0.6. @jsuarezruiz Can you tell me why it wasn't included in that release?

@@ -126,6 +126,21 @@ public override SizeRequest GetDesiredSize(double widthConstraint, double height
return base.GetDesiredSize(widthConstraint, heightConstraint);
}

protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec)
Copy link
Member

Choose a reason for hiding this comment

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

How did you reach the conclusion of this fix?

I think there's still some issues around our fix on this one. So, I'm curious to pick your brain how you got here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the idea for this fix was initially posted by @Bennynation at #5924 (comment). Maybe he can tell us how he got there? (I only took his sketch and molded it into a PR.)

Copy link
Member

@PureWeen PureWeen Feb 3, 2024

Choose a reason for hiding this comment

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

I think I finally have a pretty good idea.

The problem stems from this line here
https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/widget/ListView.java;l=1321

When you have an AT_MOST measure, the listview will basically create a scrap copy of itself to measure which confuses the whole xplat system. Because it doesn't know that the platform is just creating an in memory listview to get a height measurement. The code here basically bypasses all of that.

Now just have to figure out if this solution makes things worse, better, or if there's a goldilocks version.

@PureWeen
Copy link
Member

PureWeen commented Feb 5, 2024

Closing in favor of
#20130

Which is somewhat similar.

@PureWeen PureWeen closed this Feb 5, 2024
@janusw
Copy link
Contributor Author

janusw commented Feb 5, 2024

Closing in favor of #20130

Alright, thanks for working on this. When will we have the pleasure of seeing this fix (and #19723) in a released version?

@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter community ✨ Community Contribution platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TableView with Picker
6 participants