Skip to content

Enhance table rendering and string handling for East Asian characters#12

Open
goweii wants to merge 4 commits into
onepub-dev:mainfrom
goweii:main
Open

Enhance table rendering and string handling for East Asian characters#12
goweii wants to merge 4 commits into
onepub-dev:mainfrom
goweii:main

Conversation

@goweii
Copy link
Copy Markdown

@goweii goweii commented Apr 5, 2026

No description provided.

goweii added 2 commits April 5, 2026 19:00
- Updated table.dart to use displayWidth for accurate column width calculation.
- Modified pubspec.yaml to clean up dependency version formatting.
- Added tests in string_test.dart for East Asian character display width and text wrapping.
- Introduced a new test in table_test.dart to validate table rendering with East Asian characters.
@bsutton
Copy link
Copy Markdown

bsutton commented May 29, 2026

Apologies for the late response.

Thanks for the contribution. I tested this locally and it is not merge-ready yet.

dart test fails on the PR head with 4 table formatting failures. The current main passes the same test suite locally, so this appears to be introduced by the PR.

The new East Asian tests are useful, but the width calculation change affects more than CJK text. Existing table output changes around characters like × and superscript digits, causing column spacing regressions.

Could you please update the implementation so the existing table formatting tests continue to pass, while keeping the new East Asian table/string behavior covered?

@goweii
Copy link
Copy Markdown
Author

goweii commented May 30, 2026

Thanks for the review and for pointing this out.

I’ve updated the width calculation so East Asian ambiguous-width characters are treated as narrow by default. This keeps characters such as × and superscript digits from widening the existing table output, while still preserving the new East Asian wide/fullwidth handling.

I verified the PR locally:

  • dart analyze passes with no errors
  • dart test passes: +42: All tests passed!

Please let me know if you’d like any further changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants