[X] Simplify OnPlatformExtension #4829
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| #nullable disable | ||
|
|
||
| namespace Microsoft.Maui.Controls.Xaml | ||
| { | ||
| class SimplifyOnPlatformVisitor : IXamlNodeVisitor | ||
| { | ||
| public SimplifyOnPlatformVisitor(string targetFramework) | ||
| { | ||
| TargetFramework = targetFramework; | ||
| } | ||
|
|
||
| public string TargetFramework { get; } | ||
|
|
||
| public TreeVisitingMode VisitingMode => TreeVisitingMode.BottomUp; | ||
| public bool StopOnDataTemplate => false; | ||
| public bool VisitNodeOnDataTemplate => true; | ||
| public bool StopOnResourceDictionary => false; | ||
| public bool IsResourceDictionary(ElementNode node) => false; | ||
| public bool SkipChildren(INode node, INode parentNode) => false; | ||
|
|
||
| public void Visit(ValueNode node, INode parentNode) | ||
| { | ||
| } | ||
|
|
||
| public void Visit(MarkupNode node, INode parentNode) | ||
| { | ||
| } | ||
|
|
||
| public void Visit(ElementNode node, INode parentNode) | ||
| { | ||
| if (node.XmlType.Name != nameof(OnPlatformExtension) || node.XmlType.NamespaceUri != XamlParser.MauiUri) | ||
| return; | ||
| if (string.IsNullOrEmpty(TargetFramework)) | ||
| return; | ||
|
|
||
| string target = null; | ||
| if (TargetFramework.Contains("-android")) | ||
| target = nameof(OnPlatformExtension.Android); | ||
| if (TargetFramework.Contains("-ios")) | ||
| target = nameof(OnPlatformExtension.iOS); | ||
| if (TargetFramework.Contains("-macos")) | ||
| target = nameof(OnPlatformExtension.macOS); | ||
| if (TargetFramework.Contains("-maccatalyst")) | ||
|
There was a problem hiding this comment. Is there no "Windows" platform extensions? Or do we not care in that case? There was a problem hiding this comment. This is kind of interesting since -windows could be wpf, or winforms or winappsdk in theory right? We only support winappsdk now, but who knows in the future... So perhaps target framework is not enough to check for -windows at some point... There was a problem hiding this comment. there's no 1-1 mapping between .net TFM for windows and Maui targeted platform There was a problem hiding this comment. What are the "platform extensions" for windows? How does that switch happen at runtime? There was a problem hiding this comment. https://github.com/dotnet/maui/blob/main/src/Controls/src/Xaml/MarkupExtensions/OnPlatformExtension.cs#L13-L21 There was a problem hiding this comment. #4491 added So, today we only have 1 windows target, perhaps we should optimize for the now. If we need to, in the future could we optimize such that at least There was a problem hiding this comment. this might be a good time to review our current platform list, and names, and what defines a platform. should iPadOS be it's own platform, what about deprecating windows in favour of winui, wpf, ... There was a problem hiding this comment. Am I reading this correctly, in that if it's not windows, then there's just no optimization in the code path like there is for the other known tfm's? If that's the case I think this is fine as is and we should try and merge it in. There was a problem hiding this comment. yep, that's it, if the TFM is unlisted, we do nothing |
||
| target = nameof(OnPlatformExtension.MacCatalyst); | ||
|
|
||
| if (target is null) | ||
| return; | ||
|
|
||
| if ( node.Properties.TryGetValue(new XmlName("", target), out INode targetNode) | ||
| || node.Properties.TryGetValue(new XmlName("", nameof(OnPlatformExtension.Default)), out targetNode)) | ||
| { | ||
| if (!ApplyPropertiesVisitor.TryGetPropertyName(node, parentNode, out XmlName name)) | ||
| return; | ||
| if (parentNode is IElementNode parentEnode) | ||
| parentEnode.Properties[name] = targetNode; | ||
| } | ||
| else //no prop for target and no Default set | ||
| { | ||
| if (!ApplyPropertiesVisitor.TryGetPropertyName(node, parentNode, out XmlName name)) | ||
| return; | ||
| //if there's no value for the targetPlatform, ignore the node. | ||
| //this is slightly different than what OnPlatform does (return default(T)) | ||
| if (parentNode is IElementNode parentEnode) | ||
| parentEnode.Properties.Remove(name); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public void Visit(RootNode node, INode parentNode) | ||
| { | ||
| } | ||
|
|
||
| public void Visit(ListNode node, INode parentNode) | ||
| { | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.OnPlatformOptimization"> | ||
| <StackLayout> | ||
| <Label x:Name="label0" | ||
| Text="{OnPlatform Android='john', iOS='paul', Default='ringo' }" | ||
| Padding="{OnPlatform iOS='2,3,4,5'}" /> | ||
|
|
||
| </StackLayout> | ||
| </ContentPage> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Microsoft.Maui.Controls; | ||
| using Microsoft.Maui.Controls.Core.UnitTests; | ||
| using Microsoft.Maui.Essentials; | ||
| using NUnit.Framework; | ||
| using Mono.Cecil.Cil; | ||
| using Mono.Cecil; | ||
|
|
||
| namespace Microsoft.Maui.Controls.Xaml.UnitTests | ||
| { | ||
| public partial class OnPlatformOptimization : ContentPage | ||
| { | ||
| public OnPlatformOptimization() | ||
| { | ||
| InitializeComponent(); | ||
| } | ||
|
|
||
| public OnPlatformOptimization(bool useCompiledXaml) | ||
| { | ||
| //this stub will be replaced at compile time | ||
| } | ||
|
|
||
| [TestFixture] | ||
| public class Tests | ||
| { | ||
| [SetUp] public void Setup() => Device.PlatformServices = new MockPlatformServices(); | ||
| [TearDown] public void TearDown() => Device.PlatformServices = null; | ||
|
|
||
| [Test] | ||
| public void OnPlatformExtensionsAreSimplified([Values("net6.0-ios", "net6.0-android")] string targetFramework) | ||
| { | ||
| MockCompiler.Compile(typeof(OnPlatformOptimization), out var methodDef, targetFramework); | ||
| Assert.That(!methodDef.Body.Instructions.Any(instr=>InstructionIsOnPlatformExtensionCtor(methodDef, instr)), "This Xaml still generates a new OnPlatformExtension()"); | ||
| } | ||
|
|
||
| bool InstructionIsOnPlatformExtensionCtor(MethodDefinition methodDef, Mono.Cecil.Cil.Instruction instruction) | ||
| { | ||
| if (instruction.OpCode != OpCodes.Newobj) | ||
| return false; | ||
| if (!(instruction.Operand is MethodReference methodRef)) | ||
| return false; | ||
| if (!Build.Tasks.TypeRefComparer.Default.Equals(methodRef.DeclaringType, methodDef.Module.ImportReference(typeof(OnPlatformExtension)))) | ||
| return false; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TargetFrameworkdoesn't change for the lifetime of this object. Does it make sense to do this switch every time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
despite being in the Xaml project, this code is only ever executed by the compiler, and we're usually ok to sacrifice a few ms over code simplicity and readability. I'm 100% sure there's an alternative code that is as readable, as debuggable and as simple, but I'm not sure it will ever save the time we currently spend discussing it :)