CmdPal: Enhance font icon classification and visuals#41573
Conversation
- Introduce `FontIconGlyphClassifier` for classifying emojis and symbols. - Update `IconPathConverter` to utilize the classifier for better handling. - Add `SampleIconPage` with new test cases, including symbols with both text and emoji representations. - Simplify `IconBox` padding logic by removing classification and expanding padding for font icons by default. - Add alignment properties and improve icon source management in `IconBox`. - Support Unicode Variation Selectors VS15 (force text) and VS16 (force emoji). - Update default emoji classification to exclude Extended Pictographic characters.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
michaeljolley
added
the
Product-Command Palette
There was a problem hiding this comment.
Pull Request Overview
This PR enhances font icon classification and visual handling in the Command Palette by introducing sophisticated emoji and symbol detection capabilities. The changes improve how icons are categorized, displayed, and aligned across the UI.
Key changes:
- Introduces
FontIconGlyphClassifierfor accurate classification of emojis, symbols, and text characters using ICU Unicode boundary detection - Updates
IconPathConverterto use the new classifier for better font family selection and invalid icon handling - Adds comprehensive icon demonstration page with various Unicode examples
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
SamplesListPage.cs |
Adds navigation to new sample icon demonstration page |
SampleIconPage.cs |
New comprehensive demo page showcasing various icon types and Unicode classifications |
Microsoft.Terminal.UI.vcxproj |
Project file updates to include new FontIconGlyphClassifier components |
IconPathConverter.cpp |
Updated to use new classifier for improved icon handling and font selection |
FontIconGlyphClassifier.idl |
IDL interface definition for the new glyph classification system |
FontIconGlyphClassifier.h |
Header file for the classifier implementation |
FontIconGlyphClassifier.cpp |
Core implementation of Unicode-based glyph classification using ICU |
IconBox.cs |
Enhanced icon alignment and padding calculations with error handling |
expect.txt |
Spell check additions for new Unicode-related terms |
| #include "IconPathConverter.g.cpp" | ||
|
|
||
| // #include "Utils.h" | ||
| #include "FontIconGlyphClassifier.h" |
There was a problem hiding this comment.
[nitpick] The commented line // #include \"Utils.h\" should be removed rather than left as a comment if it's no longer needed.
| // Switching back to EnqueueAsync has broken icons in tags (they don't show) | ||
| // _ = @this._queue.EnqueueAsync(() => | ||
| @this._queue.TryEnqueue(new(async () => | ||
| @this._queue.TryEnqueue(async void () => |
There was a problem hiding this comment.
Using async void is generally discouraged except for event handlers as it makes error handling and testing difficult. Consider using async Task and handling the task appropriately, or use a fire-and-forget pattern with proper error handling.
| @this._queue.TryEnqueue(async void () => | |
| @this._queue.TryEnqueue(async () => |
|
|
||
| if (iconData is not null && | ||
| @this.Source is FontIconSource) | ||
| @this.Padding = new Thickness(Math.Round(iconSize * -0.2)); |
There was a problem hiding this comment.
The magic number 0.2 should be extracted to a named constant to improve code maintainability and make the scaling factor's purpose clear.
Summary of the Pull Request
Introduces
FontIconGlyphClassifierfor classifying emojis and symbols.\u25B6):Updates
IconPathConverterto use the new classifier for improved icon handling.Adds
SampleIconPageto demonstrate various icon usages and classifications.Adjusts icon alignment in
IconBoxso icons are centered.Scales negative padding for emojis in
IconBoxwith control size, fixing misalignment and clipping (noticeable in tags and the details pane hero image).Applies negative padding to all font icons. This removes the need for classification in these cases and ensures symbols rendered below the baseline remain visible.
Based on microsoft/terminal#19143:
Co-authored-by: Dustin L. Howett [email protected]
Pictures? Pictures!
Keyboard and flag/country emojis may look a bit off, but that’s how they’re actually rendered:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed