feat: separate search boxes for Frozen and Preview sections#97
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSearch UI moved from a custom IMGUI control to Unity's SearchField and into section-owned instances. Index-based match caching was replaced by collection-based count caching and fixed-arity match APIs. Added a GUID→path in-memory cache invalidated on project changes and a helper to draw hidden-count labels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as PreviewSection UI
participant Search as SearchBoxControl
participant Cache as GuidPathCache
participant AssetDB as AssetDatabase
UI->>Search: Draw(matchedCount?, totalCount?)
UI->>Search: CountMatches(items, predicate)
Note over Search,Cache: predicate resolves item GUID → path
Search->>Cache: GetPath(guid)
Cache->>AssetDB: GUIDToAssetPath(guid) if not cached
AssetDB-->>Cache: returns path
Cache-->>Search: cached path
Search-->>UI: returns match booleans / matched count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Editor/TextureCompressor/UI/Preview/PreviewSection.cs (1)
64-69:⚠️ Potential issue | 🟡 MinorInvalidate the search count cache here too.
GeneratePreview()already clears_searchBox's count cache on Lines 76-77, butInvalidateCache()only drops_previewDataand_guidPathCache. If a search was active,_searchBoxstill holds the old preview collection reference, which keeps invalidated data alive longer than necessary and can leak stale counts into the next reuse of this section.Suggested fix
public void InvalidateCache() { _previewData = null; _previewSettingsHash = 0; _guidPathCache.Clear(); + _searchBox.InvalidateCountCache(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/TextureCompressor/UI/Preview/PreviewSection.cs` around lines 64 - 69, InvalidateCache currently nulls _previewData, resets _previewSettingsHash and clears _guidPathCache but does not clear _searchBox's cached counts, leaving stale preview references alive; update InvalidateCache to also perform the same _searchBox count/cache clearing that GeneratePreview does (i.e. invoke the same method or logic used in GeneratePreview to reset _searchBox's count cache so the search box no longer holds references to the old preview collection).
🧹 Nitpick comments (1)
Editor/TextureCompressor/UI/Sections/FrozenTexturesSection.cs (1)
26-27: Recompute search state after drawing the search field.
isSearching/filteredCountare captured before_searchBox.Draw(...), so filtering and “no results/hidden count” can be one interaction behind while typing.Suggested patch
- bool isSearching = _searchBox.IsSearching; - int filteredCount = _searchBox.CountMatches(config.FrozenTextures, MatchesFrozenSearch); + bool isSearching = _searchBox.IsSearching; + int filteredCount = _searchBox.CountMatches(config.FrozenTextures, MatchesFrozenSearch); @@ // Search box for frozen textures _searchBox.Draw(filteredCount, frozenCount); + // SearchField updates text during Draw; refresh derived values for this frame. + isSearching = _searchBox.IsSearching; + filteredCount = _searchBox.CountMatches(config.FrozenTextures, MatchesFrozenSearch);Also applies to: 50-57, 77-79, 90-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/TextureCompressor/UI/Sections/FrozenTexturesSection.cs` around lines 26 - 27, The code captures _searchBox.IsSearching and _searchBox.CountMatches(config.FrozenTextures, MatchesFrozenSearch) before calling _searchBox.Draw(...), causing UI state (isSearching/filteredCount) to lag; fix by moving the reads of _searchBox.IsSearching and the CountMatches call to immediately after each corresponding _searchBox.Draw(...) invocation so the search state is recomputed using the latest input; apply this change for the occurrences around the blocks that use _searchBox.Draw, MatchesFrozenSearch and the captured isSearching/filteredCount (the occurrences noted at the other ranges as well).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Editor/Common/UI/Controls/SearchBoxControl.cs`:
- Around line 108-135: The CountMatches method's cache key omits the predicate,
so different predicates over the same collection can return a stale count;
modify the caching in CountMatches to include the predicate identity (e.g.,
store a reference to the passed Func<T,bool> in a new field like
_cachedPredicate or compute a stable predicate fingerprint and compare it along
with _cachedCountSearchText, _cachedCountUseFuzzy, _cachedSourceRef, and
_cachedSourceCount) and only reuse _cachedCount when the predicate matches;
update assignments to record the current predicate when recalculating; also add
a regression test exercising two different predicates over the same source to
ensure counts differ.
- Around line 57-97: The Draw method renders input and toggles (via
_searchField.OnGUI(SearchText) and the UseFuzzySearch toggle) but also displays
matchedCount/totalCount in the same IMGUI pass, causing a stale "Showing x of y"
because caller computes counts before Draw updates SearchText/UseFuzzySearch;
fix by removing the count label from Draw and exposing a separate API so callers
recompute after inputs are applied — either add a new method (e.g.,
DrawCounts(int matchedCount, int totalCount) or GetCountLabel()) or change Draw
to return a flag or out value indicating "inputs changed" so the caller can
recompute counts and then call the new DrawCounts to render up-to-date values;
reference Draw, SearchText, UseFuzzySearch, _searchField.OnGUI and ensure
callers use the new pattern to recalc counts after detecting changed == true.
In `@Editor/TextureCompressor/UI/Sections/FrozenTexturesSection.cs`:
- Around line 264-271: MatchesFrozenSearch currently uses an empty string when
assetPath resolution fails, so rows that display frozen.TextureGuid aren't
searchable; update the logic in MatchesFrozenSearch to use the GUID string as
the textureName fallback (e.g. frozen.TextureGuid.ToString()) when assetPath is
null/empty, and then call _searchBox.MatchesSearchAny(textureName, assetPath) so
the visible GUID text is included in search matching; locate this change in the
MatchesFrozenSearch method which references _guidPathCache.GetPath,
frozen.TextureGuid, and _searchBox.MatchesSearchAny.
---
Outside diff comments:
In `@Editor/TextureCompressor/UI/Preview/PreviewSection.cs`:
- Around line 64-69: InvalidateCache currently nulls _previewData, resets
_previewSettingsHash and clears _guidPathCache but does not clear _searchBox's
cached counts, leaving stale preview references alive; update InvalidateCache to
also perform the same _searchBox count/cache clearing that GeneratePreview does
(i.e. invoke the same method or logic used in GeneratePreview to reset
_searchBox's count cache so the search box no longer holds references to the old
preview collection).
---
Nitpick comments:
In `@Editor/TextureCompressor/UI/Sections/FrozenTexturesSection.cs`:
- Around line 26-27: The code captures _searchBox.IsSearching and
_searchBox.CountMatches(config.FrozenTextures, MatchesFrozenSearch) before
calling _searchBox.Draw(...), causing UI state (isSearching/filteredCount) to
lag; fix by moving the reads of _searchBox.IsSearching and the CountMatches call
to immediately after each corresponding _searchBox.Draw(...) invocation so the
search state is recomputed using the latest input; apply this change for the
occurrences around the blocks that use _searchBox.Draw, MatchesFrozenSearch and
the captured isSearching/filteredCount (the occurrences noted at the other
ranges as well).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d21771ab-0d98-45ea-81f3-a67480976fc6
📒 Files selected for processing (9)
Editor/Common/UI/Controls/SearchBoxControl.csEditor/Common/UI/Styles/EditorStylesCache.csEditor/Common/UI/Utils/EditorDrawUtils.csEditor/Common/UI/Utils/GuidPathCache.csEditor/Common/UI/Utils/GuidPathCache.cs.metaEditor/TextureCompressor/TextureCompressorEditor.csEditor/TextureCompressor/UI/Preview/PreviewSection.csEditor/TextureCompressor/UI/Sections/FrozenTexturesSection.csTests/Editor/UI/SearchBoxControlTests.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Editor/TextureCompressor/UI/Preview/PreviewSection.cs (1)
226-230: Consider clearing_guidPathCacheon close for consistency.
InvalidateCache()clears_guidPathCache, but closing the preview doesn't. While not a functional bug (the cache is cleared on next preview generation), clearing it here would be consistent and release memory sooner.♻️ Suggested change
if (GUILayout.Button("Close Preview")) { _showPreview = false; _previewData = null; + _guidPathCache.Clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/TextureCompressor/UI/Preview/PreviewSection.cs` around lines 226 - 230, When handling the "Close Preview" button (the GUILayout.Button("Close Preview") branch where _showPreview is set to false and _previewData is set to null), also clear the preview GUID cache to free memory and keep behavior consistent with InvalidateCache(); either call InvalidateCache() there or explicitly clear/reset _guidPathCache (e.g. _guidPathCache.Clear() or _guidPathCache = null), ensuring you reference the same _guidPathCache symbol used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Editor/TextureCompressor/UI/Preview/PreviewSection.cs`:
- Around line 226-230: When handling the "Close Preview" button (the
GUILayout.Button("Close Preview") branch where _showPreview is set to false and
_previewData is set to null), also clear the preview GUID cache to free memory
and keep behavior consistent with InvalidateCache(); either call
InvalidateCache() there or explicitly clear/reset _guidPathCache (e.g.
_guidPathCache.Clear() or _guidPathCache = null), ensuring you reference the
same _guidPathCache symbol used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e75b3809-f8cb-47c9-857b-ad3dd028e681
📒 Files selected for processing (3)
Editor/Common/UI/Styles/EditorStylesCache.csEditor/TextureCompressor/UI/Preview/PreviewSection.csTests/Editor/UI/SearchBoxControlTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Editor/Common/UI/Styles/EditorStylesCache.cs
- Tests/Editor/UI/SearchBoxControlTests.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/Editor/UI/GuidPathCacheTests.cs (1)
31-39: Make teardown deletion independent of successful import.Cleanup currently deletes only when
LoadAssetAtPathresolves an asset. If import fails, tracked files can be left behind. Prefer deleting by tracked path directly.♻️ Suggested cleanup hardening
foreach (var path in _createdAssetPaths) { - if ( - !string.IsNullOrEmpty(path) - && AssetDatabase.LoadAssetAtPath<Object>(path) != null - ) + if (!string.IsNullOrEmpty(path)) { AssetDatabase.DeleteAsset(path); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Editor/UI/GuidPathCacheTests.cs` around lines 31 - 39, Teardown currently only deletes assets when AssetDatabase.LoadAssetAtPath<Object>(path) returns non-null; change the cleanup to delete by tracked path regardless of import success: iterate _createdAssetPaths, skip null/empty strings, and call AssetDatabase.DeleteAsset(path) for each entry (optionally wrapping the call in a try/catch or checking the boolean return to log failures). Update the foreach in the teardown that references _createdAssetPaths and replace the conditional that uses AssetDatabase.LoadAssetAtPath with a direct delete call to ensure leftover files are removed even if import failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/Editor/UI/GuidPathCacheTests.cs`:
- Line 62: The single-line call assigning renameError should be reformatted to
the repository's multi-line call layout: replace the single-line
AssetDatabase.RenameAsset(originalPath, "GuidPathCacheTextureRenamed")
invocation with a multi-line invocation where AssetDatabase.RenameAsset is
called with each argument on its own line and the closing parenthesis on its own
line, preserving the assignment to the variable renameError (reference:
AssetDatabase.RenameAsset and renameError in GuidPathCacheTests).
---
Nitpick comments:
In `@Tests/Editor/UI/GuidPathCacheTests.cs`:
- Around line 31-39: Teardown currently only deletes assets when
AssetDatabase.LoadAssetAtPath<Object>(path) returns non-null; change the cleanup
to delete by tracked path regardless of import success: iterate
_createdAssetPaths, skip null/empty strings, and call
AssetDatabase.DeleteAsset(path) for each entry (optionally wrapping the call in
a try/catch or checking the boolean return to log failures). Update the foreach
in the teardown that references _createdAssetPaths and replace the conditional
that uses AssetDatabase.LoadAssetAtPath with a direct delete call to ensure
leftover files are removed even if import failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7120672-acc1-4ca5-9132-fb1ed8697d83
📒 Files selected for processing (4)
Editor/Common/UI/Utils/GuidPathCache.csTests/Editor/UI/GuidPathCacheTests.csTests/Editor/UI/GuidPathCacheTests.cs.metaTests/Editor/UI/SearchBoxControlTests.cs
✅ Files skipped from review due to trivial changes (2)
- Tests/Editor/UI/GuidPathCacheTests.cs.meta
- Tests/Editor/UI/SearchBoxControlTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Editor/Common/UI/Utils/GuidPathCache.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactoring
Bug Fixes
Style
Tests