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 |
 |
|
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 |
03d7a34 to
79e4789
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe change adds Unicode 17.0.0 supplementary bidi data and a generator for that data. It also adds C++ functions that classify supplementary code points and detect paragraph direction in UTF-16 text. ChangesUnicode bidirectional classification
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Text
participant Get_Paragraph_Level
participant Get_Supplementary_Direction
participant supplementarybidi.inl
participant GetStringTypeW
Text->>Get_Paragraph_Level: provide UTF-16 text
Get_Paragraph_Level->>Get_Supplementary_Direction: classify valid surrogate pairs
Get_Supplementary_Direction->>supplementarybidi.inl: search supplementary ranges
Get_Paragraph_Level->>GetStringTypeW: classify non-surrogate BMP characters
Get_Paragraph_Level-->>Text: return paragraph level
Suggested reviewers: Merge Risk: 🔵 Low · up to Direction detection may assign an empty paragraph the direction of text in the next paragraph. This is a bounded issue, but paragraph separation should be clarified or fixed before the helper is used for rendering. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new direction helper is not connected to a rendering path in this PR, so no new active security exposure was identified. Its input and generated-data contracts matter when a later change begins using it. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e349eecd-f1d5-4445-9ec2-2dcf71941e61
📒 Files selected for processing (5)
Core/Libraries/Source/WWVegas/WW3D2/CMakeLists.txtCore/Libraries/Source/WWVegas/WW3D2/supplementarybidi.inlCore/Libraries/Source/WWVegas/WW3D2/unicode-license.txtCore/Libraries/Source/WWVegas/WW3D2/unicodebidi.hscripts/generate_supplementary_bidi.py
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
79e4789 to
1e735f5
Compare
1e735f5 to
5802fb2
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
OmarAglan
changed the title
this is a helper, and is needed for #3292, well if use complex text path it uses Uniscribe to shape normally authored Arabic or Hebrew and mixed direction ui text this is mainly used befor itemization, the itemization needs the paragraph initial bidi level (we use 0 for LTR or 1 for RTL), this donot implement the full bidi algorithm, this is more simpler approach in figuring out initial bidi level, i dont want to make a whole bidi algorithm, (about 5000 code of line to get it fully setup +_+). |
5802fb2 to
51b90cf
Compare
| #pragma once | ||
|
|
||
| #ifndef WIN32_LEAN_AND_MEAN | ||
| #define WIN32_LEAN_AND_MEAN |
| #ifndef WIN32_LEAN_AND_MEAN | ||
| #define WIN32_LEAN_AND_MEAN | ||
| #endif | ||
| #include <windows.h> |
There was a problem hiding this comment.
the algorithm it self do not require Windows, as you see the current implementation uses GetStringTypeW for bmp classification and Windows types in the API
i suppose i can replace that classification with generated Unicode data too and use portable types, That would remove the need for windows.h and duplicate macro mentioned in #3393 and the windows only CMake condition once and for all
i can do that if agreed upon!
There was a problem hiding this comment.
The wide char type currently is wchar_t. Becomes unichar after #3321
There was a problem hiding this comment.
The wide char type currently is
wchar_t. Becomesunicharafter #3321
so what i need to do is what exactly? cant figure it out!
do i make the helper portable using the project’s current wchar_t type, and remove the Windows include and macro?!
There was a problem hiding this comment.
The wide char type currently is
wchar_t. Becomesunicharafter #3321
i have these changes localy, if we agree upon i will push it:
- now Windows dependencies is removed so there is no windows.h or WCHAR or WORD or GetStringTypeW or WIN32_LEAN_AND_MEAN
- i used wchar_t
- made the classification portable and now we have generated Unicode 17 table that now covers both bmp and supplementary characters
- now CMake build the library on every platform
note that the algorithm still finds the first strong direction in the first paragraph and skips isolate contents and handles surrogate pairs
the shaping and visual reordering remain the renderer responsibility
there is a tradeoff here (as a maintainer maybe we need to consider the larger generated table in exchange for consistent classification across operating systems)
bmp results now follow Unicode 17 rather than the host Windows version, making build on multiple platforms as possible
There was a problem hiding this comment.
Yes right now we just use wchar_t
bmp results now follow Unicode 17 rather than the host Windows version
What does this mean in practice? Any complications?
There was a problem hiding this comment.
in practice, paragraph base direction would use the same Unicode data on every platform.
i did compared it with GetStringTypeW locally, i did find (for example) that Windows reports U+061C (arabic letter mark) as having no direction (why???!), while Unicode 17 classifies it as strong RTL, and a paragraph starting with that mark followed by Latin text would get RTL base direction.
also U+0600 (arabic number sign) is weak under Unicode 17, so it would no longer determine paragraph direction by itself
i assume That the main compatibility concern is that an older Uniscribe implementation may use different classifications internally, that helper only supplies the initial paragraph level, it does not add Unicode 17 shaping or font support
the full rendering still needs to be tested and to be validated in the pr #3292.
this table is also larger and requires regeneration for future Unicode updates (when needed of course), i did some local direction checks VC6 game builds and they passed
There was a problem hiding this comment.
There was a problem hiding this comment.
So if I understand this correctly the presented solution here will not work reliably, is that right? If so, is there an alternative superior implementation that works reliably?
I still do not understand what the goal of this change is, because the explanations have not been clear to me.
There was a problem hiding this comment.
sorry if my explanation is unclear, I'm having a hard time putting my words Clearly
the goal here is to choose the initial paragraph direction before Uniscribe itemization in #3292, lets take for example, 123 مرحبا should start with RTL paragraph level because the numbers are weak characters and Arabic supplies the first strong character
Hello مرحبا should start with LTR level. This supplies the starting direction for mixed direction UI text.
the helper simply implements the Unicode 17 first strong rule using the same data on every platform, i did some classification checks and paragraph regression cases and they pass.
Usp10 still performs the remaining bidirectional analysis, shaping and rendering.
the changes localy is pushed, now Windows include and macro and API types are removed
the API uses wchar_t and and CMake builds the helper on every platform (i only tested linux and windows).
51b90cf to
6d0f511
Compare
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
I do not understand what is going on unicodebidi.cpp but it looks like c++ code so good enough.
I would like to wait for @OmniBlade review before merging.
cool with me, take your time, and thanks for your effort |
4 participants
Split and replace #3231.
Adds a portable helper that determines a paragraph initial direction from its first strong character, following UAX #9 rules P2/P3.
The helper supplies the initial paragraph level for Uniscribe itemization in #3292, which #3293 integrates into single-line UI text. For example, a paragraph beginning with Arabic or Hebrew selects RTL, while one beginning with Latin selects LTR. Leading numbers and punctuation do not determine the direction.
The helper:
Both BMP and supplementary characters use generated Unicode 17.0.0 data. This removes the Windows API dependency and enables building the helper on every platform. The generator validates the source version and checksum; the Unicode license is retained alongside the generated table.
Usp10 remains required by the renderer in #3292 for bidirectional analysis, shaping, measurement, and visual ordering. This helper only supplies the initial paragraph level.
Validation
Implementation and local verification developed with AI assistance.