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

[Windows] Remove workaround for label text decorations #22650

Merged

Conversation

MartyIX
Copy link
Collaborator

@MartyIX MartyIX commented May 25, 2024

Description of Change

According to microsoft/microsoft-ui-xaml#1093 (comment) the issue microsoft/microsoft-ui-xaml#1093 should be fixed in WinUI 3 in WinAppSDK 1.4. Removed code was added in #726 which was three years ago. The issue was fixed last year.

Performance impact

Two measurements for main branch:

Two measurements for the PR branch:

image

-> 98% improvement. It's really big so that's why I did second measurements but it appears that it really has big impact (on all labels). Anyway, even the measurements were wrong, from performance point of view doing less takes less time.

Issues Fixed

Contributes to #21787

Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label May 25, 2024
@MartyIX MartyIX changed the title [Windows] Optimize text decorations for label (1) [Windows] Remove workaround for label text decorations May 25, 2024
@MartyIX MartyIX requested a review from jsuarezruiz May 25, 2024 22:12
@MartyIX MartyIX added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) platform/windows 🪟 labels May 25, 2024
@MartyIX
Copy link
Collaborator Author

MartyIX commented May 26, 2024

I have verified that the code still works using sandbox (branch, MartyIX@b9c7f17) which simply does:

MainPage.xaml.cs

<ContentPage
    xmlns="http:https://schemas.microsoft.com/dotnet/2021/maui"
    xmlns:x="http:https://schemas.microsoft.com/winfx/2009/xaml"
    x:Class="Maui.Controls.Sample.MainPage"
    xmlns:local="clr-namespace:Maui.Controls.Sample">
  <VerticalStackLayout>
    <Label x:Name="myLabel" Text="This is the text"/>
    <Button Text="Click me" Clicked="Button_Clicked"/>
  </VerticalStackLayout>
</ContentPage>

MainPage.cs

using System;
using System.Collections.ObjectModel;
using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Maui;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;

namespace Maui.Controls.Sample
{
	public partial class MainPage : ContentPage
	{
		private int i = 0;

		public MainPage()
		{
			InitializeComponent();
		}

		private void Button_Clicked(object sender, EventArgs e)
		{
			i++;
			if (i % 2 == 1)
			{
				myLabel.TextDecorations = TextDecorations.Underline | TextDecorations.Strikethrough;
			}
			else
			{
				myLabel.TextDecorations = TextDecorations.None;
			}
		}
    }
}

@MartyIX MartyIX marked this pull request as ready for review May 26, 2024 08:26
@MartyIX MartyIX requested a review from a team as a code owner May 26, 2024 08:26
@MartyIX MartyIX requested review from Eilon and Foda and removed request for Eilon May 26, 2024 08:26
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@jsuarezruiz
Copy link
Contributor

I have verified that the code still works using sandbox (branch, MartyIX@b9c7f17) which simply does:

MainPage.xaml.cs

<ContentPage
    xmlns="http:https://schemas.microsoft.com/dotnet/2021/maui"
    xmlns:x="http:https://schemas.microsoft.com/winfx/2009/xaml"
    x:Class="Maui.Controls.Sample.MainPage"
    xmlns:local="clr-namespace:Maui.Controls.Sample">
  <VerticalStackLayout>
    <Label x:Name="myLabel" Text="This is the text"/>
    <Button Text="Click me" Clicked="Button_Clicked"/>
  </VerticalStackLayout>
</ContentPage>

MainPage.cs

using System;
using System.Collections.ObjectModel;
using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Maui;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;

namespace Maui.Controls.Sample
{
	public partial class MainPage : ContentPage
	{
		private int i = 0;

		public MainPage()
		{
			InitializeComponent();
		}

		private void Button_Clicked(object sender, EventArgs e)
		{
			i++;
			if (i % 2 == 1)
			{
				myLabel.TextDecorations = TextDecorations.Underline | TextDecorations.Strikethrough;
			}
			else
			{
				myLabel.TextDecorations = TextDecorations.None;
			}
		}
    }
}

Used to include an UITest.

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Collaborator Author

MartyIX commented May 28, 2024

Thank you

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon added the area-controls-label Label, Span label May 28, 2024
Copy link
Member

@Foda Foda left a comment

Choose a reason for hiding this comment

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

Great contribution! Thank you!

@rmarinho rmarinho merged commit 6370c04 into dotnet:main May 28, 2024
49 checks passed
@MartyIX MartyIX deleted the feature/2024-05-26-Windows-TextDecorations branch May 28, 2024 21:19
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-label Label, Span community ✨ Community Contribution fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10 platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants