-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Introduce TextOptions #20107
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
base: master
Are you sure you want to change the base?
Introduce TextOptions #20107
Conversation
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.
Pull Request Overview
This pull request introduces TextOptions as a new dedicated type for configuring text rendering, separating text-specific settings from the general RenderOptions. This change improves API design by providing a clearer separation of concerns between general rendering options and text-specific rendering configuration.
Key Changes
- Introduces
TextOptionsstruct withTextRenderingMode,TextHintingMode, andBaselinePixelAlignproperties - Moves
TextRenderingModefromRenderOptionstoTextOptions - Implements a new
TwoLevelCachefor efficient text blob caching based on complete text options - Updates all drawing context implementations to support text options push/pop operations
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/Avalonia.Base/Media/TextOptions.cs |
New struct defining text rendering options with methods for getting/setting options on visuals |
src/Avalonia.Base/Media/TextHintingMode.cs |
New enum defining text hinting modes (None, Slight, Normal, Full) |
src/Avalonia.Base/Media/TextRenderingMode.cs |
Enhanced with XML documentation for each enum value |
src/Avalonia.Base/Media/RenderOptions.cs |
Removed TextRenderingMode property and related methods |
src/Avalonia.Base/Visual.cs |
Added TextOptions property alongside existing RenderOptions |
src/Avalonia.Base/Platform/IDrawingContextImpl.cs |
Added PushTextOptions and PopTextOptions methods to interface |
src/Skia/Avalonia.Skia/TwoLevelCache.cs |
New cache implementation for storing text blobs with eviction support |
src/Skia/Avalonia.Skia/GlyphRunImpl.cs |
Updated to use TextOptions for text blob caching instead of simple edging array |
src/Skia/Avalonia.Skia/DrawingContextImpl.cs |
Implemented text options stack and applies hinting/baseline settings |
src/Windows/Avalonia.Direct2D1/Media/DrawingContextImpl.cs |
Added text options support with separate text antialiasing control |
src/Headless/Avalonia.Headless/HeadlessPlatformRenderInterface.cs |
Added no-op text options methods for headless rendering |
src/Avalonia.Base/Rendering/ImmediateRenderer.cs |
Updated to push text options before render options |
src/Avalonia.Base/Rendering/Composition/Server/ServerCompositionVisual.cs |
Added text options push/pop in rendering pipeline |
src/Avalonia.Base/Rendering/Composition/Server/DrawingContextProxy.cs |
Added text options proxy commands |
tests/Avalonia.Skia.UnitTests/Media/GlyphRunTests.cs |
Updated to use new TextOptions API |
tests/Avalonia.RenderTests/Media/GlyphRunTests.cs |
Migrated from RenderOptions.SetTextRenderingMode to TextOptions.SetTextRenderingMode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Avalonia.Base/Rendering/Composition/Server/ServerCompositionVisual.cs
Show resolved
Hide resolved
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
Co-authored-by: Copilot <[email protected]>
…nto feature/TextOptions
|
You can test this PR using the following package version. |
|
Notes from the API review meeting:
|
|
Also, make sure documentation states that the various backends/platforms may behave a bit differently here. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
| @@ -0,0 +1,10 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
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.
This file has likely been committed by mistake, please remove.
| /// <remarks>Use this enumeration to control whether the baseline of rendered content is aligned to the | ||
| /// pixel grid, which can affect visual crispness and positioning. The value may influence rendering quality, | ||
| /// especially at small font sizes or when precise alignment is required.</remarks> | ||
| public enum BaselinePixelAlignment |
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.
I know we've been pretty inconsistent with the usual "one type per file", but other used enums in this file are in their own files. Please move it to its own.
| /// <remarks>Use this enumeration to control whether the baseline of rendered content is aligned to the | ||
| /// pixel grid, which can affect visual crispness and positioning. The value may influence rendering quality, | ||
| /// especially at small font sizes or when precise alignment is required.</remarks> | ||
| public enum BaselinePixelAlignment |
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.
Other enums used in RenderOptions and TextOptions specify a byte underlying type for tight packing of the render data. We should probably do the same here for consistency?
| } | ||
|
|
||
| // If primary matches after factory (unlikely) return primary and dispose created | ||
| if (_comparer.Equals(_primaryKey!, key)) |
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.
Unless I'm mistaken, this isn't unlikely (as noted by the comment above), but completely impossible? _primaryKey and key didn't change and the primary key comparison was already handled by TryGet at the beginning of the method.
| } | ||
|
|
||
| // Rotate into secondary (evict last) | ||
| var newSec = new KeyValuePair<TKey, TValue>[sec.Length]; |
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.
This should be rotated in place, there is no need to allocate a new array each time.
| /// thread-safe.</remarks> | ||
| /// <typeparam name="TKey">The type of keys used to identify cached values. Must be non-nullable.</typeparam> | ||
| /// <typeparam name="TValue">The type of values to be stored in the cache. Must be a reference type.</typeparam> | ||
| internal class TwoLevelCache<TKey, TValue> |
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.
I'm aware this is an internal helper, but a couple of unit tests testing basic features (primary key, secondary cache, eviction) might be nice. This is typically the type of helper that has just enough complexity to contain hidden bugs ;)
(For example, we had several bugs in the past with InlineDictionary that manifested only in some specific scenarios.)
| [InlineData(TextRenderingMode.Alias, TextHintingMode.None)] | ||
| [InlineData(TextRenderingMode.Antialias, TextHintingMode.Light)] | ||
| [InlineData(TextRenderingMode.Alias, TextHintingMode.Light)] | ||
| [Theory(Skip = "Backend dependent")] |
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.
Can we at least execute the tests on Windows?
What does the pull request do?
This PR introduces TextOptions similar to the current RenderOptions. This allows customization of certain font rasterization properties, such as font hinting, antialiasing, or baseline pixel alignment.
What is the current behavior?
Currently, it is only possible to customize the text antialiasing.
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Default Skia mapping
Freetype
DWrite
CoreText
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Fixes: #12510
Fixes: #19553