Skip to content

Conversation

@ramezgerges
Copy link
Contributor

GitHub Issue: closes https://github.com/unoplatform/kahua-private/issues/398

PR Type:

What is the current behavior? 🤔

What is the new behavior? 🚀

PR Checklist ✅

Please check if your PR fulfills the following requirements:

Other information ℹ️

Copilot AI review requested due to automatic review settings January 18, 2026 14:24
@github-actions github-actions bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Jan 18, 2026
Copy link
Contributor

Copilot AI left a 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 adds spellchecking functionality to the TextBox control for Skia-based platforms. It implements spell checking using the WeCantSpell.Hunspell library with an embedded US English dictionary, provides visual feedback via red wavy underlines, and integrates spell check suggestions into the context menu.

Changes:

  • Adds Hunspell-based spell checking to UnicodeText rendering with embedded US English dictionary
  • Integrates spell check suggestions into TextBox context menu
  • Provides FeatureConfiguration API for custom dictionaries
  • Adds localized resource strings for spell check UI across 100+ languages

Reviewed changes

Copilot reviewed 95 out of 96 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
src/Uno.UI/Uno.UI.Skia.csproj Adds WeCantSpell.Hunspell package reference and embeds dictionary files
src/Uno.UI/UI/Xaml/Documents/UnicodeText.skia.cs Core spell checking implementation with word detection, dictionary lookup, visual rendering, and suggestion generation
src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.skia.cs Wires IsSpellCheckEnabled property to DisplayBlock
src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.skia.cs Initializes spell check state on DisplayBlock
src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.pointers.skia.cs Adds spell check suggestions to context menu
src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs Passes spell check flag to UnicodeText constructor
src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.cs Optimizes selection change handling
src/Uno.UI/FeatureConfiguration.cs Adds CustomSpellCheckDictionaries configuration API
src/Uno.UI/Resources/en_US.aff Hunspell affix rules for English
src/Uno.UI/Resources/en_US.dic Embedded US English dictionary (~49K words)
src/Uno.UI/UI/Xaml/Controls/WinUIResources/* Adds "Spelling" localized strings for 100+ languages

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22383/wasm-skia-net9/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22383/docs/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 191690 has failed on Uno.UI - CI.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22383/docs/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22383/wasm-skia-net9/index.html

Copilot AI review requested due to automatic review settings January 22, 2026 20:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 95 out of 96 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1461 to +1491
private static List<WordList> GetWordLists()
{
var wordLists = new List<WordList>();
var assembly = Assembly.GetAssembly(typeof(UnicodeText))!;
var enAff = assembly.GetManifestResourceNames().First(r => r.Contains("en_US.aff"));
var enDic = assembly.GetManifestResourceNames().First(r => r.Contains("en_US.dic"));
wordLists.Add(WordList.CreateFromStreams(assembly.GetManifestResourceStream(enDic), assembly.GetManifestResourceStream(enAff)));

if (FeatureConfiguration.TextBox.CustomSpellCheckDictionaries is { } dictionaries)
{
foreach (var (dic, aff) in dictionaries)
{
if (aff != null)
{
try
{
wordLists.Add(WordList.CreateFromStreams(dic, aff));
}
catch (Exception e)
{
if (typeof(UnicodeText).Log().IsEnabled(LogLevel.Error))
{
typeof(UnicodeText).Log().Error($"Failed to load dictionary", e);
}
}
}
}
}

return wordLists;
}
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The GetWordLists method loads and parses dictionary files each time it's called, which is an expensive operation. Since _wordLists is a static field shared across all UnicodeText instances, this creates a potential memory issue where dictionaries are loaded multiple times unnecessarily if _wordLists is set to null or garbage collected. Additionally, the embedded en_US.dic file has 49,570 lines which will consume significant memory. Consider implementing proper singleton pattern or caching to ensure dictionaries are only loaded once per application lifetime.

Copilot uses AI. Check for mistakes.
Comment on lines +1471 to +1477
foreach (var (dic, aff) in dictionaries)
{
if (aff != null)
{
try
{
wordLists.Add(WordList.CreateFromStreams(dic, aff));
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The CustomSpellCheckDictionaries property allows streams to be provided but there's no check that 'dic' is not null before passing it to CreateFromStreams. Only 'aff' is checked for null. This asymmetry could lead to a NullReferenceException if a user provides a null dictionary stream. Either check both for null or document that 'dic' must not be null.

Copilot uses AI. Check for mistakes.
Comment on lines +974 to +975
var correctionClusterStart = _textIndexToGlyph[Math.Max(wordStart + correction.Value.correctionStart, runStartIndex)];
var correctionClusterEnd = _textIndexToGlyph[Math.Min(wordStart + correction.Value.correctionEnd, runEndIndex) - 1]; // -1 because the end is exclusive for boundaries but inclusive for glyph map
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The spell-check drawing logic accesses _textIndexToGlyph array at indices calculated from word boundaries and corrections, but there's no bounds checking. If wordStart + correction.Value.correctionStart or wordStart + correction.Value.correctionEnd exceed the text length or fall outside valid indices, this will cause an IndexOutOfRangeException. Add bounds validation before array access.

Copilot uses AI. Check for mistakes.
var assembly = Assembly.GetAssembly(typeof(UnicodeText))!;
var enAff = assembly.GetManifestResourceNames().First(r => r.Contains("en_US.aff"));
var enDic = assembly.GetManifestResourceNames().First(r => r.Contains("en_US.dic"));
wordLists.Add(WordList.CreateFromStreams(assembly.GetManifestResourceStream(enDic), assembly.GetManifestResourceStream(enAff)));
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The GetManifestResourceStream method can return null if the resource is not found, but the code passes the result directly to WordList.CreateFromStreams without null checking. This could cause a NullReferenceException. Add null checks for both stream retrievals before calling CreateFromStreams.

Copilot uses AI. Check for mistakes.
Comment on lines +415 to +418
var text = Text;
var newText = text[..correctionStart] + suggestion + text[correctionEnd..];
Text = newText;
Select(correctionStart + suggestion.Length, 0);
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The ReplaceWithSuggestion method doesn't validate that correctionStart and correctionEnd are within bounds of the text. If the text has changed between when the context menu was opened and when the suggestion is clicked, this could cause an ArgumentOutOfRangeException. Add bounds validation before attempting to slice the string.

Suggested change
var text = Text;
var newText = text[..correctionStart] + suggestion + text[correctionEnd..];
Text = newText;
Select(correctionStart + suggestion.Length, 0);
var text = Text ?? string.Empty;
// Ensure correction range is valid for the current text to avoid ArgumentOutOfRangeException
if (correctionStart < 0 ||
correctionEnd < correctionStart ||
correctionStart > text.Length ||
correctionEnd > text.Length)
{
return;
}
suggestion ??= string.Empty;
var newText = text[..correctionStart] + suggestion + text[correctionEnd..];
Text = newText;
var caretIndex = Math.Min(correctionStart + suggestion.Length, newText.Length);
Select(caretIndex, 0);

Copilot uses AI. Check for mistakes.
Comment on lines +1425 to +1459
private static List<(int correctionStart, int correctionEnd)?> GetSpellCheckSuggestions(List<int> wordBoundaries, string text)
{
var ret = new List<(int correctionStart, int correctionEnd)?>();
var start = 0;
foreach (var end in wordBoundaries)
{
var word = text.Substring(start, end - start);
var startTrimmedWord = word.TrimStart();
var trimmedWord = startTrimmedWord.TrimEnd();

_wordLists ??= GetWordLists();

if (trimmedWord.Length > 0 && !trimmedWord.Any(c => char.IsPunctuation(c) || char.IsNumber(c) || char.IsSeparator(c) || char.IsWhiteSpace(c) || char.IsSymbol(c)))
{
if (_wordLists.Any(wordList => wordList.Check(trimmedWord)))
{
ret.Add(null);
}
else
{
var correctionStart = word.Length - startTrimmedWord.Length;
var correctionEnd = word.Length - startTrimmedWord.Length + trimmedWord.Length;
ret.Add((correctionStart, correctionEnd));
}
}
else
{
ret.Add(null);
}

start = end;
}

return ret;
}
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The GetSpellCheckSuggestions method is called during text layout for every word in the text, which could be a performance bottleneck for large text content. Each word requires dictionary lookups via wordList.Check() which may be expensive. Consider adding a cache for frequently-checked words or deferring spell checking to an async background operation, especially for long documents.

Copilot uses AI. Check for mistakes.
@unodevops
Copy link
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22383/wasm-skia-net9/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22383/docs/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 192632 has failed on Uno.UI - CI.

@ramezgerges ramezgerges mentioned this pull request Jan 23, 2026
@unodevops
Copy link
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22383/wasm-skia-net9/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22383/docs/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 192852 has failed on Uno.UI - CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/skia ✏️ Categorizes an issue or PR as relevant to Skia

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants