Fix incorrect text highlighting when using pinyin queries #4151
Conversation
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the pinyin query text highlighting feature where characters were incorrectly highlighted when translated indices matched exact boundaries in the translation mapping. The issue occurred when a matched index in the translated pinyin string corresponded to a boundary position between original Chinese characters.
Key changes:
- Fixed the
MapToOriginalIndexmethod to correctly handle exact boundary matches by adding+ 1to the result whenBinarySearchfinds an exact match
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdjusts translated→original index mapping for exact matches, refines pinyin tokenization/spacing around Chinese vs non‑Chinese boundaries, and expands unit tests to assert finer-grained translated→original index mappings for mixed Chinese/non‑Chinese input. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)Flow.Launcher.Test/TranslationMappingTest.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
26-35: Consider adding a test case for the space at index 9.The test cases cover a good range of scenarios, but notably skip index 9, which should be the space between "Yong" and "Hu". Adding
[TestCase(9, 5)]or[TestCase(9, 6)](depending on expected behavior) would provide more complete coverage of the space insertion logic that was modified in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs(5 hunks)Flow.Launcher.Test/TranslationMappingTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
Flow.Launcher.Infrastructure/TranslationMapping.cs (2)
TranslationMapping(6-33)AddNewIndex(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
39-50: Excellent test documentation and setup.The comments clearly illustrate the character-by-character translation mapping, making the test logic easy to follow. The per-character AddNewIndex calls for ASCII characters and multi-character mappings for Chinese characters correctly reflect the updated translation behavior.
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (4)
132-135: Correct fix to prevent double spaces.The additional check
content[i - 1] != ' 'prevents inserting a space before a Chinese character when the previous character is already a space, avoiding unnecessary double spacing in the translation.
147-150: Good symmetry in space handling logic.The conditional space insertion after Chinese characters mirrors the logic before Chinese characters (line 132), ensuring consistent handling of existing spaces and preventing double spacing throughout the translation.
153-154: Key fix for the highlighting bug.This change is critical to resolving issue #3965:
- Using fixed length
1for non-Chinese characters ensures correct 1:1 index mapping- Appending
content[i]directly (instead ofresultList[i]) is more efficient and guarantees the original character is preserved- Together, these changes prevent the "omitted last character" issue in mixed pinyin/non-pinyin queries by maintaining accurate translated-to-original index mappings
191-193: Minor formatting improvement.The reformatted ternary expression is slightly more readable, though functionally unchanged. The logic correctly returns the double pinyin translation if available, otherwise falls back to the full pinyin.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
🥷 Code experts: no user but you matched threshold 10 VictoriousRaptor has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
42-43: Consider clarifying the position markers.The format
"012345678901"uses single digits where the last "01" represents positions 10 and 11, which could be momentarily confusing. While technically correct, a two-row format might be clearer:// Test case : // 0123456 // Flow 用户 -// 012345678901 +// 0 1 +// 01234567890 1 // Flow Yong HuOr simply add spacing to align with the string:
-// 012345678901 -// Flow Yong Hu +// 0 5 10 +// Flow Yong Hu
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/TranslationMappingTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
Flow.Launcher.Infrastructure/TranslationMapping.cs (2)
TranslationMapping(6-33)AddNewIndex(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher.Test/TranslationMappingTest.cs (2)
12-23: LGTM!The test correctly verifies that
AddNewIndexstores the cumulative boundary (translatedIndex + length) for each original index.
56-68: LGTM!The helper methods appropriately use reflection to access private fields for white-box testing. This is a standard and acceptable pattern for verifying internal state in unit tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/TranslationMappingTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
Flow.Launcher.Infrastructure/TranslationMapping.cs (2)
TranslationMapping(6-33)AddNewIndex(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
26-37: Per‑index mapping tests look consistent and completeThe per‑character [TestCase] set for translated indices 0–11 matches the
TranslationMapping.MapToOriginalIndexbehavior (including the boundary at index 9 and the final'u'at index 11) and now fully exercises the mixed Chinese/non‑Chinese example relevant to the pinyin highlighting bug.
Fixes #3965