You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Redesigned the Advanced Search dialog from a 4-column table into a more efficient 2-column layout with a dedicated filter panel and results view. Cleaned up rendering code, improved layout responsiveness, and streamlined the filter organization.
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp` — refactored UI into a 2-column layout with separate render functions for filters, updated bottom bar, and cleaned up rendering logic
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.h` — updated class structure, removed old column renderers, added new section-based render methods
Added state-based 'Searching...' UI feedback to AdvancedSearchDialog and SearchResultsWidget, and refactored the search results preview into a grid layout with card-based rendering.
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp` — added deferred search execution, state-based empty messages, grid layout for results, and search sorting logic
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.h` — added search state tracking flags and search execution method
- `ImguiMapEditor/UI/Widgets/SearchResultsWidget.cpp` — added searching state feedback and setter methods for search buffer and searching flag
- `ImguiMapEditor/UI/Widgets/SearchResultsWidget.h` — added search state flag and setter interfaces
This PR removes the QuickSearchPopup widget and ItemPickerService, consolidating search functionality into the AdvancedSearchDialog. Ctrl+F hotkey is remapped to open the advanced search dialog instead of the quick popup. The AdvancedSearchDialog is refactored with a grid-based preview UI replacing the previous list layout, implements deferred search execution via a new executeMapSearch() method, and reorganizes filter controls into dedicated render sections. SearchResultsWidget gains is_searching_ state tracking to display search progress. All callback wiring in Application, CallbackMediator, RenderOrchestrator, and MenuBar is updated to remove quick-search references and ensure consistent hotkey/menu integration.
Open-Tibia-Tools/tibia-imgui-map-editor#15: Modifies search menu and callback wiring in CallbackMediator/MenuBar and related hotkey behavior overlapping this PR's callback rewiring.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 8.57% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'feat(ui): Remake of Advanced Search' accurately reflects the main changes: removing the QuickSearchPopup component and significantly refactoring the AdvancedSearchDialog with a new 2-column grid layout for search results.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
Comment @coderabbitai help to get the list of available commands and usage tips.
Refactors the Advanced Search feature into a two-panel layout with improved state handling and result rendering, wires it to Ctrl+F and the Search menu as the primary catalog search, removes the legacy QuickSearchPopup and ItemPickerService, and enhances SearchResultsWidget with explicit searching state and messaging.
File-Level Changes
Change
Details
Files
Redesigned Advanced Search dialog UI and UX, including layout, filters, results rendering, and footer actions.
Replaced 4-column ImGui table layout with two bordered child panels for filters (left) and results (right), plus a bordered footer with centered buttons.
Split filter rendering into dedicated sections (Search, OR types, AND properties, Hints) using ImGui::SeparatorText and two-column checkbox tables to better group type and property filters.
Reimplemented result rendering from a vertical list with text and copy buttons into a responsive icon grid with 32x32 clickable cards, rounded backgrounds, hover tooltips with larger previews, and state-based empty messaging (idle, no filters, no matches, searching).
Added deferred map search execution via is_searching_ and pending_map_search_ flags so that the UI can show a "Searching..." state in both AdvancedSearchDialog and SearchResultsWidget before running the map search.
Changed footer layout to show result count on the left and compact right-aligned buttons (Search Map, disabled Select Item placeholder, Cancel) without fixed widths.
Adjusted search behavior and result ordering in Advanced Search and SearchResultsWidget to better communicate searching state and result semantics.
Updated updatePreviewResults to early-return only when both query and all filters are empty, and to sort preview results by item server_id ascending with creatures last.
Introduced explicit is_searching_ flag in AdvancedSearchDialog to drive empty-state messaging for results and to coordinate with SearchResultsWidget when a map search is initiated.
Extended SearchResultsWidget with is_searching_ state, a setter for it, and a setter for the search_buffer so that it can display appropriate messages (Searching..., No results found, Type to search...) and be pre-populated from AdvancedSearchDialog.
Ensured SearchResultsWidget clears is_searching_ in setResults and clear, and sets it to true in doSearch before invoking the async search callback.
Rewired keyboard shortcuts and menu actions so Advanced Search replaces the old Quick Search as the primary catalog search entry point.
Changed CallbackMediator wiring to open AdvancedSearchDialog from the Search menu "Find Items..." action and from the hotkey quick search callback (Ctrl+F), while keeping Ctrl+Shift+F mapped to show the SearchResultsWidget.
Updated MenuBar search menu to remove "Quick Find" and repurpose "Find Items..." to use the QUICK_SEARCH shortcut label and a tooltip describing filtered catalog search.
Adjusted RenderOrchestrator to render only the AdvancedSearchDialog (as Ctrl+F) and removed references to QuickSearchPopup in the dialog rendering pipeline.
Updated Application and related context structs to pass around only AdvancedSearchDialog and SearchResultsWidget pointers for search UI components.
Removed legacy QuickSearchPopup and ItemPickerService infrastructure and simplified SearchController wiring.
Deleted ItemPickerService and QuickSearchPopup source and header files, and removed them from CMakeLists.txt build configuration.
Simplified SearchController by removing QuickSearchPopup management and ItemPickerService wiring, leaving only AdvancedSearchDialog and SearchResultsWidget as UI components and MapSearchService as the backing service.
Cleaned up CallbackMediator, RenderOrchestrator, and related context structs to drop QuickSearchPopup and ItemPickerService references and fields.
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
In AdvancedSearchDialog::updatePreviewResults, the condition if (search_items && search_service_ && !property_filter_.hasAnySelected() || !type_filter_.creature) relies on &&/|| precedence and is hard to read; consider adding parentheses to make the intended logic explicit (e.g., if ((search_items && search_service_ && !property_filter_.hasAnySelected()) || !type_filter_.creature) or similar).
In the sorting lambda for preview_results_, the server ID is narrowed to uint16_t (uint16_t a_id = ...); if server IDs can exceed 65535, consider using the original wider type to avoid silent truncation affecting sort order.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `AdvancedSearchDialog::updatePreviewResults`, the condition `if (search_items && search_service_ && !property_filter_.hasAnySelected() || !type_filter_.creature)` relies on `&&`/`||` precedence and is hard to read; consider adding parentheses to make the intended logic explicit (e.g., `if ((search_items && search_service_ && !property_filter_.hasAnySelected()) || !type_filter_.creature)` or similar).
- In the sorting lambda for `preview_results_`, the server ID is narrowed to `uint16_t` (`uint16_t a_id = ...`); if server IDs can exceed 65535, consider using the original wider type to avoid silent truncation affecting sort order.
## Individual Comments### Comment 1
<locationpath="ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp"line_range="401-407" />
<code_context>
- // Search items (only if not creature-only filter)
+
bool search_items = !type_filter_.creature || type_filter_.hasAnySelected();
if (search_items && search_service_ && !property_filter_.hasAnySelected() || !type_filter_.creature) {
- // If only creature is selected, skip item search- if (!(type_filter_.creature && !type_filter_.depot && !type_filter_.mailbox && + if (!(type_filter_.creature && !type_filter_.depot && !type_filter_.mailbox && !type_filter_.trash_holder && !type_filter_.container && !type_filter_.door && !type_filter_.magic_field && !type_filter_.teleport && !type_filter_.bed && !type_filter_.key && !type_filter_.podium)) {- + auto item_results = search_service_->searchItemDatabase(- search_buffer_,- type_filter_,
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify boolean logic in item search condition to avoid unintended behavior.
The condition `if (search_items && search_service_ && !property_filter_.hasAnySelected() || !type_filter_.creature)` relies on `&&`/`||` precedence and is effectively `if ((search_items && search_service_ && !property_filter_.hasAnySelected()) || !type_filter_.creature)`. This allows the body to run whenever `!type_filter_.creature` is true, even if `search_items` is false or `search_service_` is null (which would then be dereferenced). Please add explicit grouping (e.g. `if (search_service_ && search_items && !property_filter_.hasAnySelected())`) and handle the `!type_filter_.creature` case separately if it’s intended as a special override.
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR successfully remakes the Advanced Search dialog, replacing the older Quick Search popup with a more robust 2-column layout and grid-based results with sprite previews. It also improves the Search Results Widget by adding a "Searching..." state and better empty state handling.
🔍 General Feedback
Architecture: The removal of ItemPickerService and the consolidation of search logic into MapSearchService and AdvancedSearchDialog is a good simplification.
UI/UX: The new grid-based results with tooltips and double-click to search map is a significant improvement over the old list-based quick search.
Correctness: Found one logical bug in updatePreviewResults that could prevent item results from showing when both creature and property filters are active.
Performance: The grid uses ImGuiListClipper, which is good for performance with large result sets.
I've left a few inline comments regarding the logic bug and some minor UI polish opportunities.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Bug in logic: This condition incorrectly skips item search if a creature filter is selected alongside any property filter, even if item types (like Depot) are also selected.
For example, if the user selects Type: Depot and Type: Creature and Property: Unpassable, they expect to see Unpassable Depots. However, because !type_filter_.creature is false and !property_filter_.hasAnySelected() is false, the entire condition evaluates to false, and no items are searched.
The item search should be performed if at least one item type is selected (or no types are selected) regardless of whether the creature filter is active, as searchItemDatabase correctly handles the property filtering.
// ... search_service_->searchItemDatabase already handles filtering items by type and property.
if (search_service_) {
(Wait, I should check if hasAnySelected includes creature. If it does, then the suggested check is type_filter_.hasAnySelected()).
Actually, the inner check already filters out the "only creature" case. So you can probably just simplify this to if (search_service_) {.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The is_searching_ flag in the widget is set to true here, but AdvancedSearchDialog::executeMapSearch (called next frame) will immediately call results_widget_->setResults(), which sets is_searching_ back to false before the widget even renders its next frame. Similar to the dialog's own is_searching_ flag, this deferred logic doesn't actually result in a visible "Searching..." state for synchronous searches.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The "Searching..." state will never be rendered because is_searching_ is set to false immediately before calling executeMapSearch() and then rendering the rest of the frame.
To fix this, executeMapSearch() should be deferred by one more frame, or is_searching_ should only be set to false AFTER the search completes (e.g. at the end of executeMapSearch).
However, since the search is synchronous and blocks the main thread, the user still won't see the spinner until the next frame anyway. If the search is fast, this doesn't matter much.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sorting by uint16_t might be fragile if server IDs ever exceed 65535 (though unlikely in current OT protocol). Using int or uint32_t for a_id/b_id would be safer. Also, using a larger constant than UINT16_MAX for creatures (like 0xFFFFFFFF) would ensure they always stay at the end.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The remake of the Advanced Search dialog is currently not up to standards. A critical logic error in the filtering condition will cause a application crash when searching for items because operator precedence allows the code to bypass a null check on the search service. Furthermore, the 'Searching...' state implementation is functionally broken as it executes a blocking search on the main thread before the UI can render the updated state, resulting in a frame hitch rather than a progress indicator.
The PR also introduces a feature regression by removing the 'Copy' utility for IDs/Names and includes incomplete 'Select Item' functionality. Security-wise, the use of strncpy in the search widget should be replaced with safer alternatives to comply with secure coding standards.
About this PR
The PR description is empty. Please provide context regarding the design requirements and confirm if the removal of the 'Copy ID/Name' feature was an intentional design decision.
The 'Select Item' button is currently a disabled placeholder. If this feature is out of scope for this PR, consider removing the UI element until the backend logic is ready to avoid user confusion.
Test suggestions
Search with name or ID correctly filters the preview results list
Applying multiple type filters (OR) broadens the result set
Applying property filters (AND) narrows the result set
Double-clicking a result item triggers a map search through the SearchService
Deferred search logic correctly triggers 'is_searching' state before executing the search
Sorting correctly handles creatures by moving them to the end of the results
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Search with name or ID correctly filters the preview results list
2. Applying multiple type filters (OR) broadens the result set
3. Applying property filters (AND) narrows the result set
4. Double-clicking a result item triggers a map search through the SearchService
5. Deferred search logic correctly triggers 'is_searching' state before executing the search
6. Sorting correctly handles creatures by moving them to the end of the results
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Critical: Potential null pointer dereference due to operator precedence. If type_filter_.creature is false, the search_service_ null check is bypassed. Wrap the search service and property filter checks in parentheses or add an explicit guard: (search_items && search_service_ && !property_filter_.hasAnySelected()) || !type_filter_.creature still risks the crash if the right side is true; the logic should likely be search_service_ && ((search_items && !property_filter_.hasAnySelected()) || !type_filter_.creature).
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Suggestion: The use of strncpy is discouraged as it is error-prone regarding null-termination. Use snprintf or a safer string-copy alternative to prevent potential buffer overflows.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImguiMapEditor/Application/CallbackMediator.cpp`:
- Around line 542-545: The hotkey handler registered via
ctx.hotkey->setAdvancedSearchCallback currently forces vs->show_search_results =
true so it never closes; change the lambda in that registration (the closure
capturing vs from ctx.view_settings) to toggle the flag instead (set
vs->show_search_results to its negation) so Ctrl+Shift+F both opens and closes
the Search Results widget.
In `@ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp`:
- Around line 467-475: AdvancedSearchDialog::render currently calls
executeMapSearch which invokes MapSearchService::search() inline (blocking the
UI); replace those synchronous calls so the creature/item branches use the
service's asynchronous search path (the same async API used elsewhere) instead
of MapSearchService::search(). Keep setting
results_widget_->setSearchBuffer(...) as before, invoke the async search on
search_service_ with the same search string and mode (for creature name and item
server_id), and wire the async completion callback to populate the local results
variable and update the results_widget_ (and cancel any prior async search if
the service supports cancellation) so the map scan runs off the render thread
rather than synchronously in executeMapSearch/AdvancedSearchDialog::render.
- Around line 400-408: The if-condition that gates calling
search_service_->searchItemDatabase can be true when search_service_ is null
because of operator precedence and the trailing "!type_filter_.creature" term;
update the condition so search_service_ is always required before dereferencing
(e.g. group the left-side checks with parentheses) and also extend the
creature-only exclusion list to include type_filter_.weapon, type_filter_.armor,
type_filter_.ammo, and type_filter_.rune so creature-only cases still suppress
item previews; specifically modify the boolean expression that uses
search_items, search_service_, property_filter_.hasAnySelected(), and all
type_filter_ flags guarding the call to search_service_->searchItemDatabase to
ensure (search_items && search_service_ && !property_filter_.hasAnySelected())
is evaluated together and the creature-only check includes
weapon/armor/ammo/rune.
In `@ImguiMapEditor/UI/Widgets/SearchResultsWidget.cpp`:
- Around line 434-437: Currently the widget sets is_searching_ = true before
checking on_search_async_, which leaves the widget stuck in searching state if
the callback isn't wired (SearchController::onMapLoaded). Change the logic so
you only set is_searching_ to true when on_search_async_ is valid: check
on_search_async_ first and then set is_searching_ and call
on_search_async_(search_buffer_, search_items_, search_creatures_); otherwise
ensure is_searching_ is explicitly set to false (or early-return) so the flag is
never left true when no backend callback exists.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c190ec71-08fd-48e9-8ef2-9a690ea687ea
📥 Commits
Reviewing files that changed from the base of the PR and between e98957a and f34a8a1.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Don't scan the map synchronously from the render loop.
executeMapSearch() is called from AdvancedSearchDialog::render(), but it performs MapSearchService::search() inline. That search walks the map, so on large maps this still blocks the UI thread right after the one-frame "Searching..." state. Please route this through the existing async search path instead of calling search() directly here.
🧰 Tools🪛 Cppcheck (2.20.0)
[style] 467-467: The function 'forEachTileOnFloor' is never used.
(unusedFunction)
[style] 474-474: The function 'forEachTileOnFloorMutable' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp` around lines 467 - 475,
AdvancedSearchDialog::render currently calls executeMapSearch which invokes
MapSearchService::search() inline (blocking the UI); replace those synchronous
calls so the creature/item branches use the service's asynchronous search path
(the same async API used elsewhere) instead of MapSearchService::search(). Keep
setting results_widget_->setSearchBuffer(...) as before, invoke the async search
on search_service_ with the same search string and mode (for creature name and
item server_id), and wire the async completion callback to populate the local
results variable and update the results_widget_ (and cancel any prior async
search if the service supports cancellation) so the map scan runs off the render
thread rather than synchronously in
executeMapSearch/AdvancedSearchDialog::render.
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
The PR successfully refactors the search functionality by consolidating the "Quick Search" and "Advanced Search" into a single, unified experience. The redesign of the AdvancedSearchDialog into a 2-column layout with a results grid is a significant UX improvement, and the use of ImGuiListClipper ensures it remains performant even with large result sets.
However, there are some concerns regarding the performance of the map search, which is currently executed synchronously on the main thread, potentially leading to UI freezes. Additionally, the handling of the "Searching..." state has some minor logical flaws that might prevent the spinner from being visible.
🔍 General Feedback
Performance: offload executeMapSearch to a background thread to prevent blocking the UI during map-wide searches.
UX: The grid layout is excellent, but a slightly larger cell size might help in identifying creatures and complex items.
Consistency: Consider unifying the async search patterns across SearchController, AdvancedSearchDialog, and SearchResultsWidget.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The 32x32 CELL_SIZE might be too cramped for some creature previews or larger items. While Tibia tiles are 32x32, multi-tile creatures might not be very recognizable at this scale. Increasing this slightly (e.g., to 48x48) might improve usability while still keeping a good density.
The reason will be displayed to describe this comment to others. Learn more.
🟡 filters_changed_ is set to true on every keystroke in the search input. While searchItemDatabase is generally fast, iterating over ~30k item types and performing fuzzy matching on every character might cause micro-stutters. Consider adding a small debounce or only updating when the query length is >= 2-3 characters (unless it's numeric).
The reason will be displayed to describe this comment to others. Learn more.
🟡 setSearching is called here, but executeMapSearch in the next frame sets is_searching_ = false before actually finishing the work. This defeats the purpose of the spinner.
The reason will be displayed to describe this comment to others. Learn more.
🟡 setResults clears is_searching_, which is correct if the search is async. However, if the caller is synchronous (like AdvancedSearchDialog), the UI will be frozen during the search anyway.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The footer height calculation uses a magic number 20.0f. Consider using style variables (e.g., ImGuiStyleVar_ItemSpacing) or a more descriptive constant for better maintainability and scaling with different DPI/themes.
The reason will be displayed to describe this comment to others. Learn more.
🔴 executeMapSearch is called synchronously on the UI thread. Since it iterates over all map tiles (via search_service_->search), this will cause a significant UI freeze on large maps. It should be offloaded to a background thread, possibly by utilizing the SearchController::launchAsync mechanism already present in the codebase.
The reason will be displayed to describe this comment to others. Learn more.
🟡 is_searching_ is set to false immediately before calling executeMapSearch. Combined with the fact that executeMapSearch is synchronous, the "Searching..." state and spinner in SearchResultsWidget will likely never be visible to the user, or only for a single frame.
if (pending_map_search_) {
pending_map_search_ = false;
is_searching_ = false; // Should remain true until search completesexecuteMapSearch();
}
The reason will be displayed to describe this comment to others. Learn more.
🟡 The SearchResultsWidget now has its own is_searching_ state, but it seems to be duplicated with the logic in AdvancedSearchDialog. It might be cleaner to have SearchResultsWidget manage its own "searching" state or have a unified way to report progress if searches are moved to SearchController.
Removed the `is_searching_` state flag and associated UI logic, as deferred search execution was already handling the requirement effectively. Adjusted footer padding calculation for better UI spacing consistency.
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.cpp` — removed `is_searching_` management, updated search triggering logic, and refined footer height calculation
- `ImguiMapEditor/UI/Dialogs/AdvancedSearchDialog.h` — removed `is_searching_` member variable
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR remakes the Advanced Search dialog and consolidates it with the Quick Search functionality. The UI has been improved with a grid preview of results and better state management (e.g., "Searching..." state).
🔍 General Feedback
Overall, the refactoring is a good step towards a more unified search experience. The use of deferred search logic and ImGuiListClipper shows attention to performance and UI responsiveness.
I've noted a few minor issues regarding:
Fragile logic in filter checking.
Potential edge cases in sorting.
Minor UX improvements for the loading state.
Defensive programming for layout calculations.
Most of these are non-blocking but would improve the robustness of the search feature.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The creature_only boolean calculation is fragile as it manually excludes every other flag in TypeFilter. If TypeFilter is updated with new types in the future, this logic will likely break or become incorrect.
Consider adding a method like TypeFilter::hasAnyItemTypeSelected() to encapsulate this check.
The reason will be displayed to describe this comment to others. Learn more.
🟡 If footer_h happens to be larger than the available vertical space, content_h will be negative, which can lead to undefined behavior or assertions in ImGui::BeginChild.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Using UINT16_MAX to sort creatures at the end might cause unexpected ordering if an item in the database actually has a server_id of 65535. While rare in many OT versions, it's a magic number dependency.
Consider using a more robust comparison that explicitly checks is_creature first.
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
Area: UIChanges to the wxWidgets GUI (palettes, dialogs, menus, toolbar).Type: EnhancementNew feature or request.
2 participants
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.
github-actions
Bot
added
Type: Enhancement
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
Refactor
UI