support variation selectors, ZWJ sequences and surrogate pairs in length calculation#2082
Conversation
|
@microsoft-github-policy-service agree |
|
you have to look at the expected output in the terminal as also vs code or github tend to render it incorrectly! 
|
|
Sorry, but I have a bit difficulty understanding these changes. I appreciate the PR but all logic for Unicode calculation should go into the wcwidth library. Why use UnicodeCalculator on net6 and above and a complete different approach otherwise? |
|
the problem is the length counting in char by char. the easy fix would have been to fix the one-by-one counting to detect the "wide" and change the result but it would have been a single fix only. this approach now fixes more cases. |
|
The string overload in UnicodeCalculator does this already afaik. |
|
I appreciate it, but I would rather get the calculation correct in wcwidth to be honest since it's used by other projects as well (the reason for it existing). I'll take a look at this PR as soon as I can. |
…airs in length calculation ( fixes spectreconsole#1847 )
fabsenet
force-pushed
the
1847-unicode-widechar-table
branch
from
8ae634d to
fbeae03
Compare
| public static int GetCellLength(ReadOnlySpan<char> text) | ||
| public static int GetCellLength(string text) | ||
| { | ||
| #if !NETSTANDARD2_0 |
There was a problem hiding this comment.
this obviosly is now not fixed for .net standard 2.0. UnicodeCalculator.GetWidth(string) is not available for .net standard?
There was a problem hiding this comment.
You are completely correct. I look forward to drop netstandard2.0 at some point (around the 2.0 release of Spectre.Console). For now, I'm OK with having two different implementations (and potentially erroneous rendering on .NET Framework). What do you think?
|
i reworked it to use the UnicodeCalculator.getstring more, this makes it a bit easier now! |
|
Ok, I've looked through it, and I think I understand the changes now. Will take it for a spin after I've merged another emoji-related bug tonight. |
patriksvensson
left a comment
patriksvensson
left a comment
There was a problem hiding this comment.
I think this looks good. No tests are failing and it solves the issue. Some minor nit-pick thing that needs to be fixed though.
|
Hi i am not sure what to do right now. i am fine with the change of extra brackets but i do not see an accept button anywhere? do i need to put this change in my branch? i see no simple accept button anywhere? |
|
I already took care of it 🙂 |
|
Merged! Thank you for your contribution. Much appreciated! 👍 |
|
I'm guessing this update made live rendering much slower. 55.1: 55.2 and latest: |
|
i watched the small videos now 10+ times and i do not understand what you want to show here. I am fine with trusting you, that it feels slower, but the animation does not transport it. |
|
@tmakluf Can you open a new issue? Thanks! |
Updated [Spectre.Console](https://github.com/spectreconsole/spectre.console) from 0.54.0 to 0.55.2. <details> <summary>Release notes</summary> _Sourced from [Spectre.Console's releases](https://github.com/spectreconsole/spectre.console/releases)._ ## 0.55.2 ## What's Changed * Support variation selectors, ZWJ sequences and surrogate pairs in length calculation by @fabsenet in spectreconsole/spectre.console#2082 * Add default value to selection prompt and multiselection prompt by @AntekOlszewski in spectreconsole/spectre.console#2079 ## New Contributors * @fabsenet made their first contribution in spectreconsole/spectre.console#2082 **Full Changelog**: spectreconsole/spectre.console@0.55.1...0.55.2 ## 0.55.1 ## What's Changed * Add tests to verify public API by @patriksvensson in spectreconsole/spectre.console#2073 * use StringComparer.OrdinalIgnoreCase as default comparer for TextPrompt by @AntekOlszewski in spectreconsole/spectre.console#2077 * Fix markup link rendering regression by @patriksvensson in spectreconsole/spectre.console#2084 * Add VS16 suffix to non-presentation emojis by @patriksvensson in spectreconsole/spectre.console#2087 * Ensure rendered exceptions take up minimal space by @patriksvensson in spectreconsole/spectre.console#2089 * Fix link parsing to terminate properly by @zhuman in spectreconsole/spectre.console#2091 ## New Contributors * @zhuman made their first contribution in spectreconsole/spectre.console#2091 **Full Changelog**: spectreconsole/spectre.console@0.55.0...0.55.1 ## 0.55.0 This release brings new features, performance improvements, bug fixes, and some important architectural changes. > [!CAUTION] > There are breaking changes in this release, so make sure you review the release notes and try things out before upgrading in production. ## New Spectre.Console.Ansi Library One of the biggest changes in this release is the introduction of [Spectre.Console.Ansi](https://www.nuget.org/packages/spectre.console.ansi), a new standalone library for writing ANSI escape sequences to the terminal without taking a full dependency on `Spectre.Console`. This makes it easy to add ANSI support to lightweight tools and libraries where pulling in the full Spectre.Console package would be overkill. Spectre.Console itself now depends on this library internally. We've also added some nice convenience methods for the .NET Console class: ```csharp using Spectre.Console.Ansi; Console.Markup("[yellow]Hello[/] "); Console.MarkupLine("[blue]World[/]"); Console.Ansi(writer => writer .BeginLink("https://spectreconsole.net", linkId: 123) .Decoration(Decoration.Bold | Decoration.Italic) .Foreground(Color.Yellow) .Write("Spectre Console") .ResetStyle() .EndLink()); ``` ## Style Is Now a Struct `Style` has been converted from a class to a struct, and link/URL information has been extracted into a separate `Link` type. This improves allocation performance, especially in rendering-heavy scenarios, but is a breaking change for code that relies on reference semantics. ## Progress Improvements The `Progress` widget received a lot of love in this release. It now uses `TimeProvider` instead of the wall clock, making it significantly easier to write deterministic tests. `ProgressTask` has a new `Tag` property for attaching arbitrary metadata, and you can now override the global hide-when-completed behavior on individual tasks. Tasks can also be removed from the progress context entirely. Speed calculations have been improved with configurable max sampling age and ... (truncated) Commits viewable in [compare view](spectreconsole/spectre.console@0.54.0...0.55.2). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/19497) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
3 participants
Fixes #1847
AI
I am working with unicode, windows-1252 and codepages as part of my day job for quite some time. I used Claude Code here as a tool to write faster. I checked every little change myself.
Changes
src/Spectre.Console/Internal/Cell.csGetCellLength(string): NET6+ → delegates toUnicodeCalculator.GetWidth(string)which already handles grapheme clusters correctly viaEnumerateRunes();netstandard2.0 → iterates withStringInfo.GetTextElementEnumeratorGetCellLength(ReadOnlySpan<char>): now delegates to the string path instead of iterating char-by-charprivate GetClusterCellLength(string): FE0F → 2, ZWJ → 2, surrogate pair → code point width lookupsrc/Spectre.Console/Rendering/Segment.csSplit(): char-foreach replaced with StringInfo enumerator — prevents splits in the middle of a grapheme clusterTruncate(): same changeSplitSegment(): same changesrc/Spectre.Console.Tests/Unit/CellTests.cs(new)src/Spectre.Console.Tests/Unit/Widgets/Table/TableTests.csShould_Render_Table_With_Wide_Emoji_CorrectlyPlease upvote 👍 this pull request if you are interested in it.