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 for 3848 - FlexLayout doesn't measure content #6085

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

nogginbox
Copy link
Contributor

Description of Change

I've fixed FlexLayoutManager.Measure so that it measures its children's frames to work out the bounding box of the content. The current implementation ignores the children completely.

This code sample behaves as expected and is sized correctly based on the content:

<StackLayout>
    <Label>Top</Label>
    <FlexLayout Grid.Row="0" Grid.Column="1" Wrap="Wrap" AlignContent="Start" AlignItems="Start">
        <BoxView BackgroundColor="AliceBlue" HeightRequest="40" WidthRequest="24" />
        <BoxView BackgroundColor="BlanchedAlmond" HeightRequest="24" WidthRequest="24" />
        <BoxView BackgroundColor="Chartreuse" HeightRequest="32" WidthRequest="24" />
        <BoxView BackgroundColor="DeepPink" HeightRequest="16" WidthRequest="24" />
        <BoxView BackgroundColor="Firebrick" HeightRequest="24" WidthRequest="24" />
        <BoxView BackgroundColor="Gold" HeightRequest="40" WidthRequest="24" />
        <BoxView BackgroundColor="HotPink" HeightRequest="16" WidthRequest="24" />
        <BoxView BackgroundColor="AliceBlue" HeightRequest="40" WidthRequest="24" />
        <BoxView BackgroundColor="BlanchedAlmond" HeightRequest="24" WidthRequest="24" />
        <BoxView BackgroundColor="Chartreuse" HeightRequest="32" WidthRequest="24" />
        <BoxView BackgroundColor="DeepPink" HeightRequest="16" WidthRequest="24" />
        <BoxView BackgroundColor="Firebrick" HeightRequest="24" WidthRequest="24" />
        <BoxView BackgroundColor="Gold" HeightRequest="40" WidthRequest="24" />
        <BoxView BackgroundColor="HotPink" HeightRequest="16" WidthRequest="24" />
    </FlexLayout>
    <Label>Bottom</Label>
</StackLayout>

The implementation was inspired and informed by StackLayoutManager.Measure.

Issues Fixed

@Eilon Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Apr 13, 2022
@rmarinho rmarinho requested a review from hartez April 14, 2022 18:11
@rmarinho
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@nogginbox
Copy link
Contributor Author

nogginbox commented Apr 15, 2022

I'm not sure the build error is caused by my changes:

Unhandled exception. System.Net.WebException: The remote server returned an error: (503) Service Unavailable.
   at System.Net.HttpWebRequest.GetResponse()
   at System.Net.WebClient.GetWebResponse(WebRequest request)
  ...
##[error]The process '/Users/builder/azdo/_work/_tool/provisionator/0.2.629/x64/provisionator' failed with exit code null

How do I check?

@nogginbox
Copy link
Contributor Author

The tests got rerun and everything succeeded this time.

@rmarinho
Copy link
Member

Do you think you can add a unit test for this?

@mattleibow mattleibow added this to the 6.0.300-rc.3 milestone Apr 20, 2022
@mattleibow mattleibow added the t/bug Something isn't working label Apr 20, 2022
@nogginbox
Copy link
Contributor Author

nogginbox commented Apr 20, 2022

@rmarinho I'm on holiday this week, but next week I'll look at the unit tests for StackLayoutManager and see if I can base them on that.

@nogginbox nogginbox force-pushed the fix-3848 branch 2 times, most recently from 3c249f7 to def5751 Compare April 28, 2022 17:50
@mattleibow
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Just marking this red so we don't forget to add a test. Just had an issue right now that appeared after a few merges because we did not have a test in a critical path. I am becoming more like Stephane and feeling test-greedy.

@nogginbox
Copy link
Contributor Author

nogginbox commented Apr 29, 2022

@mattleibow I'm working on the tests, but have managed to completely bork my VS setup trying to get the latest Maui source to run. I think you've seen my call for help discussion. This implementation is running in my own project, but as a new class called NogginFlexLayout, so I'm happy it works, but does need formal tests in place.

There were no tests for FlexLayout before so I've based them on the StackLayout ones.

@hartez
Copy link
Contributor

hartez commented Apr 29, 2022

This all looks good, so rather than hold things up waiting for tests, I'm going to go ahead and merge this.

@nogginbox If you want to PR the tests later when you have a working dev environment again, that would be great; otherwise, I'll add some myself when I've got a moment.

@hartez hartez merged commit 06989b8 into dotnet:main Apr 29, 2022
@nogginbox nogginbox deleted the fix-3848 branch May 4, 2022 07:00
@nogginbox
Copy link
Contributor Author

Started unit tests on this draft PR #6800

@Palku
Copy link

Palku commented May 23, 2022

Is FlexLayout sizing only fixed for Windows or should it work in Android in RC3 as well?
For me it works targeting Windows but not Android. In Android I have to do some ForceLayout() or toggle views back and forth to make the size update correctly.

@nogginbox
Copy link
Contributor Author

nogginbox commented May 23, 2022

This fix should work in all OS versions as there is no OS specific code in it. I did not test in Android though.

If the layout is correct on the first draw and only breaks when the size changes, then that sounds like a different issue.

@Palku
Copy link

Palku commented May 24, 2022

This is my page

<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             x:Class="MauiApp5.MainPage">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="80" />
            <RowDefinition Height="Auto" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition/>
        </Grid.ColumnDefinitions>
        <Label Grid.Row="0" Grid.Column="0" Text="Header" BackgroundColor="Red"/>
        <FlexLayout Grid.Row="1" Grid.Column="0" Wrap="Wrap" JustifyContent="Start" AlignItems="Start" AlignContent="Start" BackgroundColor="Green">
            <Button Text="Button 1" HeightRequest="80" WidthRequest="160"/>
            <Button Text="Button 2" HeightRequest="80" WidthRequest="160"/>
            <Button Text="Button 3" HeightRequest="80" WidthRequest="160"/>
            <Button Text="Button 4" HeightRequest="80" WidthRequest="160"/>
            <Button Text="Button 5" HeightRequest="80" WidthRequest="160"/>
            <Button Text="Button 6" HeightRequest="80" WidthRequest="160"/>
        </FlexLayout>
        <Label Grid.Row="2" Grid.Column="0" Text="Remaining space" BackgroundColor="Yellow"/>
    </Grid>
</ContentPage>

It's the other way around, it doesn't calculate size correctly on the first draw.
The FlexLayout gets height 0 in Android but works fine in Windows.

image
image

First image is Android(Android 11 Api 30), second is Windows.
If you think it's a different bug I'll create a issue.

@nogginbox
Copy link
Contributor Author

If it draws fine after a redraw then it seems like a different bug. It seems like the Measure method I fixed is not being called at all, or the things it is trying to measure do not have size yet.

Either way I think it needs a new bug ticket. No one will be tracking comments on a fixed PR.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter t/bug Something isn't working
Projects
None yet
6 participants