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 OnPlatform + Setter when no match for current platform #17061

Merged
merged 3 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="utf-8" ?>
<ContentPage
x:Class="Maui.Controls.Sample.Issues.Issue12604"
xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml">
<ContentPage.Resources>
<Style x:Key="ButtonStyle1" TargetType="Button">
<Setter Property="FontSize" Value="{OnPlatform iOS=36}" />
<Setter Property="TextColor" Value="{OnPlatform iOS=Red}" />
</Style>
<Style x:Key="ButtonStyle2" TargetType="Button">
<Setter Property="FontSize">
<OnPlatform x:TypeArguments="x:Double">
<On Platform="iOS">36</On>
</OnPlatform>
</Setter>
<Setter Property="TextColor">
<OnPlatform x:TypeArguments="Color">
<On Platform="iOS">Red</On>
</OnPlatform>
</Setter>
</Style>
</ContentPage.Resources>

<ScrollView WidthRequest="400">
<VerticalStackLayout
Padding="10,0"
Spacing="15"
VerticalOptions="Center">
<Label Text="FontSize for all buttons below should be the same: 36 on iOS, default on other platforms"
AutomationId="TestLabel" />
<Button Style="{StaticResource ButtonStyle1}" Text="Button label" />
<Button Style="{StaticResource ButtonStyle2}" Text="Button label" />
<Button FontSize="{OnPlatform iOS=36}" Text="Button label" />
<Button Text="Button label">
<Button.FontSize>
<OnPlatform x:TypeArguments="x:Double">
<On Platform="iOS">36</On>
</OnPlatform>
</Button.FontSize>
</Button>
</VerticalStackLayout>
</ScrollView>
</ContentPage>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Xaml;
using Microsoft.Maui.Platform;
using Microsoft.Maui;
using System.Linq;
using System;

#if IOS
using UIKit;
using CoreGraphics;
#endif

namespace Maui.Controls.Sample.Issues
{
//[XamlCompilation(XamlCompilationOptions.Compile)]
[Issue(IssueTracker.Github, 12604, "Setter showing a Build Error when using XAML OnPlatform Markup Extension", PlatformAffected.All)]
public partial class Issue12604 : ContentPage
{
public Issue12604()
{
InitializeComponent();
Copy link
Contributor

Choose a reason for hiding this comment

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

please look at the other tests, to see how we test for both runtime and compiled Xaml

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ public IEnumerable<Instruction> ProvideValue(VariableDefinitionReference vardefr
if (SetterValueIsCollection(bpRef, module, node, context))
yield break;

// valueNode is null, for example, when OnPlatform doesn't have a match for the current platform, so the property should not be set
if (valueNode == null)
throw new XamlParseException("Missing Value for Setter", (IXmlLineInfo)node);
yield break;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this will have effects outside of OnPlatform

Copy link
Member Author

Choose a reason for hiding this comment

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

@StephaneDelcroix - Yes, this change can have effects outside of OnPlatform, but previously those cases failed with the exception here, so the risk of breaking anything, making things worse, seems small - it previously just error'ed out. That said, if you can tell me how this code is actually used outside of OnPlatform, like suggest some test scenarios, I can test those.

Copy link
Contributor

Choose a reason for hiding this comment

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

The property would not be set, right?. So for example, here:

<Style x:Key="ButtonStyle0" TargetType="Button">
    <Setter Property="FontSize" Value="{OnPlatform iOS=36}" />
</Style>

The FontSize property on Android, would have the default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right


//if it's an elementNode, there's probably no need to convert it
if (valueNode is IElementNode)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,13 @@ public object ProvideValue(IServiceProvider serviceProvider)
if (!TryGetValueForPlatform(out var value))
{
if (bp != null)
return bp.GetDefaultValue(valueProvider.TargetObject as BindableObject);
{
object targetObject = valueProvider.TargetObject;

if (targetObject is Setter)
return null;
else return bp.GetDefaultValue(targetObject as BindableObject);
}
if (propertyType.IsValueType)
return Activator.CreateInstance(propertyType);
return null;
Expand Down
21 changes: 21 additions & 0 deletions src/Controls/tests/UITests/Tests/Issues/Issue12064.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using Microsoft.Maui.Appium;
using NUnit.Framework;

namespace Microsoft.Maui.AppiumTests.Issues
{
public class Issue12064 : _IssuesUITest
{
public Issue12064(TestDevice device)
: base(device)
{ }

public override string Issue => "Setter showing a Build Error when using XAML OnPlatform Markup Extension";

[Test]
public void Issue12064Test()
{
App.WaitForElement("TestLabel");
VerifyScreenshot();
}
}
}
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.