Use Highlight matches from Everything #4196
Conversation
To solve inaccurate highlight when facing Chinese Pinyin searches
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
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. 📝 WalkthroughWalkthroughParses Everything API highlighted filename strings into zero-based highlight index lists; exposes them via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Consumer
participant ResultMgr as ResultManager
participant Everything as EverythingApi
participant Native as Everything DLL
Note over UI,ResultMgr: User initiates search
UI->>ResultMgr: SearchAsync(query)
ResultMgr->>Everything: Request results for query
Everything->>Native: Everything_GetResultHighlightedFileName(index)
Native-->>Everything: highlighted filename string (e.g. "Fi*le**Name*")
Everything->>Everything: EverythingHighlightStringToHighlightList(string)
Everything-->>ResultMgr: SearchResult (includes HighlightData list)
ResultMgr->>UI: Constructed result (TitleHighlightData from HighlightData or fuzzy fallback)
UI-->>UI: Render highlights
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (1)
7-9: Parameterless constructor may be redundant.Record structs automatically receive a compiler-generated parameterless constructor. The explicit empty constructor here is redundant but doesn't cause issues.
🔎 Optional: Remove redundant constructor
-public SearchResult() -{ -} - public string FullPath { get; init; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.csPlugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.csPlugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-24T19:06:48.344Z
Learnt from: Koisu-unavailable
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Flow.Launcher/ViewModel/MainViewModel.cs:0-0
Timestamp: 2025-06-24T19:06:48.344Z
Learning: In Flow.Launcher's Explorer plugin results, the SubTitle property always contains the directory containing the file. For file results, Title contains the filename and SubTitle contains the parent directory. For directory results, SubTitle contains the directory path itself.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
EverythingApiDllImport(8-161)
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
List(218-261)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
[warning] 218-218:
Hightlight is not a recognized word. (unrecognized-spelling)
⏰ 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)
- GitHub Check: build
🔇 Additional comments (6)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
160-161: LGTM - Good integration of Everything's highlight data.The code correctly retrieves both the run count score and highlighted filename from Everything's API, then converts the highlight string to a list of indices using the new helper method.
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (1)
16-16: LGTM - Clean addition of highlight data property.The
HighlightDataproperty is well-integrated with appropriate nullability and init-only accessor, consistent with the existing property pattern.Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs (3)
68-70: LGTM - Proper propagation of highlight data.The code correctly passes
HighlightDatafrom the search result to both folder and file result creation methods, enabling Everything's highlights to flow through to the UI.
96-104: LGTM - Excellent fallback mechanism for highlights.The implementation correctly uses Everything's highlight data when available (
HighlightData) and gracefully falls back to the existing fuzzy search highlighting when not. This maintains backward compatibility and ensures highlights work in all scenarios.
286-306: LGTM - Consistent highlight handling for file results.The file result creation mirrors the folder result logic, using Everything's highlights when available with a fallback to fuzzy search. This consistency ensures a uniform user experience across result types.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
150-150: P/Invoke signature is correctly implemented.The
stringreturn type withCharSet.Unicodeis the proper .NET marshaling approach for native Unicode string pointers. The consumer code inEverythingAPI.csexpects astringdirectly, confirming this implementation aligns with usage. Note: this is a new implementation (not a change fromIntPtr), and the design choice is consistent with similarly intentional use ofIntPtrforEverything_GetResultHighlightedPathin the same file.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
213-261: Parsing logic is correct.The implementation properly handles the Everything highlight format:
- Single
*toggles highlighting on/off- Consecutive
**represents a literal asterisk character- Null and empty strings are handled gracefully
The logic correctly tracks the actual string position (excluding markers) and builds the highlight index list.
Optional: Consider adding unit tests
Unit tests would help verify edge cases such as:
- Empty string, single
*, trailing*- Multiple consecutive
**- Unclosed highlight regions (odd number of
*)- Mixed patterns like
"a*b**c*d"Example test cases:
[Test] public void EverythingHighlightStringToHighlightList_BasicHighlight() { var result = EverythingApi.EverythingHighlightStringToHighlightList("abc*123*def"); CollectionAssert.AreEqual(new[] { 3, 4, 5 }, result); } [Test] public void EverythingHighlightStringToHighlightList_LiteralAsterisk() { var result = EverythingApi.EverythingHighlightStringToHighlightList("a*b**c*d"); CollectionAssert.AreEqual(new[] { 1, 2, 3 }, result); // "b*c" is highlighted } [Test] public void EverythingHighlightStringToHighlightList_EmptyString() { var result = EverythingApi.EverythingHighlightStringToHighlightList(""); Assert.IsEmpty(result); }Note: If this method is only used internally within
EverythingApi, consider making itprivateorinternalinstead ofpublic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
EverythingApiDllImport(8-161)Flow.Launcher/Storage/QueryHistory.cs (1)
Add(36-65)
⏰ 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 (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
160-161: LGTM! Highlight data integration is correct.The integration properly populates
HighlightDatafrom Everything's highlighted filename API, with the parsing delegated to the new helper method. The uint cast forEverything_GetResultRunCountis safe sinceidxis a non-negative loop counter.
There was a problem hiding this comment.
Pull request overview
This pull request addresses inaccurate highlighting in Chinese Pinyin searches by using highlight matches directly from the Everything search API instead of relying on the internal string matcher. The Everything API provides more accurate highlighting information for non-ASCII characters.
Key changes:
- Added
HighlightDataproperty toSearchResultto store highlight indices from Everything - Modified result creation methods to accept and use highlight data when available, falling back to the existing fuzzy search
- Implemented a parser to convert Everything's highlight string format (using asterisks as delimiters) into a list of character indices
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs | Added HighlightData property to store highlight indices and an explicit parameterless constructor |
| Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs | Updated CreateFolderResult and CreateFileResult to accept optional HighlightData parameter and use it when available |
| Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs | Changed return type of Everything_GetResultHighlightedFileName from IntPtr to string for easier interop |
| Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs | Added EverythingHighlightStringToHighlightList method to parse Everything's highlight format and integrated it into the search results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
🥷 Code experts: Jack251970 Jack251970, jjw24 have 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:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Co-authored-by: Copilot <[email protected]>
This reverts commit 7e9dc18.
Changed SearchResult to a readonly record struct for better immutability and performance. Updated HighlightData to initialize as an empty list using a collection expression instead of null. Retained the default constructor with a clarifying comment.
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
LGTM! Need jjw24 to do tests with Everything
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
218-261: Parsing logic is correct, but test coverage is strongly recommended.The implementation correctly handles the Everything highlight format:
- Single
*toggles highlighting on/off**represents a literal asterisk character- Tracks character indices in the unescaped string
However, this parsing logic is complex enough to warrant comprehensive unit tests covering edge cases such as:
- Empty or null strings
- Strings without any highlights
- Single and multiple highlighted sections
- Strings containing literal asterisks (
**)- Trailing or leading
*characters- Malformed input
Given the "Chill" review mode, while tests are strongly recommended for this logic, they can be added in a follow-up if time-constrained. However, please prioritize test coverage before the next release.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (1)
7-7: Fix typo in comment.The comment contains a typo: "necesssary" should be "necessary" (three 's' instead of two).
🔎 Proposed fix
- // Constructor is necesssary for record struct + // Constructor is necessary for record struct
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.csPlugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-24T19:06:48.344Z
Learnt from: Koisu-unavailable
Repo: Flow-Launcher/Flow.Launcher PR: 3770
File: Flow.Launcher/ViewModel/MainViewModel.cs:0-0
Timestamp: 2025-06-24T19:06:48.344Z
Learning: In Flow.Launcher's Explorer plugin results, the SubTitle property always contains the directory containing the file. For file results, Title contains the filename and SubTitle contains the parent directory. For directory results, SubTitle contains the directory path itself.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs (2)
Flow.Launcher/ViewModel/MainViewModel.cs (4)
Result(1768-1809)Result(1811-1839)Query(1201-1229)List(1321-1381)Plugins/Flow.Launcher.Plugin.Explorer/Search/QuickAccessLinks/QuickAccess.cs (2)
List(11-28)List(30-40)
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (2)
Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs (1)
ResultType(193-217)Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
List(218-261)
⏰ 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)
- GitHub Check: build
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs (1)
2-2: LGTM! Clean integration of highlight data with appropriate fallback.The addition of optional
highlightDataparameters to bothCreateFolderResultandCreateFileResultmaintains backward compatibility while enabling Everything's highlight data to flow through to results. The fallback to fuzzy search when highlight data is unavailable ensures existing functionality remains intact.Also applies to: 68-68, 70-70, 96-96, 104-104, 286-286, 306-306
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchResult.cs (1)
5-10: LGTM! Appropriate use of readonly record struct with explicit constructor.The changes correctly implement:
- The
readonlymodifier ensures struct immutability.- The explicit parameterless constructor is necessary for readonly record structs with field initializers to execute those initializers.
- The collection expression
[]forHighlightDatais a clean C# 12+ feature.Also applies to: 17-17
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
160-161: LGTM! Proper integration of Everything's highlight data.The changes correctly:
- Use
Convert.ToInt32for explicit conversion of the run count.- Populate
HighlightDataby parsing Everything's highlighted filename format.
|
@VictoriousRaptor can you show me a screenshot of the inaccurate highlighting please. |
I can't give a screenshot now because I don't remember a specific query that has in accurate highlight. I'll post it if I have one. Sometimes it happens when I'm using double pinyin or with pinyin acronym. Not sure if it's only with pinyin, most of my english named documents works good. The last question should be discussed. |
To solve inaccurate highlight when facing Chinese Pinyin searches.
Our string matcher doesn't match the same highlight as Everything so sometimes the highlight looks weird.