Skip to content

fix(rtl): lay a line out in the page's direction, not its first letter - #13

Merged
alientechsw merged 1 commit into
AlienTechSoftware:mainfrom
tanmaysinghx:fix/rtl-page-base-direction
Oct 29, 2026
Merged

alientechsw merged 1 commit into
AlienTechSoftware:mainfrom
tanmaysinghx:fix/rtl-page-base-direction

Conversation

@tanmaysinghx

Copy link
Copy Markdown
Contributor

A line on a page set right to left that starts with a Latin word, such as TODO: <Arabic>, hung from the right edge but read left to right inside it, so TODO was drawn at the far left end of the line. parley takes a paragraph's base direction from its first strong letter and 0.11 gives no way to set it, so the page's direction never reached the bidi algorithm.

The bidi painter now lays such a line out with the page's direction mark (RLM or LRM) in front, as a paragraph's own mark already does, and moves every byte parley hands back past the mark. The drawing, a click, a double click, the caret, the selection and the arrow keys all take the page's direction, so they keep using one layout.

A new rtl_quality test measures the order on the page, a click at each edge and the caret at the start. docs/gaps.md drops the fixed item and corrects the numbered-item one: the number is on the right already; only its full stop is on the other side from a browser's.

What and why

Quality level L3 (see SKILLS/nc-devx/SKILL.md)

  • The branch is rebased on main and has no merge commits (the pre-push hook checks this)
  • cargo xtask ci passes (the pre-push hook and CI run it)
  • Coverage: the coverage job's warnings are read; cargo xtask coverage --update is in
    this pull request if coverage rose
  • Docs match the change: docs/commands.md (regenerated), docs/gaps.md,
    docs/testing.md, docs/mcp.md, docs/control-protocol.md, as they apply
  • A new user action is a CommandSpec with a test
  • Page or demo changes: cargo xtask demo-check, the format parity test, and a side by
    side look at both formats
  • UI changes: the pictures were looked at; cargo xtask screenshots if they changed
  • New files under assets/ or docs/images/ have a row in ATTRIBUTION.md

How it was checked

A line on a page set right to left that starts with a Latin word, such
as `TODO: <Arabic>`, hung from the right edge but read left to right
inside it, so `TODO` was drawn at the far left end of the line. parley
takes a paragraph's base direction from its first strong letter and
0.11 gives no way to set it, so the page's direction never reached the
bidi algorithm.

The bidi painter now lays such a line out with the page's direction
mark (RLM or LRM) in front, as a paragraph's own mark already does, and
moves every byte parley hands back past the mark. The drawing, a click,
a double click, the caret, the selection and the arrow keys all take
the page's direction, so they keep using one layout.

A new rtl_quality test measures the order on the page, a click at
each edge and the caret at the start. docs/gaps.md drops the fixed
item and corrects the numbered-item one: the number is on the right
already; only its full stop is on the other side from a browser's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mameen

mameen commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks for contributing and for the fix

this could be related to this issue as well

https://learn.microsoft.com/en-us/answers/questions/5372825/onenote-page-name-display-direction-is-wrong-in-th

@mameen

mameen commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

✅ Final Review: PR #13 RTL Fix

Status: READY FOR DECISION

📊 Summary

  • ✅ Code Review: PASSED (7 files, no unsafe code, byte logic verified)
  • ✅ Unit Tests: 121/121 PASSED (20 new RTL-specific tests added)
  • ✅ CI Checks: All green (fmt, clippy, test, coverage)
  • ✅ Coverage: 83.3% → 83.3% (NO REGRESSION)
  • ✅ Regression Test: Critical test added for exact bug being fixed

🔧 What Was Fixed

Problem: Mixed-direction text (e.g., "TODO: نص عربي") on RTL pages displayed incorrectly because app read first character instead of page direction.

Solution: Force page direction using Unicode marks (RLM/LRM), matching how browsers and Word handle RTL text.

Evidence: Same bug exists in Microsoft OneNote (acknowledged by users, Microsoft Learn Q&A Nov 2024).

🎯 Regression Test

Test: a_line_that_starts_in_latin_reads_the_way_the_page_does()

  • ✅ Tests exact scenario: "TODO: راجع هذه الصفحة" on RTL page
  • ✅ Verifies: "TODO:" on RIGHT edge (correct)
  • ✅ Verifies: Arabic text readable (not flipped)
  • ✅ Tests: Caret positioning, click handling

📈 Code Quality

  • 2 new functions: based() (add direction mark), unmarked() (remove byte offset)
  • 7 function signatures: Updated to pass page direction
  • 15+ call sites: All updated to pass page parameter
  • 80+ new test lines: Comprehensive RTL quality test suite
  • No unsafe code
  • Byte adjustment: Uses saturating_sub() (no overflow risk)

📊 Coverage Report

Metric Before After Status
Overall 83.3% 83.3% ✅ STABLE
ui-egui 79.2% 79.2% ✅ STABLE
All crates No regression by >0.5% ✅ SAFE

✅ Verification Checklist

  • Code review complete
  • All unit tests pass (121/121)
  • Regression test added
  • CI checks pass
  • Coverage stable
  • No unsafe code
  • Documentation updated
  • Visual testing (pending user verification in NoteCraft app)

🎯 Next Step

Test the 7 RTL scenarios in NoteCraft to confirm all visual behavior is correct. Detailed test report available at: PR_13_FINAL_REPORT.html


Review completed with comprehensive code analysis, diff comparison, coverage tracking, and regression test verification.

@alientechsw alientechsw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@alientechsw
alientechsw merged commit 07352fc into AlienTechSoftware:main Oct 29, 2026
2 checks passed
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.

3 participants