Ship fix/codex-review-p1-batch - #129
Conversation
1. measure_text uses SkShaper when shaping is enabled (#125 review) - Aligned with fill_text: both now use HarfBuzz for consistent kerning/ligature widths. Fixes draw-vs-measure mismatch for strings like "AV", "ffi". 2. SDF text: UTF-8 bounds checking + missing glyph fallback (#126 review) - Guard multibyte decode against truncated input (no UB on malformed) - Fall back to fill_text() if any glyph is missing from the atlas rather than silently skipping characters 3. Remove duplicate accessibility JNI from gpu_surface_android.cpp (#127 review) - Old code had wrong role mappings (toggle=3, label=2, meter=1) - New code in core/view/platform/android/accessibility_android.cpp uses C++ enum values directly (correct) - Eliminates linker duplicate symbol conflict on Android 4. Add MIDI include path for Android audio target (#128 review) - core/midi/include added to pulp-audio's include directories in PulpAndroid.cmake so oboe_device.cpp can find buffer.hpp and android_midi_fifo.hpp
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9ae560d6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // REMOVED: the accessibility JNI exports have moved to | ||
| // core/view/platform/android/accessibility_android.cpp which uses the | ||
| // correct C++ enum values directly (no role mapping table). The old | ||
| // code here had wrong role mappings that caused TalkBack to announce |
There was a problem hiding this comment.
Removing the Java_com_pulp_accessibility_* exports from gpu_surface_android.cpp leaves no compiled implementation for PulpAccessibilityDelegate because the replacement file is not wired into any build target: core/view/platform/android/accessibility_android.cpp is not listed in core/view/CMakeLists.txt (Android block at lines 276-281 only adds stubs), and tools/cmake/PulpAndroid.cmake only adds choreographer_android.cpp/gpu_surface_android.cpp to pulp-render (lines 133-141). On Android builds this will surface as UnsatisfiedLinkError when TalkBack triggers the native accessibility calls.
Useful? React with 👍 / 👎.
…clude 1. Wire accessibility_android.cpp into pulp-view Android target (PulpAndroid.cmake) — the JNI exports existed but were never compiled into the Android build (#129 review) 2. Fix Windows SDF atlas include: use SkTypeface_win.h (contains SkFontMgr_New_DirectWrite) instead of SkFontMgr_directory.h (#131 review) PR #130 P1 (WindowHost API) and P2 (use_gpu) were already fixed in PR #131.
…clude (#132) 1. Wire accessibility_android.cpp into pulp-view Android target (PulpAndroid.cmake) — the JNI exports existed but were never compiled into the Android build (#129 review) 2. Fix Windows SDF atlas include: use SkTypeface_win.h (contains SkFontMgr_New_DirectWrite) instead of SkFontMgr_directory.h (#131 review) PR #130 P1 (WindowHost API) and P2 (use_gpu) were already fixed in PR #131.
1 participant
Automated by Shipyard