Double pinyin query #2427
Double pinyin query #2427
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
VictoriousRaptor
force-pushed
the
double-pin
branch
from
73c4a69 to
22f6ad7
Compare
|
@VictoriousRaptor what's the status of this pr? |
POC. I'm using it daily and so far so good. Since there're many double pinyin mappings we should make it user configurable. Is it acceptable to allow users to edit a json file that defines the mapping? |
|
well I can't think anything better...maybe I do want this to be plugin-able, but can't think of a good way to design the api. |
VictoriousRaptor
force-pushed
the
double-pin
branch
from
0c01c95 to
f673000
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for a double pinyin query by introducing a new double_pinyin.json resource and integrating this functionality into the settings and pinyin translation mechanisms.
- Adds a new JSON resource for double pinyin mappings.
- Updates the project file to include the new resource.
- Extends Settings, TranslationMapping, StringMatcher, and PinyinAlphabet to support double pinyin translation.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/Resources/double_pinyin.json | New resource file containing pinyin mappings. |
| Flow.Launcher/Flow.Launcher.csproj | Updated to copy the double_pinyin.json resource to output. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Added new settings (UseDoublePinyin and DoublePinyinSchema). |
| Flow.Launcher.Infrastructure/TranslationMapping.cs | No functional changes; existing mapping logic is maintained. |
| Flow.Launcher.Infrastructure/StringMatcher.cs | Adjustments to pinyin translation checks and method modifiers. |
| Flow.Launcher.Infrastructure/PinyinAlphabet.cs | Major modifications to load double pinyin data and update pinyin translation logic. |
| Flow.Launcher.Infrastructure/IAlphabet.cs | Minor interface update reflecting translation API changes. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
What's you guys opinion about putting a json in resources and let |
📝 Walkthrough""" WalkthroughA new interface for alphabet translation was introduced, along with a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StringMatcher
participant PinyinAlphabet
participant Settings
participant double_pinyin.json
User->>StringMatcher: Input query
StringMatcher->>PinyinAlphabet: ShouldTranslate(query)
PinyinAlphabet->>Settings: Check UseDoublePinyin & Schema
alt Double pinyin enabled
PinyinAlphabet->>double_pinyin.json: Load schema mapping
end
StringMatcher->>PinyinAlphabet: Translate(query)
PinyinAlphabet->>TranslationMapping: Map indices (if translated)
StringMatcher->>User: Return translated string and mapping
Poem
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
Flow.Launcher.Infrastructure/StringMatcher.cs (1)
229-240: Digit-detection bug inside the static acronym helpers
IsAcronymNumbernow lives in a static context, but its body still compares thecharagainst integer literals 0–9:stringToCompare[idx] >= 0 && stringToCompare[idx] <= 9This never evaluates as intended (the code points 0–9 are control characters).
Replace withchar.IsDigitor'0'/'9'bounds:-private static bool IsAcronymNumber(string s, int i) - => s[i] >= 0 && s[i] <= 9; +private static bool IsAcronymNumber(string s, int i) + => char.IsDigit(s[i]);
🧹 Nitpick comments (5)
Flow.Launcher/Resources/double_pinyin.json (1)
1-1: Please pretty-format or chunk this JSON for maintainability.A single-line 100 k+ character blob is almost impossible to review, diff, or merge.
Pretty-printing (or splitting each scheme into its own file) will ease future edits without affecting run-time behavior.Flow.Launcher.Infrastructure/TranslationMapping.cs (2)
53-86: Binary search degrades to O(N) due to inside loops.Inside the search you compute
indexDefwith aforloop every time the translated index falls between ranges, turning worst-case lookup into O(N).
Pre-compute cumulative deltas once during construction instead, or store(translatedStart, originalStart, delta)tuples to keepMapToOriginalIndexstrictly O(log N) without extra loops.
89-94: Follow .NET naming conventions.Rename
endConstruct()➜EndConstruct()to match PascalCase guidelines for public methods.Flow.Launcher.Infrastructure/PinyinAlphabet.cs (2)
29-37: Potential memory leak due to everlasting Settings subscription
PinyinAlphabetsubscribes to_settings.PropertyChangedwith an inline lambda but never unsubscribes.
If multiple instances are ever created (unit tests, DI scopes, plugin reloads) this will accumulate handlers and keep old objects alive.Consider:
_settings.PropertyChanged += SettingsChanged; ... private void SettingsChanged(object? sender, PropertyChangedEventArgs e) { ... }and detach in
Dispose()/ finalizer or ensure the class is registered as a singleton.
57-75: File-based table load – hard-coded path & sync I/O
- Using
AppContext.BaseDirectory+"Resources"assumes the resources directory is next to the exe; that breaks for single-file publish or relocatingResources.- Synchronous
File.OpenReadon the UI thread can block startup.Recommendation:
-var tablePath = Path.Join(AppContext.BaseDirectory, "Resources", "double_pinyin.json"); +var tablePath = Path.Combine(AppContext.BaseDirectory, "Resources", "double_pinyin.json"); // keeps interop +// optionally: allow overriding via settings and load asynchronouslyEven better: embed the JSON as an embedded resource and read via
GetManifestResourceStream, eliminating path issues entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Infrastructure/IAlphabet.cs(1 hunks)Flow.Launcher.Infrastructure/PinyinAlphabet.cs(1 hunks)Flow.Launcher.Infrastructure/StringMatcher.cs(3 hunks)Flow.Launcher.Infrastructure/TranslationMapping.cs(1 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Flow.Launcher.csproj(1 hunks)Flow.Launcher/Resources/double_pinyin.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/Flow.Launcher.csproj (1)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
🧬 Code Graph Analysis (1)
Flow.Launcher.Infrastructure/StringMatcher.cs (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
ShouldTranslate(77-82)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Infrastructure/StringMatcher.cs
[warning] 32-32: Ioc is not a recognized word. (unrecognized-spelling)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs
[warning] 28-28: Ioc is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (4)
Flow.Launcher/Flow.Launcher.csproj (1)
130-132: Resource inclusion looks correct.The JSON is treated as
NonewithPreserveNewest, consistent with other resources. No action needed.Flow.Launcher.Infrastructure/IAlphabet.cs (1)
6-21: Looks good – clear contract.The interface cleanly separates eligibility (
ShouldTranslate) from translation (Translate).Flow.Launcher.Infrastructure/StringMatcher.cs (1)
70-76: Translation guard now hinges onquery– confirm semanticsSwitching from
!CanBeTranslated(query)toShouldTranslate(query)makes translation depend on the query itself.
This is correct iff:
ShouldTranslatereliably returnstruefor “Latin/double-pinyin input” and never for mixed or Chinese queries.- All callers respect the same logic (i.e., no other place still uses the obsolete
CanBeTranslated).Please double-check the new predicate against edge-cases (e.g., odd-length double-pinyin strings, mixed latin/chinese queries) to avoid silently skipping pinyin translation.
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
46-53:JsonSerializer.Deserialize(Stream, …)requires .NET 6+If Flow Launcher still targets .NET 5 or earlier, this overload won’t compile.
Ensure the target framework isnet6.0or higher, or switch to reading the stream into a string and deserialising from that.
There was a problem hiding this comment.
@VictoriousRaptor please fix the suggestions from coderabbitai
Docstrings generation was requested by @VictoriousRaptor. * #2427 (comment) The following files were modified: * `Flow.Launcher.Infrastructure/IAlphabet.cs` * `Flow.Launcher.Infrastructure/PinyinAlphabet.cs` * `Flow.Launcher.Infrastructure/StringMatcher.cs` * `Flow.Launcher.Infrastructure/TranslationMapping.cs`
|
Note Generated docstrings for this pull request at #3751 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
108-110: Fix the incorrect length parameter in index mapping.The length parameter in
AddNewIndexshould betranslated.Length, nottranslated.Length + 1. The space is formatting and shouldn't be included in the content mapping.- map.AddNewIndex(i, resultBuilder.Length, translated.Length + 1); + map.AddNewIndex(i, resultBuilder.Length, translated.Length);The space has already been appended (line 108), so
resultBuilder.Lengthcorrectly points to the start of the translated text, but the length should only cover the translated content, not the formatting space.
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
45-53: Consider adding JsonSerializerOptions for consistency and security.While the deserialization logic is correct, specifying JsonSerializerOptions can improve security and ensure consistent behavior across the application.
- Dictionary<string, Dictionary<string, string>> table = JsonSerializer.Deserialize<Dictionary<string, Dictionary<string, string>>>(jsonStream); + var options = new JsonSerializerOptions { PropertyNameCaseInsensitive = true }; + Dictionary<string, Dictionary<string, string>> table = JsonSerializer.Deserialize<Dictionary<string, Dictionary<string, string>>>(jsonStream, options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs(1 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Flow.Launcher.Infrastructure/PinyinAlphabet.cs
[warning] 26-26:
Ioc is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (10)
4-11: LGTM! Well-chosen imports for the new functionality.The new imports appropriately support JSON deserialization, dependency injection, file I/O, and logging requirements.
15-15: Good refactoring to implement IAlphabet interface.This improves abstraction and makes the class more testable and maintainable.
17-22: Well-designed fields for thread safety and immutability.The ConcurrentDictionary provides thread-safe caching, and ReadOnlyDictionary ensures the pinyin table cannot be modified after loading.
24-37: Excellent use of dependency injection and reactive configuration.The constructor properly initializes dependencies and sets up automatic reloading when pinyin settings change, ensuring the service stays synchronized with user preferences.
55-75: Robust error handling with graceful degradation.The file loading logic properly handles errors and provides a sensible fallback (empty dictionary) to maintain application functionality when the resource file is unavailable.
77-80: Simple and correct implementation.The method properly checks for Chinese characters without the problematic even-length validation mentioned in past reviews.
82-90: Well-structured caching and early return optimization.The method efficiently handles caching and avoids unnecessary processing for non-Chinese content.
116-116: Good fix for the previousIsChinese flag maintenance.The flag is now properly updated, addressing the issue from previous reviews where spacing logic wasn't working correctly.
Also applies to: 123-123
139-146: Clean and robust double pinyin conversion.The method handles all cases gracefully through dictionary lookup, including single-letter pinyin that was mentioned in past reviews.
1-151: Excellent refactoring with well-integrated double pinyin support.The file demonstrates good software engineering practices:
- Clean interface implementation
- Thread-safe caching
- Proper dependency injection
- Robust error handling
- User-configurable double pinyin schemas
The overall architecture supports the PR objectives well by making double pinyin mappings user-configurable through JSON files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
45-75: Consider improving error handling for better user experience.The JSON loading logic is well-structured, but the silent fallback to an empty dictionary when schema loading fails might confuse users who expect double pinyin to work.
Consider logging a more specific warning or providing user feedback when the schema is invalid:
catch (System.Exception e) { - Log.Exception(nameof(PinyinAlphabet), "Failed to load double pinyin table from file: " + tablePath, e); + Log.Exception(nameof(PinyinAlphabet), $"Failed to load double pinyin table from file: {tablePath}. Schema '{_settings.DoublePinyinSchema}' may be invalid.", e); currentDoublePinyinTable = new ReadOnlyDictionary<string, string>(new Dictionary<string, string>()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Flow.Launcher.Infrastructure/PinyinAlphabet.cs
[warning] 26-26:
Ioc is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (7)
4-11: LGTM! Appropriate imports for the new functionality.The new imports support JSON deserialization, dependency injection, and file I/O operations needed for the double pinyin feature.
17-37: LGTM! Well-designed field declarations and constructor.The use of
ConcurrentDictionaryfor thread-safe caching andReadOnlyDictionaryfor the immutable lookup table is appropriate. The property change handler ensures dynamic reloading when settings change.
77-80: LGTM! Simplified implementation resolves previous odd-length rejection issue.The method now correctly focuses on whether the string contains Chinese characters rather than imposing length restrictions, which improves the user experience for double pinyin queries.
82-90: LGTM! Efficient implementation with proper caching.The method correctly implements the
IAlphabetinterface contract and uses the cache effectively with early returns for non-Chinese content.
92-135: LGTM! Translation logic correctly addresses previous issues.The implementation properly handles:
- Index mapping without off-by-one errors (space is appended before adding the mapping)
- The
previousIsChineseflag is now correctly updated (line 116 and 123)- Efficient string building with
StringBuilder- Proper integration with double pinyin conversion
The translation mapping logic appears to be correctly implemented now.
139-146: LGTM! Simple and correct double pinyin conversion.The method correctly looks up mappings in the table and falls back to the original pinyin when no mapping exists, which is the expected behavior.
26-26: Static analysis false positive - Ioc is correct.
Iocis the correct class name fromCommunityToolkit.Mvvm.DependencyInjectionand represents "Inversion of Control," a well-established design pattern.
jjw24
added
the
review in progress
- Always add index to mapping rather than only Chinese characters - Simplify mapping algorithm - Add unit test for TranslationMapping
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
93-137: Verify index mapping correctness for consecutive Chinese characters.Based on past review comments, there may be an off-by-one error in the index mapping when
previousIsChineseis true. TheAddNewIndexcall appears to happen before appending the space, which could cause incorrect mapping.Let me verify the current implementation against the past review concern:
#!/bin/bash # Verify the index mapping logic in BuildCacheFromContent method echo "Checking index mapping logic..." # Extract the relevant method to analyze the flow ast-grep --pattern $'if (previousIsChinese) { $$$ map.AddNewIndex($$$); $$$ }'
🧹 Nitpick comments (3)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
42-54: Consider exposing internal state through proper testing interface.While the reflection-based approach works, consider adding internal test accessors or making the
originalToTranslatedlist internal withInternalsVisibleToattribute for cleaner test code.// In TranslationMapping.cs, add: +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Flow.Launcher.Test")] // Change the field to internal: -private readonly List<int> originalToTranslated = []; +internal readonly List<int> originalToTranslated = [];This would eliminate the need for reflection in tests and improve maintainability.
Flow.Launcher.Infrastructure/TranslationMapping.cs (1)
11-11: Fix typo in comment.Minor spelling error in the comment.
- // Asssuming one original item maps to multi translated items + // Assuming one original item maps to multi translated itemsFlow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
45-53: Consider more specific exception types for better error handling.The method uses
InvalidOperationExceptionfor configuration issues, but a more specific exception type would help callers distinguish between different failure modes.- throw new InvalidOperationException("DoublePinyinSchema is invalid or double pinyin table is broken."); + throw new ArgumentException($"DoublePinyinSchema '{_settings.DoublePinyinSchema}' is not found in the double pinyin table.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs(1 hunks)Flow.Launcher.Infrastructure/TranslationMapping.cs(1 hunks)Flow.Launcher.Test/TranslationMappingTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher.Infrastructure/TranslationMapping.cs (1)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3561
File: Flow.Launcher/ViewModel/SelectBrowserViewModel.cs:53-58
Timestamp: 2025-05-21T10:37:07.696Z
Learning: When implementing collection item removal operations in Flow Launcher, always handle index boundaries carefully. After removing an item from a collection, ensure the updated index remains within valid bounds (>= 0 and < collection.Count) to prevent IndexOutOfRangeExceptions, especially when decrementing indexes.
🧬 Code Graph Analysis (2)
Flow.Launcher.Test/TranslationMappingTest.cs (1)
Flow.Launcher.Infrastructure/TranslationMapping.cs (3)
TranslationMapping(7-36)AddNewIndex(15-21)MapToOriginalIndex(23-28)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (2)
Flow.Launcher.Infrastructure/TranslationMapping.cs (3)
TranslationMapping(7-36)AddNewIndex(15-21)endConstruct(30-35)Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-552)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/TranslationMappingTest.cs
[warning] 3-3:
NUnit is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (12)
Flow.Launcher.Test/TranslationMappingTest.cs (3)
1-9: LGTM! Proper test file structure.The test file is well-structured with appropriate namespacing and test framework setup.
10-21: Test logic is correct and validates core functionality.The test properly verifies that
AddNewIndexstores cumulative boundary values (translatedIndex + length) in the internal list. The expected values of 8 and 10 match the calculation logic.
23-40: Excellent parameterized test coverage.The test cases comprehensively cover edge cases including:
- Boundary values (0, 2, 3, 5, 6)
- The mapping logic from the example "a测试" → "a Ce Shi"
- Expected behavior of binary search with bitwise complement for insertion points
The Chinese comment provides helpful context for understanding the test scenario.
Flow.Launcher.Infrastructure/TranslationMapping.cs (2)
15-21: LGTM! Proper construction state validation.The method correctly validates the construction state and stores cumulative boundary values. The logic aligns with the binary search requirements in
MapToOriginalIndex.
30-35: LGTM! Proper finalization with state validation.The method correctly prevents multiple calls and marks the mapping as immutable. The exception handling ensures proper usage patterns.
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (7)
17-18: LGTM! Proper concurrent caching implementation.The concurrent dictionary correctly handles multi-threaded access for caching translations with their mappings.
24-37: Excellent dynamic configuration handling.The constructor properly initializes settings, loads the double pinyin table, and sets up reactive updates when relevant settings change. This ensures the feature responds immediately to user configuration changes.
39-43: LGTM! Clean reload mechanism.The reload method correctly updates the double pinyin table and clears the cache to ensure consistency after settings changes.
55-75: Robust error handling with fallback mechanism.The method properly handles file loading errors and provides a safe fallback (empty dictionary) when the double pinyin table cannot be loaded. The logging ensures debugging information is available.
77-81: LGTM! Inverted logic aligns with new interface contract.The method correctly returns
truewhen the string should be translated (i.e., doesn't contain Chinese characters). This matches the expected behavior for theIAlphabetinterface.
83-91: LGTM! Efficient caching with proper early returns.The method correctly handles the caching logic with appropriate early returns when translation is not needed or when cached results are available.
139-150: LGTM! Clean double pinyin conversion with proper fallback.The
ToDoublePinmethod correctly looks up the full pinyin in the double pinyin table and returns the original if no mapping exists. The region organization improves code readability.
| public int MapToOriginalIndex(int translatedIndex) | ||
| { | ||
| int loc = originalToTranslated.BinarySearch(translatedIndex); | ||
|
|
||
| return loc > 0 ? loc : ~loc; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Binary search logic is correct but check edge case.
The binary search implementation correctly handles both exact matches and insertion points using bitwise complement. However, verify the boundary condition when loc == 0 (exact match at first position).
🏁 Script executed:
#!/bin/bash
# Verify binary search behavior for edge cases
echo "Testing binary search edge cases..."
# Create a simple C# test to verify BinarySearch behavior
cat > test_binary_search.cs << 'EOF'
using System;
using System.Collections.Generic;
class Program
{
static void Main()
{
var list = new List<int> { 5, 10, 15 };
// Test cases for BinarySearch behavior
Console.WriteLine($"Search 5: {list.BinarySearch(5)}"); // Should be 0
Console.WriteLine($"Search 3: {list.BinarySearch(3)}"); // Should be -1
Console.WriteLine($"Search 7: {list.BinarySearch(7)}"); // Should be -2
Console.WriteLine($"Search 12: {list.BinarySearch(12)}"); // Should be -3
Console.WriteLine($"Search 20: {list.BinarySearch(20)}"); // Should be -4
// Test the condition loc > 0 ? loc : ~loc
foreach (int searchValue in new[] { 5, 3, 7, 12, 20 })
{
int loc = list.BinarySearch(searchValue);
int result = loc > 0 ? loc : ~loc;
Console.WriteLine($"Value {searchValue}: loc={loc}, result={result}");
}
}
}
EOF
# Compile and run if dotnet is available
if command -v dotnet &> /dev/null; then
dotnet new console --force --name BinarySearchTest --output /tmp/test
cp test_binary_search.cs /tmp/test/Program.cs
cd /tmp/test && dotnet run
else
echo "dotnet not available for testing"
fiLength of output: 181
Fix boundary condition in MapToOriginalIndex
The current check loc > 0 treats a match at index 0 as “not found,” returning ~0 (−1) instead of 0. Change it to loc >= 0 so exact matches at the first position are handled correctly.
Affected location:
- Flow.Launcher.Infrastructure/TranslationMapping.cs (around lines 23–28)
Suggested diff:
- public int MapToOriginalIndex(int translatedIndex)
- {
- int loc = originalToTranslated.BinarySearch(translatedIndex);
-
- return loc > 0 ? loc : ~loc;
- }
+ public int MapToOriginalIndex(int translatedIndex)
+ {
+ int loc = originalToTranslated.BinarySearch(translatedIndex);
+ return loc >= 0 ? loc : ~loc;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int MapToOriginalIndex(int translatedIndex) | |
| { | |
| int loc = originalToTranslated.BinarySearch(translatedIndex); | |
| return loc > 0 ? loc : ~loc; | |
| } | |
| public int MapToOriginalIndex(int translatedIndex) | |
| { | |
| int loc = originalToTranslated.BinarySearch(translatedIndex); | |
| return loc >= 0 ? loc : ~loc; | |
| } |
🤖 Prompt for AI Agents
In Flow.Launcher.Infrastructure/TranslationMapping.cs around lines 23 to 28, the
condition checking the result of BinarySearch uses loc > 0, which incorrectly
treats a found index at 0 as not found. Change the condition to loc >= 0 to
correctly handle matches at the first position and return the correct index.
No description provided.