dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.27k stars 1.76k forks source link

XamlC should optimize OnPlatform #5583

Open eerhardt opened 2 years ago

eerhardt commented 2 years ago

Description

In the .NET Podcasts app, it has the following XAML:

https://github.com/microsoft/dotnet-podcasts/blob/00be1bb25b6940fee77f0e7b711a04dea2358ced/src/Mobile/Controls/HeaderControl.xaml#L9-L32

            <OnPlatform x:TypeArguments="View">
                <On Platform="UWP, macOS">
                    <Grid RowDefinitions="*, auto">
                        <SearchBar x:Name="searchBar"
                                   HorizontalOptions="Start"
                                   WidthRequest="460"
                                   Text="{Binding TextToSearch, Source={x:Reference selfMediaElementView}}"
                                   SearchCommand="{Binding SearchCommand, Source={x:Reference selfMediaElementView}}" />
                        <Label Text="See All Categories"
                               Style="{StaticResource BodyLLabelStyle}"
                               TextColor="{StaticResource Primary}"
                               Grid.Row="1"
                               HorizontalOptions="End"
                               Margin="0,0,30,0"
                               IsVisible="{Binding ShowSearchCategories, Source={x:Reference selfMediaElementView}}"
                               FontSize="16">
                            <Label.GestureRecognizers>
                                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped" />
                            </Label.GestureRecognizers>
                        </Label>
                    </Grid>
                </On>
                <On Platform="Android,iOS">
                    <Grid ColumnDefinitions="*,*"
                          Margin="16,12">
                        <Image Source="{AppThemeBinding Light=logo_header_horizontal.png,Dark=logo_color_horizontal_darkmode.png}"
                               HeightRequest="34"
                               WidthRequest="125"
                               VerticalOptions="Center"
                               HorizontalOptions="Start" />
                        <Image Source="search.png"
                               Grid.Column="1"
                               VerticalOptions="Center"
                               HorizontalOptions="End"
                               HeightRequest="20"
                               WidthRequest="20">
                            <Image.GestureRecognizers>
                                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped" />
                            </Image.GestureRecognizers>
                        </Image>
                    </Grid>
                </On>
            </OnPlatform>

When building the app for Android, I'm still seeing the controls inside the <On Platform="UWP, macOS"> clause get created and properties set on them. For example, the SearchBar is only used in UWP, macOS, but in my compiled Android app I see:

private void InitializeComponent()
{
    ReferenceExtension referenceExtension = new ReferenceExtension();
    BindingExtension bindingExtension = new BindingExtension();
    ReferenceExtension referenceExtension2 = new ReferenceExtension();
    BindingExtension bindingExtension2 = new BindingExtension();
    SearchBar searchBar = new SearchBar();

This pattern is used in 3 places in the .NET Podcasts app:

  1. The above
  2. The Player control
  3. The ShowDetailPage.

We should optimize this pattern so when building for Android, the other platforms' controls are not created.

cc @StephaneDelcroix @jonathanpeppers

Steps to Reproduce

  1. git clone https://github.com/microsoft/dotnet-podcasts.git
  2. cd dotnet-podcasts/src/Mobile
  3. dotnet build Microsoft.NetConf2021.Maui.csproj -t:Run -c Release -f net6.0-android -r android-arm64
  4. Inspect the IL of HeaderControl.InitializeComponent()

Version with bug

Preview 14 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android, Windows, macOS

Affected platform versions

All

Did you find any workaround?

No response

Relevant log output

No response

StephaneDelcroix commented 2 years ago

4829 was about the {OnPlatform} markup extension, not the <OnPlatform\> generic construct. Looking if we can apply the same treatment over here...

veikkoeeva commented 2 years ago

I'll add a related use case that led me to find this. It seem to be related also to the linked cases.

The case: use OnPlatform to choose ResourceDictionary on compile time based on platform.

Elaborating.

As for example, I would like to use styles to have:

1) Material 3 on Android. 2) Mica/Acrylic on Windows.

Using default Maui application as an example, I could install MaterialColorUtilities and include it only on Android with appropriate #if-#endif in code (the compiler/linker could optimize it away on unused platforms even without MSBuild conditional based on code preprocessor directives, I suppose):

<ItemGroup Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'android'">
    <PackageReference Include="MaterialColorUtilities" Version="0.1.1" />
    <PackageReference Include="MaterialColorUtilities.Maui" Version="0.1.1" />
  </ItemGroup>

But now it looks to me there is not a good pattern to implement different styles on Android and Windows if one doesn't want to ship platform specific styles to all platforms with the associated resource usage. Which seem to be the topic of this issue too.

But maybe it is worth bring another use case to discussion that is similar if OnPlatform becomes also a construct that the compiler can optimize. Going through setters one-by-one is cumbersome and indeed, not all styles seem to be applicable to all platforms (maybe could be set explicitly to some default value on all platforms or null so every platform is covered).

So, I was thinking easiest fix could be to make the default App.xaml

    <Application.Resources>
        <ResourceDictionary>
            <ResourceDictionary.MergedDictionaries>
                <ResourceDictionary Source="Resources/Styles/Colors.xaml" />
                <ResourceDictionary Source="Resources/Styles/Styles.xaml" />
            </ResourceDictionary.MergedDictionaries>
        </ResourceDictionary>
    </Application.Resources>
</Application>

ResourceDictionary Source paths conditional. But this is not possible. The path needs to be constant to the resource: e.g. I cannot choose Colors.xaml and Styles.xaml by way of multitargeting in MSBuild only, since the whole path needs to be present. And there's no conditional operators available during compile time here, as far as I understand.

As for an example of what I think is a similar effect: Using regex in MSBuild to modify the path in App.xaml during build before compilation so that the could be made to point to the right location. Example: Resources/Styles/Android/Colors.xaml and Resources/Styles/Android/Styles.xaml would be included when compiling on Android target. (And then switch back so that version control would not be affected.)

Using this case as an example, this way the styles could be made appropriate with Material 3 on Android, Windows and other platforms and only those styles be included on compiled applications.

So, maybe it could be useful to extend OnPlatform kind of construct also to include resources conditionally.

jonathanpeppers commented 2 years ago

@veikkoeeva you should file a new issue, this seems off-topic. This issue is about computing the <OnPlatform> element at compile time instead of runtime.

veikkoeeva commented 2 years ago

@jonathanpeppers I'm making a case to choose the ResourceDictionary Source path at compile time based on platform. With some explanation why I think it would be useful. Though it cannot be done at runtime either, of which I may be wrong (I'm not that familiar with Maui/Xamarin). So maybe, the new issue should be the ability to set ResourceDictionary Source using OnPlatform (or achieving the same result with some another mechanism).

I edited the original issue to make my intention clearer. As noted, could be a case for a new issue in any case (if this is indeed not possible as of this writing, I'm not 100% sure).

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.