Conversation
PR Summary by QodoAdd Unicode-aware paragraph direction detection
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record |
|
| Filename | Overview |
|---|---|
| Core/Libraries/Source/WWVegas/WW3D2/unicodebidi.h | Adds the UTF-16 first-strong direction algorithm; behavior is coherent, but control-body formatting and automated coverage need attention. |
| Core/Libraries/Source/WWVegas/WW3D2/supplementarybidi.inl | Adds sorted generated Unicode supplementary bidi ranges, with repository comment-date and license-prologue violations. |
| scripts/generate_supplementary_bidi.py | Adds a checksum-pinned reproducible table generator but omits the required GPL source prologue and does not preserve its validation cases as tests. |
| Core/Libraries/Source/WWVegas/WW3D2/CMakeLists.txt | Registers the new header and generated include data with the shared WW3D2 interface source set. |
| Core/Libraries/Source/WWVegas/WW3D2/unicode-license.txt | Adds the Unicode data license associated with the generated classification table. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[UTF-16 paragraph] --> B[Read next code unit]
B --> C{Isolate control?}
C -->|Initiator| D[Increase isolate depth]
C -->|PDI| E[Decrease positive depth]
C -->|No| F{Inside isolate?}
D --> B
E --> B
F -->|Yes| B
F -->|No| G{Valid surrogate pair?}
G -->|Yes| H[Decode supplementary code point]
H --> I[Binary-search Unicode bidi ranges]
G -->|No, valid BMP| J[Call GetStringTypeW]
G -->|Malformed surrogate| B
I --> K{Strong direction?}
J --> K
K -->|RTL| L[Return level 1]
K -->|LTR| M[Return level 0]
K -->|Neutral| B
B -->|End of text| M
Prompt To Fix All With AI
### Issue 1
scripts/generate_supplementary_bidi.py:1
**Missing GPL Prologues**
The new generator and generated `.inl` file begin without GPL license headers. This violates the repository requirement that newly created source files include GPL headers and must be corrected before merging. The same issue appears in `Core/Libraries/Source/WWVegas/WW3D2/supplementarybidi.inl`.
### Issue 2
Core/Libraries/Source/WWVegas/WW3D2/supplementarybidi.inl:3
**Outdated Comment Date**
This newly added generated-code comment says `Copyright 2025 Unicode, Inc.`, and the generator emits the same text. That violates the repository requirement that new comments not reference dates before the current year, 2026, and must be corrected before merging.
### Issue 3
Core/Libraries/Source/WWVegas/WW3D2/unicodebidi.h:60
**Inline Control Bodies**
Several new `if` bodies are placed on the same line as their conditions, including the isolate-depth decrement, neutral-content skip, direction assignments, and final return. This violates the repository requirement to place control-statement bodies on separate lines for precise debugger breakpoints and must be corrected before merging.
### Issue 4
Core/Libraries/Source/WWVegas/WW3D2/unicodebidi.h:50-79
**Direction Logic Untested**
The new direction detector has no committed automated tests despite adding separate behavior for nested and unterminated isolates, malformed surrogate pairs, supplementary RTL ranges, and Windows BMP classification. Without preserving the stated local cases in the repository, later changes to this traversal or its generated table can silently regress paragraph direction.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(text): Add Unicode paragraph direct..." | Re-trigger Greptile
|
Why do we need the unicode license and copyright headers? |
|
|
||
| Direction direction = Neutral; | ||
| if (ch >= 0xD800 && ch <= 0xDBFF) { | ||
| if (index + 1 < length && text[index + 1] >= 0xDC00 && text[index + 1] <= 0xDFFF) { |
There was a problem hiding this comment.
Do you understand everything that's going on here or is it left to reviewers to figure out?
There was a problem hiding this comment.
well, the goal here is specifically paragraph base direction detection according to the first strong character, rather than implementing the complete bidi algorithm, so the loop do this at the moment:
- it skips the contents of directional isolates for the paragraph direction decision, and safely decodes valid UTF-16 surrogate pairs
- it ignores malformed surrogate sequences rather than treating them as directional characters, and uses
GetStringTypeW(CT_CTYPE2)for BMP characters
it then uses the generated Unicode bidi class ranges for supplementary code points because GetStringTypeW does not reliably classify those, and returns level 0 or 1 when the first strong L or R or AL character is found.
my reason is this generated table is intentionally limited to what this helper needs rather than trying to duplicate make the whole the complete Unicode bidi algorithm (more code more complix, and it will take a lot of time)
well, i suppose we can improve the loop or i can add more comments if needed.
hmm this is the supplementary bidi table and it is generated from Unicode's if you read the license of unicode v3 , you would see that it requires the copyright and permission notice to be included with any copies of the data software or it can be included in associated documentation also the short copyright and license header on the generated .inl is there to make the to be more clear and it points to the full license i suppose, if it will do no harm in anyway, i can remove it if needed |
2 participants
Adds a helper for finding a paragraph's first strong text direction. It decodes UTF-16 surrogate pairs and skips directional-isolate contents, allowing supplementary RTL characters to participate in paragraph-direction detection.
BMP classification uses Windows. Supplementary classification uses generated Unicode 17.0.0 data, with a reproducible generator, source checksum, and Unicode license. The helper is consumed by the separate complex-text rasterization change; this PR does not activate UI shaping.
Validation
git diff --check.Implementation developed with AI assistance.