feat: New improved Search Tool - #999
Conversation
…arch Replaced legacy find item dialog with new advanced finder featuring fuzzy name search, type/property/interaction/visual filters, creature search support, and state persistence. Added ForceUse/MultiUse/FullTile item flags with parser support across DAT/OTB/XML formats. Updated C++ standard to C++23.
Extracted results rendering into new find_item_window_views module with List/Grid view modes using NanoVG. Replaced legacy radio-button search modes with unified token-based fuzzy search supporting wildcards and numeric matching. Updated CMakeLists.txt to register new source files. Removed deprecated search mode persistence from navigation_menu_handler.cpp and search_handler.cpp. Refactored find_item_window_model.cpp scoring logic to use multi-token matching with prefix/suffix wildcard support. Modernized find_item_window.cpp layout with left-side filter panel and tabbed results view.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant overhaul of the search functionality within the application. By replacing the legacy search dialog with a more robust and flexible 'Advanced Search Tool', users can now perform more complex queries with better filtering and visualization options. The changes also include necessary backend updates to support these new features, including updated item parsing logic and modern C++ standards. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughReplaces the legacy Find Item dialog with an Advanced Item Finder (model, views, dialog); adds paginated item/creature search operations, new item flags and parser mappings, paged search result UI, settings persistence, and raises C++ target standard to C++23. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dialog as FindItemDialog
participant Model as AdvancedFinderModel
participant View as AdvancedFinderResultsView
participant Handler as SearchHandler
participant Editor as EditorOperations
User->>Dialog: Enter query and set filters
Dialog->>Model: FilterAdvancedFinderCatalog(catalog, query)
Model-->>Dialog: Matched & scored indices
Dialog->>View: SetRows(filtered rows, preferred selection)
User->>View: Select / activate / right-activate
View-->>Dialog: Selection/activation event
Dialog->>Dialog: Determine ResultAction
alt ResultAction == SearchMap
Dialog->>Handler: Request paged search (kind, id/brush, offset)
Handler->>Editor: Run ItemSearcher/CreatureSearcher (with offset)
Editor-->>Handler: Return paged results + total
Handler->>SearchResultWindow: SetResults(rows, total, offset, limit, page_loader)
else ResultAction == SelectItem / ConfirmSelection
Dialog->>Handler: Request selection handling
Handler->>Editor: Select brush / update palette
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to the C++23 standard and introduces a comprehensive "Advanced Item Finder" UI, refactoring the legacy item search into a model-view architecture. The update includes new NanoVG-based list and grid views, expanded item flag parsing for OTB, DAT, and XML formats, and a new creature search capability. Review feedback highlights several violations of the repository's 500-line file limit in the new UI components and the requirement to use std::format instead of wxString::Format. Additionally, suggestions were made to improve architectural robustness by encapsulating session state and using explicit configuration for dialog instances.
| // along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| ////////////////////////////////////////////////////////////////////// | ||
|
|
||
| #include "app/main.h" |
There was a problem hiding this comment.
This file has been significantly refactored and is now over 700 lines long. The repository style guide (line 25) suggests splitting files that exceed 500 lines. Please consider refactoring this into smaller, more focused components. For example, the layout logic in buildLayout could be broken down into more helper functions, or parts of the dialog's logic could be extracted into separate helper classes.
| input_timer.Bind(wxEVT_TIMER, &FindItemDialog::OnInputTimer, this); | ||
| ok_button->Bind(wxEVT_BUTTON, &FindItemDialog::OnClickOK, this); | ||
| cancel_button->Bind(wxEVT_BUTTON, &FindItemDialog::OnClickCancel, this); | ||
| std::optional<SessionFinderState> g_session_finder_state; |
There was a problem hiding this comment.
The use of a file-scoped global variable g_session_finder_state to manage state between dialog invocations can make the code harder to reason about and maintain. While it's scoped to this file via an anonymous namespace, a better approach would be to encapsulate this session state within a manager class that can be passed to the dialog or accessed via a service locator. This would make the data flow more explicit.
| @@ -0,0 +1,558 @@ | |||
| #include "ui/find_item_window_model.h" | |||
There was a problem hiding this comment.
This new file is over 550 lines long, which exceeds the 500-line limit suggested by the repository style guide (line 25). To improve maintainability, please consider splitting this file into smaller, more focused modules. For example, the query parsing logic, catalog building, and state persistence could each be extracted into their own files.
| @@ -0,0 +1,842 @@ | |||
| #include "ui/find_item_window_views.h" | |||
There was a problem hiding this comment.
This new file is over 840 lines long, and the AdvancedFinderResultsView class is very large. This violates the repository style guide's 500-line limit for files (line 25). To improve maintainability and adhere to the Single Responsibility Principle, please refactor this. For instance, AdvancedFinderResultsView could be split into a base class with separate List and Grid view implementations. The rendering logic for different item types could also be extracted into helper classes or functions.
| @@ -108,7 +142,6 @@ void SearchHandler::OnSearchForItemOnSelection(wxCommandEvent& WXUNUSED(event)) | |||
| } | |||
|
|
|||
| FindItemDialog dialog(frame, "Search on Selection"); | |||
There was a problem hiding this comment.
The new FindItemDialog is being used here with its default parameters. This is fragile because it relies on the implicit behavior of the dialog's constructor and doesn't clearly state the intent for this specific use case. The new dialog is much more powerful than the old one, and its behavior can be configured.
To make the code more robust and readable, please explicitly configure the FindItemDialog for the needs of OnSearchForItemOnSelection. For example, if it should only allow selecting an item and not searching the map, you should pass the appropriate ActionSet and other parameters to the constructor.
FindItemDialog dialog(frame, "Search on Selection", false, FindItemDialog::ActionSet::ConfirmOnly);|
|
||
| void FindItemDialog::updateResultTitle(size_t count) const { | ||
| if (result_count_label_ != nullptr) { | ||
| result_count_label_->SetLabel(wxString::Format("Results: %zu |", count)); |
There was a problem hiding this comment.
The style guide (line 84) mandates the use of std::format over wxString::Format. Please update this line to use std::format for consistency and adherence to the project's coding standards. You will need to #include <format> in this file.
result_count_label_->SetLabel(std::format("Results: {} |", count));![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This Pull Request introduces a modern, feature-rich "Advanced Item Finder" to the editor, significantly enhancing the search capabilities with fuzzy matching, extensive filtering (by type, properties, interactions, and visuals), and a polished NanoVG-based UI with list and grid views. The implementation is well-structured and follows modern C++ practices.
🔍 General Feedback
- Visual Polish: The addition of the NanoVG-based results view and preview panel provides a very professional and responsive experience.
- C++23 Migration: The upgrade to C++23 is a bold move that allows for cleaner code (like
std::to_underlying), but should be double-checked for toolchain compatibility across all target platforms. - Performance Considerations: While the current implementation is efficient for thousands of items, caching the catalog and adding a small debounce to the search field would make it even snappier and more robust on lower-end hardware.
- Improved Parsers: The updates to the DAT and OTB parsers to include new flags like
ForceUseandFullTileare important for the search tool's advanced filtering logic.
| } | ||
| } | ||
|
|
||
| std::vector<AdvancedFinderCatalogRow> BuildAdvancedFinderCatalog(bool include_creatures) { |
There was a problem hiding this comment.
| add_executable(rme ${rme_H} ${rme_SRC}) | ||
|
|
||
| set_target_properties(rme PROPERTIES CXX_STANDARD 20) | ||
| set_target_properties(rme PROPERTIES CXX_STANDARD 23) |
There was a problem hiding this comment.
| return std::string(value.substr(first, last - first + 1)); | ||
| } | ||
|
|
||
| [[nodiscard]] std::vector<std::string> tokenizeLower(std::string_view value) { |
There was a problem hiding this comment.
|
|
||
| void FindItemDialog::OnTypeChange(wxCommandEvent& WXUNUSED(event)) { | ||
| RefreshContentsInternal(); | ||
| void FindItemDialog::OnTextChanged(wxCommandEvent& WXUNUSED(event)) { |
There was a problem hiding this 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: 7
🧹 Nitpick comments (3)
source/app/settings.cpp (1)
324-327: Persist these filter masks as unsigned values.These keys hold
AdvancedFinderFilterMaskvalues, butInt(...)routes them throughgetInteger()/setInteger(), which are signed. That gives the new settings a 31-bit ceiling, and any future filter using bit 31 will round-trip as negative and then load back as0insource/ui/find_item_window_model.cpp. I’d switch these to uint32-aware settings accessors or a string representation before the filter enums grow further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/settings.cpp` around lines 324 - 327, The four Int(...) calls store AdvancedFinderFilterMask values as signed ints causing overflow for bit31; change them to a uint32-aware settings accessor (e.g., replace Int(ADVANCED_ITEM_FINDER_TYPE_FILTERS, ...) / Int(ADVANCED_ITEM_FINDER_PROPERTY_FILTERS, ...) / Int(ADVANCED_ITEM_FINDER_INTERACTION_FILTERS, ...) / Int(ADVANCED_ITEM_FINDER_VISUAL_FILTERS, ...) with the project’s unsigned setting API such as UInt/UnsignedInt/GetUnsigned/SetUnsigned that round-trips a 32-bit value) or else serialize the mask to a string; also update the load/save paths used by source/ui/find_item_window_model.cpp to use the same unsigned reader so AdvancedFinderFilterMask bits >=31 survive.source/ui/menubar/search_handler.cpp (1)
16-60: Extract these advanced-finder helpers into one shared utility.This block now duplicates
selectFoundBrush, item search result wiring, and creature search result wiring fromsource/ui/menubar/navigation_menu_handler.cpp. Keeping the result-window plumbing in two places will make the next behavior tweak drift between the two menus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/menubar/search_handler.cpp` around lines 16 - 60, These three functions (selectFoundBrush, showItemSearchResults, showCreatureSearchResults) duplicate result-window and load-bar plumbing found in navigation_menu_handler.cpp; extract that common logic into a single shared helper (e.g., a utility function or small class in a common UI helper namespace) that accepts the searcher/result types (or a callable adapter), the load_bar_label, and the result-to-display mapping, reusing EditorOperations::ItemSearcher/CreatureSearcher and SearchResultWindow interactions (CreateLoadBar, DestroyLoadBar, limitReached/maxCount messaging, window->Clear(), and window->AddPosition). Then replace the local implementations in both files to call the new shared helper while keeping the existing function names/signatures used by callers (selectFoundBrush, showItemSearchResults, showCreatureSearchResults) to minimize call-site changes.source/ui/find_item_window_model.h (1)
83-86: Consider simplifying the cast chain.The outer cast to
AdvancedFinderFilterMaskfor the shift amount is semantically unusual—shift amounts are typically plain integers. Thestd::underlying_type_t<Enum>result (uint8_t) already promotes naturally for the shift operation.♻️ Simplified version
template <typename Enum> constexpr AdvancedFinderFilterMask advancedFinderBit(Enum value) { - return AdvancedFinderFilterMask { 1 } << static_cast<AdvancedFinderFilterMask>(static_cast<std::underlying_type_t<Enum>>(value)); + return AdvancedFinderFilterMask{1} << static_cast<std::underlying_type_t<Enum>>(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/find_item_window_model.h` around lines 83 - 86, Simplify advancedFinderBit by removing the outer cast to AdvancedFinderFilterMask for the shift amount: compute the shift using the enum's underlying integer (e.g., std::underlying_type_t<Enum> or an int/unsigned) and let ordinary integer promotion perform the shift, then construct/return the AdvancedFinderFilterMask from (AdvancedFinderFilterMask{1} << that_integer); update the expression in advancedFinderBit accordingly so the shift operand is a plain integer rather than an AdvancedFinderFilterMask.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/editor/operations/search_operations.h`:
- Around line 154-156: The traversal isn't short-circuited because
CreatureSearcher::operator()(Map&, Tile*, long long) returns early but
foreach_TileOnMap (the foreach wrapper in source/map/map.h that currently uses
std::ranges::for_each and discards the visitor return) never observes that;
change the iteration contract so the visitor can signal stop (e.g., make the
visitor return bool) and update foreach_TileOnMap to use a loop that checks the
visitor's return and breaks when it returns false (replace the
std::ranges::for_each usage with a manual loop or a std::any_of/for loop that
respects the boolean), and update CreatureSearcher::operator() to return the
appropriate boolean (true to continue, false to stop) so maxCount actually stops
traversal.
In `@source/ui/find_item_window_model.cpp`:
- Around line 153-185: Both scoreTextTokenMatch and scoreNumericTokenMatch fail
to treat queries with both prefix_wildcard and suffix_wildcard as "contains"
matches; update these functions (scoreTextTokenMatch and scoreNumericTokenMatch)
to check if query_token.prefix_wildcard && query_token.suffix_wildcard and then
test name_token.contains(query_token.value) and return an appropriate score
(e.g., a distinct base like 30 + length difference or similar to other wildcard
branches) before falling back to prefix/suffix rules so queries like *term*
match infix occurrences such as "sword".
In `@source/ui/find_item_window_views.cpp`:
- Around line 590-600: The hit-testing currently treats the first pixel of the
gap as part of the adjacent card because the checks use '>' instead of '>=';
update the boundary checks in the hit-test logic around row_height/row_offset
and column_width/column_offset (the block computing row, row_offset, column,
column_offset in find_item_window_views.cpp) to treat offsets equal to
card_height_ or card_width_ as outside the card (i.e., replace the row_offset >
card_height_ and column_offset > card_width_ conditions with >= so gap pixels
are non-interactive).
In `@source/ui/find_item_window.cpp`:
- Around line 395-403: The dialog currently ignores the persisted
state.last_action when restoring; update the restore/initialization logic to set
default_action_ from state.last_action before applying button defaults so the
user's previous "SearchMap" vs "SelectItem" choice is restored. Concretely, in
the code paths that set button defaults (the block using action_set_ ==
ActionSet::SearchAndSelect that calls search_map_button_->SetDefault() or
select_item_button_->SetDefault(), and the similar block at the 413-417
location), assign default_action_ = state.last_action (or read state.last_action
into default_action_ or a local used to choose the default) and then call
SetDefault() on the matching widget; ensure the other branch that sets
ok_button_->SetDefault() is unchanged.
- Line 148: g_session_finder_state (SessionFinderState) is being restored too
early and saved back unconditionally, so transient dialogs (e.g., ConfirmOnly)
overwrite the shared finder session; fix by ensuring the global
g_session_finder_state is only updated/restored from a window instance when
persist_shared_state_ is true and only written back on close when
persist_shared_state_ is true, and adjust the restore order so the persisted
shared state is loaded before any transient restoration; apply these guards in
the FindItemWindow constructor/restore logic and in the close/save path (the
places that currently touch g_session_finder_state and persist_shared_state_,
including ConfirmOnly handling) so one-off dialogs never replace the main
finder's query/selection/view mode.
- Around line 234-237: The Hints box in buildLayout() always shows both
"double-left-click to search the map" and "double-right-click to select in the
tileset", which is wrong for dialogs with action_set_ == ConfirmOnly; change
buildLayout() so the two createHintText() calls are conditional based on
action_set_ (only add the left-click hint when the action_set_ enables map
search and only add the right-click hint when it enables tileset selection).
Locate buildLayout(), the hints_box variable and the createHintText(...) calls
and wrap each Add(...) in a check against action_set_ (or use the existing
action_set_ query methods/flags) so the hint text matches the actual available
actions.
In `@source/ui/menubar/navigation_menu_handler.cpp`:
- Around line 125-133: The SearchMap branch in the Jump to Item handler can run
without an open editor; before calling showCreatureSearchResults or
showItemSearchResults for the FindItemDialog ResultAction::SearchMap, add the
same editor-open guard used elsewhere (check g_gui.IsEditorOpen()) and bail out
(or treat as cancel) if no editor is open; locate the branch handling
dialog.getResultAction() == FindItemDialog::ResultAction::SearchMap in the code
around FindItemDialog usage and wrap the calls to
showCreatureSearchResults/showItemSearchResults with that guard (or redirect to
the normal non-map search flow).
---
Nitpick comments:
In `@source/app/settings.cpp`:
- Around line 324-327: The four Int(...) calls store AdvancedFinderFilterMask
values as signed ints causing overflow for bit31; change them to a uint32-aware
settings accessor (e.g., replace Int(ADVANCED_ITEM_FINDER_TYPE_FILTERS, ...) /
Int(ADVANCED_ITEM_FINDER_PROPERTY_FILTERS, ...) /
Int(ADVANCED_ITEM_FINDER_INTERACTION_FILTERS, ...) /
Int(ADVANCED_ITEM_FINDER_VISUAL_FILTERS, ...) with the project’s unsigned
setting API such as UInt/UnsignedInt/GetUnsigned/SetUnsigned that round-trips a
32-bit value) or else serialize the mask to a string; also update the load/save
paths used by source/ui/find_item_window_model.cpp to use the same unsigned
reader so AdvancedFinderFilterMask bits >=31 survive.
In `@source/ui/find_item_window_model.h`:
- Around line 83-86: Simplify advancedFinderBit by removing the outer cast to
AdvancedFinderFilterMask for the shift amount: compute the shift using the
enum's underlying integer (e.g., std::underlying_type_t<Enum> or an
int/unsigned) and let ordinary integer promotion perform the shift, then
construct/return the AdvancedFinderFilterMask from (AdvancedFinderFilterMask{1}
<< that_integer); update the expression in advancedFinderBit accordingly so the
shift operand is a plain integer rather than an AdvancedFinderFilterMask.
In `@source/ui/menubar/search_handler.cpp`:
- Around line 16-60: These three functions (selectFoundBrush,
showItemSearchResults, showCreatureSearchResults) duplicate result-window and
load-bar plumbing found in navigation_menu_handler.cpp; extract that common
logic into a single shared helper (e.g., a utility function or small class in a
common UI helper namespace) that accepts the searcher/result types (or a
callable adapter), the load_bar_label, and the result-to-display mapping,
reusing EditorOperations::ItemSearcher/CreatureSearcher and SearchResultWindow
interactions (CreateLoadBar, DestroyLoadBar, limitReached/maxCount messaging,
window->Clear(), and window->AddPosition). Then replace the local
implementations in both files to call the new shared helper while keeping the
existing function names/signatures used by callers (selectFoundBrush,
showItemSearchResults, showCreatureSearchResults) to minimize call-site changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36ceeada-2a1b-450d-9568-0176b52abe2f
📒 Files selected for processing (17)
CMakeLists.txtsource/CMakeLists.txtsource/app/settings.cppsource/app/settings.hsource/editor/operations/search_operations.hsource/item_definitions/core/item_definition_types.hsource/item_definitions/formats/dat/dat_item_parser.cppsource/item_definitions/formats/otb/otb_item_parser.cppsource/item_definitions/formats/xml/xml_item_parser.cppsource/ui/find_item_window.cppsource/ui/find_item_window.hsource/ui/find_item_window_model.cppsource/ui/find_item_window_model.hsource/ui/find_item_window_views.cppsource/ui/find_item_window_views.hsource/ui/menubar/navigation_menu_handler.cppsource/ui/menubar/search_handler.cpp
| void operator()(Map& map, Tile* tile, long long done) { | ||
| if (result.size() >= static_cast<size_t>(maxCount)) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'map_search*' source
rg -n -C5 '\bforeach_TileOnMap\b' sourceRepository: karolak6612/remeres-map-editor-redux
Length of output: 3434
The traversal is not short-circuited; maxCount only caps displayed results.
The foreach_TileOnMap function at source/map/map.h:238-243 uses std::ranges::for_each and discards the return value from the visitor:
std::ranges::for_each(map.tiles(), [&](auto& tile_loc) {
foreach (map, tile_loc.get(), ++done)
;
});Early return from CreatureSearcher::operator() does not stop iteration. The entire map is always traversed even after result.size() >= maxCount is reached. The maxCount guard caps displayed results only, not runtime. Fix foreach_TileOnMap to respect early termination (e.g., by checking a return value or using a different iteration mechanism).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/editor/operations/search_operations.h` around lines 154 - 156, The
traversal isn't short-circuited because CreatureSearcher::operator()(Map&,
Tile*, long long) returns early but foreach_TileOnMap (the foreach wrapper in
source/map/map.h that currently uses std::ranges::for_each and discards the
visitor return) never observes that; change the iteration contract so the
visitor can signal stop (e.g., make the visitor return bool) and update
foreach_TileOnMap to use a loop that checks the visitor's return and breaks when
it returns false (replace the std::ranges::for_each usage with a manual loop or
a std::any_of/for loop that respects the boolean), and update
CreatureSearcher::operator() to return the appropriate boolean (true to
continue, false to stop) so maxCount actually stops traversal.
| if (action_set_ == ActionSet::SearchAndSelect) { | ||
| if (default_action_ == AdvancedFinderDefaultAction::SearchMap) { | ||
| search_map_button_->SetDefault(); | ||
| } else { | ||
| select_item_button_->SetDefault(); | ||
| } | ||
| } else if (selection == SearchMode::ClientIDs) { | ||
| uint16_t clientID = (uint16_t)client_id_spin->GetValue(); | ||
| for (ServerItemId id : g_item_definitions.findAllByClientId(clientID)) { | ||
| const auto item = g_item_definitions.get(id); | ||
| RAWBrush* raw_brush = item.editorData().raw_brush; | ||
| if (!raw_brush) { | ||
| continue; | ||
| } | ||
| } else if (ok_button_ != nullptr) { | ||
| ok_button_->SetDefault(); | ||
| } |
There was a problem hiding this comment.
Apply the stored last action when restoring the shared finder.
You persist state.last_action, but the default button is still chosen entirely from the constructor argument. Reopening the dialog never restores the user's previous "Search Map" vs "Select Item" preference.
💡 Suggested fix
+ if (persist_shared_state_) {
+ default_action_ = persisted_state_.last_action;
+ }
+
if (action_set_ == ActionSet::SearchAndSelect) {
if (default_action_ == AdvancedFinderDefaultAction::SearchMap) {
search_map_button_->SetDefault();
} else {Also applies to: 413-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/find_item_window.cpp` around lines 395 - 403, The dialog currently
ignores the persisted state.last_action when restoring; update the
restore/initialization logic to set default_action_ from state.last_action
before applying button defaults so the user's previous "SearchMap" vs
"SelectItem" choice is restored. Concretely, in the code paths that set button
defaults (the block using action_set_ == ActionSet::SearchAndSelect that calls
search_map_button_->SetDefault() or select_item_button_->SetDefault(), and the
similar block at the 413-417 location), assign default_action_ =
state.last_action (or read state.last_action into default_action_ or a local
used to choose the default) and then call SetDefault() on the matching widget;
ensure the other branch that sets ok_button_->SetDefault() is unchanged.
- source/app/settings.cpp — removed FIND_ITEM_MODE, JUMP_TO_ITEM_MODE, ADVANCED_ITEM_FINDER_FIND_BY, ADVANCED_ITEM_FINDER_LAST_ACTION settings - source/app/settings.h — removed corresponding Config enum entries for deleted settings - source/ui/find_item_window.cpp — removed last_action persistence logic from savePersistedState() - source/ui/find_item_window_model.cpp — removed last_action load/save, removed secondary_label assignments for items and creatures - source/ui/find_item_window_model.h — removed secondary_label field from AdvancedFinderCatalogRow, removed last_action from AdvancedFinderPersistedState - source/ui/find_item_window_views.cpp — removed <format> import [minor], deleted entire AdvancedFinderPreviewPanel class, renamed rowSummary to hoverSummary and switched to string concatenation - source/ui/find_item_window_views.h — removed AdvancedFinderPreviewPanel class declaration
Implemented pagination for search results to handle large result sets efficiently. Replaced wxListBox with custom NanoVG-based ResultCanvas for better performance and theming. Added SEARCH_RESULTS_LIMIT config (default 100000). Updated ItemSearcher/CreatureSearcher to track totalMatches and support offset-based paging. Added DestroySearchWindow() for proper cleanup. Search handlers now load results in pages with prev/next navigation.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
The new Search Tool provides a significant enhancement to the editor's capabilities, including creature search, pagination, and a much-improved result list. The move towards value-based row storage and better state management in settings is a positive highlight.
🔍 General Feedback
- The custom
NanoVGCanvasimplementation for search results is clean and provides a modern feel to the UI. - Pagination is a great addition, though the current per-page full map scan might be slow on exceptionally large maps. Consider caching search results in the future if performance becomes an issue.
- The C++23 upgrade is noted; ensure this doesn't impact CI or developer environments without a strong functional requirement for the new standard.
| [[nodiscard]] uint32_t searchResultsLimit() { | ||
| return static_cast<uint32_t>(std::max(100000, g_settings.getInteger(Config::SEARCH_RESULTS_LIMIT))); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 The use of std::max(100000, ...) for the search results limit is problematic. It prevents users from setting a limit lower than 100,000, effectively forcing a very large minimum. If the intention was to provide a default value or a maximum cap, this logic should be reconsidered.
If you want 100,000 to be the default when the setting is not present or invalid, you should handle it differently. If you want to cap it at 100,000, you should use std::min.
| [[nodiscard]] uint32_t searchResultsLimit() { | |
| return static_cast<uint32_t>(std::max(1, g_settings.getInteger(Config::SEARCH_RESULTS_LIMIT))); | |
| } |
| add_executable(rme ${rme_H} ${rme_SRC}) | ||
|
|
||
| set_target_properties(rme PROPERTIES CXX_STANDARD 20) | ||
| set_target_properties(rme PROPERTIES CXX_STANDARD 23) |
There was a problem hiding this comment.
🟡 Upgrading the C++ standard to C++23 is a major change. Unless there are specific C++23 features being utilized that aren't available in C++20, it's generally better to remain on a more widely supported standard like C++20 to ensure broader compatibility with developer environments and CI tools.
If there are C++23 features used that I missed, please disregard this comment. Otherwise, consider if C++20 is sufficient.
| set_target_properties(rme PROPERTIES CXX_STANDARD 23) | |
| set_target_properties(rme PROPERTIES CXX_STANDARD 20) |
| @@ -122,14 +125,16 @@ namespace { | |||
| case DatFlagTranslucent: | |||
| case DatFlagLyingCorpse: | |||
| case DatFlagAnimateAlways: | |||
There was a problem hiding this comment.
🟡 DatFlagFullGround was removed from the list of handled flags in the parser. If this flag is still used in Tibia .dat files (which it typically is in some versions), this could cause an "unknown flag" warning or regression for items that utilize it. If DatFlagFullTile is meant to replace it or if it's no longer relevant for the versions supported by this PR, please clarify. Otherwise, it should likely remain in the switch statement even if it's a no-op to avoid the default "unknown" case.
| case DatFlagAnimateAlways: | |
| case DatFlagFullGround: | |
| case DatFlagAnimateAlways: |
| } | ||
|
|
||
| if (rows_.empty() || total_count_ == 0) { | ||
| summary_label->SetLabel("Results: 0"); |
There was a problem hiding this comment.
🟡 The export functionality currently only exports results from the current page (stored in rows_). If there are more results available (indicated by total_count_), the user might expect all of them to be exported.
Consider either:
- Warning the user that only the current page is being exported.
- Implementing a full export that performs a dedicated map scan to find and write all matches directly to the file.
for (size_t i = 0; i < count; ++i) {
const SearchResultRow& row = rows_[i];
file.Write(wxString::Format("%u\t%s\t%d\t%d\t%d\n", row.index, row.name, row.position.x, row.position.y, row.position.z));- source/editor/operations/search_operations.h — removed limitReached() method from ItemSearchOperation and CreatureSearchOperation structs - source/ui/gui.h — removed HideSearchWindow() wrapper method - source/ui/managers/search_manager.cpp — removed HideSearchWindow() implementation - source/ui/managers/search_manager.h — removed HideSearchWindow() declaration from public API - source/ui/menubar/navigation_menu_handler.cpp — removed unused dialog_util.h include [minor]
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
source/ui/menubar/navigation_menu_handler.cpp (1)
151-159:⚠️ Potential issue | 🟠 MajorBlock
SearchMapwhen no editor is open.This handler still exposes the map-search flow even though the
SearchMapbranch immediately walksg_gui.GetCurrentMap(). With no editor open, the dialog can still returnSearchMapand then dereference invalid editor/map state.💡 Suggested fix
- FindItemDialog dialog(frame, "Jump to Item", false, FindItemDialog::ActionSet::SearchAndSelect, AdvancedFinderDefaultAction::SelectItem, true); + const auto action_set = g_gui.IsEditorOpen() + ? FindItemDialog::ActionSet::SearchAndSelect + : FindItemDialog::ActionSet::ConfirmOnly; + FindItemDialog dialog(frame, "Jump to Item", false, action_set, AdvancedFinderDefaultAction::SelectItem, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/menubar/navigation_menu_handler.cpp` around lines 151 - 159, The SearchMap branch can run when no editor/map exists; before handling FindItemDialog::ResultAction::SearchMap (after dialog.ShowModal() and dialog.getResultAction()), check that an editor/map is open via g_gui.GetCurrentMap() (or the existing editor-check helper) and early-return or treat as cancelled if it is null; only call showCreatureSearchResults or showItemSearchResults when the map/editor is valid to avoid dereferencing invalid state.
🧹 Nitpick comments (5)
source/ui/find_item_window_views.cpp (1)
576-583:const_castused to call non-const base class method.The
drawSpriteBadgemethod isconstbut needs to callGetOrCreateSpriteTexturewhich modifies texture cache state. This is acceptable given the caching semantics, but could be documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/find_item_window_views.cpp` around lines 576 - 583, The const_cast in drawSpriteBadge calling GetOrCreateSpriteTexture on AdvancedFinderResultsView should be justified or eliminated: either change the texture cache members used by GetOrCreateSpriteTexture to be mutable and keep GetOrCreateSpriteTexture logically const, or if leaving the method non-const, add a clear comment immediately above the const_cast explaining why mutation is safe (texture cache only, idempotent, thread-affinity, etc.) and reference GetOrCreateSpriteTexture and AdvancedFinderResultsView; ensure the comment documents the caching semantics and why const correctness is relaxed.source/ui/result_window.h (1)
38-38: Consider documenting thepage_loadercallback contract.The
SetResultssignature accepts apage_loadercallback but its expected behavior (called with new offset when pagination buttons are clicked) isn't documented. A brief comment would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/result_window.h` at line 38, Document the page_loader parameter on the SetResults declaration: explain that page_loader is an optional callback invoked when pagination controls are used, it receives the new page_offset (uint32_t) to request more SearchResultRow items, is allowed to be empty (default {}), and should perform loading/updating of results (e.g., call SetResults again) when the user requests a different page; add this brief comment above the SetResults prototype so maintainers understand the contract.source/ui/find_item_window_model.cpp (1)
354-383: Dynamic cast per item definition is expensive.Line 356 performs a
dynamic_casttoGameSprite*for every item definition when building the catalog. This is O(n) casts for n items.💡 Consider caching or avoiding dynamic_cast
If the sprite type is known at retrieval time, consider providing a typed accessor in the graphics system to avoid runtime type checking for every item.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/find_item_window_model.cpp` around lines 354 - 383, The buildVisualMask function does a dynamic_cast<GameSprite*> via g_gui.gfx.getSprite(definition.clientId()) for every item which is expensive; change the graphics API or usage to avoid per-item dynamic_casts by exposing a typed accessor or caching sprite metadata: add or use a getGameSprite(clientId) or a boolean/flag-based query on g_gui.gfx that returns either a GameSprite* or lightweight sprite properties, then update buildVisualMask to call that typed accessor once (or read cached metadata) instead of dynamic_casting each time, keeping checks for hasLight(), animator/frames, and other property queries against the new accessor (referencing buildVisualMask, GameSprite, g_gui.gfx.getSprite, and AdvancedFinderVisualFilter).source/ui/find_item_window.cpp (1)
543-551:const_castto callLayout()is a code smell.Using
const_cast<FindItemDialog*>(this)->Layout()in aconstmethod indicates the method shouldn't beconst, orLayout()should be called elsewhere.♻️ Consider making the method non-const
-void FindItemDialog::updateResultTitle(size_t count) const { +void FindItemDialog::updateResultTitle(size_t count) { if (result_count_label_ != nullptr) { result_count_label_->SetLabel(wxString::Format("Results: %zu |", count)); result_count_label_->SetMinSize(result_count_label_->GetBestSize()); if (wxWindow* parent = result_count_label_->GetParent(); parent != nullptr) { parent->Layout(); } - const_cast<FindItemDialog*>(this)->Layout(); + Layout(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/find_item_window.cpp` around lines 543 - 551, updateResultTitle currently uses const_cast to call Layout() which indicates the method should not be const; change the method to be non-const (remove the trailing const from FindItemDialog::updateResultTitle in both declaration and definition), remove the const_cast<FindItemDialog*>(this)->Layout() call and call this->Layout() directly, and update any callers or references as needed so the method compiles without casting; ensure the existing result_count_label_ parent Layout() call remains unchanged.source/ui/result_window.cpp (1)
392-405: Potential integer overflow in progress bar callback.The
SetLoadScalecall on line 400 castssize_tvalues toint32_t. For very large result sets (approachingSEARCH_RESULTS_LIMITof 100,000), this is fine, but if the limit is increased significantly in the future, this could overflow.💡 Consider using safer casts
- g_gui.SetLoadScale(static_cast<int32_t>(i), static_cast<int32_t>(count)); + g_gui.SetLoadScale(static_cast<int32_t>(std::min(i, static_cast<size_t>(INT32_MAX))), + static_cast<int32_t>(std::min(count, static_cast<size_t>(INT32_MAX))));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/result_window.cpp` around lines 392 - 405, The progress scaling currently casts size_t to int32_t in the export loop (rows_, count -> g_gui.SetLoadScale(static_cast<int32_t>(i), static_cast<int32_t>(count))), which can overflow if result limits grow; fix by using a wider integer or clamping: either change SetLoadScale to accept a 64-bit type (e.g. int64_t) and pass i and count as int64_t, or compute safe int values by converting to int64_t and then clamping to INT32_MAX (or the GUI max) before calling SetLoadScale; update all references to SetLoadScale and the loop variables (i, count, rows_) accordingly to ensure no narrowing cast from size_t to int32_t occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/ui/managers/search_manager.cpp`:
- Around line 12-14: The SearchManager destructor (~SearchManager) must not call
DestroySearchWindow because that function touches g_gui.aui_manager and global
destruction order between g_search and g_gui is unspecified; remove the call
from ~SearchManager (make the destructor a no-op) and rely on the existing
explicit call to g_search.DestroySearchWindow() in MainFrame::OnExit to perform
cleanup at application shutdown; ensure DestroySearchWindow remains available
for explicit teardown and consider leaving a comment in ~SearchManager
referencing MainFrame::OnExit for future maintainers.
In `@source/ui/menubar/navigation_menu_handler.cpp`:
- Around line 37-59: The pagination callback is re-querying
g_gui.GetCurrentMap() at click time, so if the user switches or closes the map
the paging runs against the wrong map; modify showItemSearchResults (and the
analogous showCreatureSearchResults at lines 62-84) to capture and use the
specific map instance (or map ID) used for the initial search: add a parameter
for the map pointer/ID to the function signature, construct
EditorOperations::ItemSearcher and call foreach_ItemOnMap with that captured map
instead of g_gui.GetCurrentMap(), and change the lambda passed into
window->SetResults to capture the map by value and call the updated
showItemSearchResults(map, item_id, load_bar_label, next_offset) so paging
always queries the original map.
- Around line 18-20: searchResultsLimit currently forces a 100000 minimum by
using std::max(100000, g_settings.getInteger(Config::SEARCH_RESULTS_LIMIT)),
preventing smaller configured page sizes; change the logic in searchResultsLimit
to clamp the configured value into a valid range (for example using std::clamp)
so the returned uint32_t is bounded between a sensible minimum (e.g. 1) and the
maximum 100000, and use g_settings.getInteger(Config::SEARCH_RESULTS_LIMIT) as
the source value so smaller configured limits are respected.
In `@source/ui/menubar/search_handler.cpp`:
- Around line 277-283: The call to result->SetResults(std::move(rows),
static_cast<uint32_t>(rows.size())) uses rows.size() after rows has been
moved-from (undefined); change it to pass the known size from found (or capture
size before moving). Specifically, in the block that builds rows using
makeSearchResultRow and found, replace the second argument to SetResults with
static_cast<uint32_t>(found.size()) (or store auto n = rows.size(); and use n)
so you don't query rows after std::move; keep the call to
result->SetResults(std::move(rows), ... ) otherwise.
---
Duplicate comments:
In `@source/ui/menubar/navigation_menu_handler.cpp`:
- Around line 151-159: The SearchMap branch can run when no editor/map exists;
before handling FindItemDialog::ResultAction::SearchMap (after
dialog.ShowModal() and dialog.getResultAction()), check that an editor/map is
open via g_gui.GetCurrentMap() (or the existing editor-check helper) and
early-return or treat as cancelled if it is null; only call
showCreatureSearchResults or showItemSearchResults when the map/editor is valid
to avoid dereferencing invalid state.
---
Nitpick comments:
In `@source/ui/find_item_window_model.cpp`:
- Around line 354-383: The buildVisualMask function does a
dynamic_cast<GameSprite*> via g_gui.gfx.getSprite(definition.clientId()) for
every item which is expensive; change the graphics API or usage to avoid
per-item dynamic_casts by exposing a typed accessor or caching sprite metadata:
add or use a getGameSprite(clientId) or a boolean/flag-based query on g_gui.gfx
that returns either a GameSprite* or lightweight sprite properties, then update
buildVisualMask to call that typed accessor once (or read cached metadata)
instead of dynamic_casting each time, keeping checks for hasLight(),
animator/frames, and other property queries against the new accessor
(referencing buildVisualMask, GameSprite, g_gui.gfx.getSprite, and
AdvancedFinderVisualFilter).
In `@source/ui/find_item_window_views.cpp`:
- Around line 576-583: The const_cast in drawSpriteBadge calling
GetOrCreateSpriteTexture on AdvancedFinderResultsView should be justified or
eliminated: either change the texture cache members used by
GetOrCreateSpriteTexture to be mutable and keep GetOrCreateSpriteTexture
logically const, or if leaving the method non-const, add a clear comment
immediately above the const_cast explaining why mutation is safe (texture cache
only, idempotent, thread-affinity, etc.) and reference GetOrCreateSpriteTexture
and AdvancedFinderResultsView; ensure the comment documents the caching
semantics and why const correctness is relaxed.
In `@source/ui/find_item_window.cpp`:
- Around line 543-551: updateResultTitle currently uses const_cast to call
Layout() which indicates the method should not be const; change the method to be
non-const (remove the trailing const from FindItemDialog::updateResultTitle in
both declaration and definition), remove the
const_cast<FindItemDialog*>(this)->Layout() call and call this->Layout()
directly, and update any callers or references as needed so the method compiles
without casting; ensure the existing result_count_label_ parent Layout() call
remains unchanged.
In `@source/ui/result_window.cpp`:
- Around line 392-405: The progress scaling currently casts size_t to int32_t in
the export loop (rows_, count -> g_gui.SetLoadScale(static_cast<int32_t>(i),
static_cast<int32_t>(count))), which can overflow if result limits grow; fix by
using a wider integer or clamping: either change SetLoadScale to accept a 64-bit
type (e.g. int64_t) and pass i and count as int64_t, or compute safe int values
by converting to int64_t and then clamping to INT32_MAX (or the GUI max) before
calling SetLoadScale; update all references to SetLoadScale and the loop
variables (i, count, rows_) accordingly to ensure no narrowing cast from size_t
to int32_t occurs.
In `@source/ui/result_window.h`:
- Line 38: Document the page_loader parameter on the SetResults declaration:
explain that page_loader is an optional callback invoked when pagination
controls are used, it receives the new page_offset (uint32_t) to request more
SearchResultRow items, is allowed to be empty (default {}), and should perform
loading/updating of results (e.g., call SetResults again) when the user requests
a different page; add this brief comment above the SetResults prototype so
maintainers understand the contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37185da5-742c-47c4-a830-286d82a2cf8d
📒 Files selected for processing (16)
source/app/settings.cppsource/app/settings.hsource/editor/operations/search_operations.hsource/ui/find_item_window.cppsource/ui/find_item_window_model.cppsource/ui/find_item_window_model.hsource/ui/find_item_window_views.cppsource/ui/find_item_window_views.hsource/ui/gui.hsource/ui/main_frame.cppsource/ui/managers/search_manager.cppsource/ui/managers/search_manager.hsource/ui/menubar/navigation_menu_handler.cppsource/ui/menubar/search_handler.cppsource/ui/result_window.cppsource/ui/result_window.h
🚧 Files skipped from review as they are similar to previous changes (2)
- source/ui/find_item_window_views.h
- source/editor/operations/search_operations.h
There was a problem hiding this comment.
This Pull Request introduces a significantly improved Advanced Item Finder and creature search functionality, including a complete overhaul of the search results UI using custom NanoVG rendering and pagination. The new features provide much-needed filtering capabilities and a more responsive, visually rich experience for map editing.
🔍 General Feedback
- New Advanced Finder: The implementation of the Advanced Item Finder is very comprehensive, offering a wide range of filters (types, properties, interactions, visuals) and both List and Grid views.
- Improved Performance: Replacing the standard
wxListBoxwith a custom NanoVG-based canvas for search results and the finder dialog significantly improves UI responsiveness when handling large result sets. - Pagination Support: The addition of pagination for map-wide search results is a great improvement for scalability and usability.
- Consistency: The move to C++23 allows for cleaner code (e.g., using
std::to_underlying), and the developer has maintained good consistency with existing architectural patterns. - Shared Utilities: There is some code duplication between
search_handler.cppandnavigation_menu_handler.cppfor search-related helper functions which should be refactored into a common utility.
| void SearchHandler::OnSearchForItem(wxCommandEvent& WXUNUSED(event)) { | ||
| if (!g_gui.IsEditorOpen()) { | ||
| return; | ||
| namespace { |
There was a problem hiding this comment.
🟠 High. The searchResultsLimit() function enforces a minimum limit of 100,000, which ignores the user setting if it is set to a lower value. This can lead to performance issues when searching large maps if a smaller limit was intended.
| namespace { | |
| [[nodiscard]] uint32_t searchResultsLimit() { | |
| return static_cast<uint32_t>(std::max(1, g_settings.getInteger(Config::SEARCH_RESULTS_LIMIT))); | |
| } |
| } | ||
|
|
||
| void AdvancedFinderResultsView::OnNanoVGPaint(NVGcontext* vg, int width, int height) { | ||
| updateLayoutMetrics(width); |
There was a problem hiding this comment.
🟡 Medium. OnNanoVGPaint calls updateLayoutMetrics (which calls UpdateScrollbar and SetScrollStep) every paint cycle. This is inefficient as layout metrics and scrollbar configuration should only be updated when the window size or data changes (which is already handled in onSize, SetRows, etc.).
| updateLayoutMetrics(width); | |
| void AdvancedFinderResultsView::OnNanoVGPaint(NVGcontext* vg, int width, int height) { | |
| if (cached_width_ != width || cached_height_ != height) { | |
| updateLayoutMetrics(width); | |
| } | |
| if (empty_state_ != EmptyState::Rows || rows_.empty()) { |
| @@ -13,41 +13,126 @@ | |||
| #include "map/map.h" | |||
There was a problem hiding this comment.
🟡 Medium. The search-related helper functions (searchResultsLimit, makeSearchResultRow, showItemSearchResults, showCreatureSearchResults) are duplicated between search_handler.cpp and navigation_menu_handler.cpp. This duplication makes the code harder to maintain and prone to inconsistencies. Consider moving these shared utilities to a common location.
… settings Refactored FindItemDialog by extracting layout code to find_item_window_layout.cpp and painting logic to find_item_window_views_paint.cpp, with internal helpers moved to find_item_window_internal.h. Added unsigned integer support to Settings for filter masks, replacing signed integers. Fixed search pagination to use correct map instance and clamp limits properly. Added prefix wildcard matching to item search scoring.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
source/ui/find_item_window.cpp (1)
73-75: Minor: CacheactiveResultsView()result to avoid redundant call.
activeResultsView()is called twice on line 74. While it's a simple getter, caching the result improves clarity.♻️ Suggested improvement
void FindItemDialog::updateButtons() { - const bool has_selection = activeResultsView() != nullptr && activeResultsView()->GetSelectedRow() != nullptr; + const auto* view = activeResultsView(); + const bool has_selection = view != nullptr && view->GetSelectedRow() != nullptr; if (search_map_button_ != nullptr) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/find_item_window.cpp` around lines 73 - 75, In updateButtons(), activeResultsView() is called twice; cache its result in a local variable (e.g., auto* view = activeResultsView()) and use view for the null-check and the subsequent GetSelectedRow() call so has_selection becomes something like view != nullptr && view->GetSelectedRow() != nullptr; this avoids the redundant getter call and improves clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/app/settings.cpp`:
- Around line 72-84: The bounds check in Settings::getUnsignedInteger is too
loose and allows key == Config::LAST to index past the end of store; change the
guard from "key > Config::LAST" to "key >= Config::LAST" (i.e. ensure key is
strictly less than Config::LAST before accessing store[key]) and make the same
correction in the other unsigned accessor(s) referenced (the unsigned accessors
around lines 118-126) so all checks use >= against Config::LAST to prevent
out-of-range indexing of store.
In `@source/ui/find_item_window_views_paint.cpp`:
- Around line 18-23: List-mode rendering prints row.server_id and row.client_id
for creature rows, producing misleading 0 values; update the list rendering
logic that formats rows (the same place where hoverSummary is used) to check
AdvancedFinderCatalogRow::isCreature() and, when true, render "SID: - CID: -
" (or use hoverSummary's output) instead of printing row.server_id and
row.client_id; locate the code that formats/display rows (references:
hoverSummary, AdvancedFinderCatalogRow, row.isCreature(), row.server_id,
row.client_id) and add the conditional branch so creatures show dashes rather
than numeric IDs.
In `@source/ui/result_window.cpp`:
- Around line 374-380: SearchResultWindow::activateRow currently recenters the
current tab regardless of the result origin; attach the originating Map*/tab
identifier to the result set/rows_ (the paged loaders in search_handler.cpp
already know the Map*) and, inside SearchResultWindow::activateRow, first
switch/reactivate that stored map/tab before calling
g_gui.SetScreenCenterPosition(rows_[index].position); update the code paths that
construct rows_ to populate the new source map/tab field and use the app's
existing API to activate that map/tab prior to centering so the recenter happens
on the correct map.
- Around line 314-330: The footer buttons (previous_page_button_,
next_page_button_, export_button_, clear_button_) are bitmap-only and lack
accessible names for screen readers; update their creation to provide
accessibility by calling SetToolTip() and/or SetName() (or SetLabel()) with
descriptive text for each button (e.g., "Previous page", "Next page", "Export
current page", "Clear results"), and ensure previous_page_button_ and
next_page_button_ at minimum get tooltips or names as a fallback so assistive
technology can expose them.
---
Nitpick comments:
In `@source/ui/find_item_window.cpp`:
- Around line 73-75: In updateButtons(), activeResultsView() is called twice;
cache its result in a local variable (e.g., auto* view = activeResultsView())
and use view for the null-check and the subsequent GetSelectedRow() call so
has_selection becomes something like view != nullptr && view->GetSelectedRow()
!= nullptr; this avoids the redundant getter call and improves clarity.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f60b033-d3e6-4275-a020-8f0b63b5b1c5
📒 Files selected for processing (19)
source/CMakeLists.txtsource/app/settings.cppsource/app/settings.hsource/editor/operations/search_operations.hsource/ui/find_item_window.cppsource/ui/find_item_window.hsource/ui/find_item_window_internal.hsource/ui/find_item_window_layout.cppsource/ui/find_item_window_model.cppsource/ui/find_item_window_model.hsource/ui/find_item_window_views.cppsource/ui/find_item_window_views_paint.cppsource/ui/gui.hsource/ui/managers/search_manager.cppsource/ui/managers/search_manager.hsource/ui/menubar/navigation_menu_handler.cppsource/ui/menubar/search_handler.cppsource/ui/result_window.cppsource/ui/result_window.h
✅ Files skipped from review due to trivial changes (1)
- source/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- source/ui/managers/search_manager.h
- source/ui/gui.h
- source/ui/menubar/navigation_menu_handler.cpp
- source/ui/managers/search_manager.cpp
- source/ui/find_item_window.h
- source/ui/find_item_window_model.h
| uint32_t Settings::getUnsignedInteger(uint32_t key) const { | ||
| if (key > Config::LAST) { | ||
| return 0; | ||
| } | ||
| const DynamicValue& dv = store[key]; | ||
| if (auto val = std::get_if<uint32_t>(&dv.val)) { | ||
| return *val; | ||
| } | ||
| if (auto val = std::get_if<int>(&dv.val)) { | ||
| return static_cast<uint32_t>(std::max(0, *val)); | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Tighten the bounds check in the new unsigned accessors.
store is sized with Config::LAST, so key == Config::LAST is already out of range. These guards still fall through and index past the vector end.
Suggested fix
uint32_t Settings::getUnsignedInteger(uint32_t key) const {
- if (key > Config::LAST) {
+ if (key >= Config::LAST) {
return 0;
}
...
}
void Settings::setUnsignedInteger(uint32_t key, uint32_t newval) {
- if (key > Config::LAST) {
+ if (key >= Config::LAST) {
return;
}
...
}Also applies to: 118-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/app/settings.cpp` around lines 72 - 84, The bounds check in
Settings::getUnsignedInteger is too loose and allows key == Config::LAST to
index past the end of store; change the guard from "key > Config::LAST" to "key
>= Config::LAST" (i.e. ensure key is strictly less than Config::LAST before
accessing store[key]) and make the same correction in the other unsigned
accessor(s) referenced (the unsigned accessors around lines 118-126) so all
checks use >= against Config::LAST to prevent out-of-range indexing of store.
| [[nodiscard]] std::string hoverSummary(const AdvancedFinderCatalogRow& row) { | ||
| if (row.isCreature()) { | ||
| return "SID: - CID: - " + row.label; | ||
| } | ||
| return "SID: " + std::to_string(row.server_id) + " CID: " + std::to_string(row.client_id) + " " + row.label; | ||
| } |
There was a problem hiding this comment.
Render creature rows without bogus SID/CID values.
hoverSummary() already treats creatures as having no SID/CID, but list mode always prints row.server_id and row.client_id. Creature results will show misleading 0 values here.
Suggested fix
+ const std::string sid_text = row.isCreature() ? "-" : std::to_string(row.server_id);
+ const std::string cid_text = row.isCreature() ? "-" : std::to_string(row.client_id);
+
nvgSave(vg);
nvgScissor(vg, column_image + 6.0f, rect.y, rect.width - column_image - 12.0f, rect.height);
- nvgText(vg, column_image + 12.0f, text_y, std::to_string(row.server_id).c_str(), nullptr);
- nvgText(vg, column_image + column_sid + 12.0f, text_y, std::to_string(row.client_id).c_str(), nullptr);
+ nvgText(vg, column_image + 12.0f, text_y, sid_text.c_str(), nullptr);
+ nvgText(vg, column_image + column_sid + 12.0f, text_y, cid_text.c_str(), nullptr);
nvgText(vg, column_image + column_sid + column_cid + 12.0f, text_y, row.label.c_str(), nullptr);
nvgRestore(vg);Also applies to: 221-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/find_item_window_views_paint.cpp` around lines 18 - 23, List-mode
rendering prints row.server_id and row.client_id for creature rows, producing
misleading 0 values; update the list rendering logic that formats rows (the same
place where hoverSummary is used) to check
AdvancedFinderCatalogRow::isCreature() and, when true, render "SID: - CID: -
" (or use hoverSummary's output) instead of printing row.server_id and
row.client_id; locate the code that formats/display rows (references:
hoverSummary, AdvancedFinderCatalogRow, row.isCreature(), row.server_id,
row.client_id) and add the conditional branch so creatures show dashes rather
than numeric IDs.
| previous_page_button_ = newd wxButton(this, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT); | ||
| previous_page_button_->SetBitmap(IMAGE_MANAGER.GetBitmapBundle(ICON_ARROW_LEFT)); | ||
| action_sizer->Add(previous_page_button_, wxSizerFlags(0).CenterVertical().Border(wxRIGHT, FromDIP(6))); | ||
|
|
||
| next_page_button_ = newd wxButton(this, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT); | ||
| next_page_button_->SetBitmap(IMAGE_MANAGER.GetBitmapBundle(ICON_ARROW_RIGHT)); | ||
| action_sizer->Add(next_page_button_, wxSizerFlags(0).CenterVertical().Border(wxRIGHT, FromDIP(12))); | ||
|
|
||
| export_button_ = newd wxButton(this, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT); | ||
| export_button_->SetBitmap(IMAGE_MANAGER.GetBitmapBundle(ICON_FILE_EXPORT)); | ||
| export_button_->SetToolTip("Export current page"); | ||
| action_sizer->Add(export_button_, wxSizerFlags(0).CenterVertical().Border(wxRIGHT, FromDIP(6))); | ||
|
|
||
| clear_button_ = newd wxButton(this, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT); | ||
| clear_button_->SetBitmap(IMAGE_MANAGER.GetBitmapBundle(ICON_TRASH_CAN)); | ||
| clear_button_->SetToolTip("Clear results"); | ||
| action_sizer->Add(clear_button_, wxSizerFlags(0).CenterVertical()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the exact code at the specified lines
cat -n source/ui/result_window.cpp | sed -n '314,330p'Repository: karolak6612/remeres-map-editor-redux
Length of output: 1447
🏁 Script executed:
# Let's check if there's any accessibility-related setup in the file
rg -A 5 -B 5 "SetToolTip|SetAccessible|SetName" source/ui/result_window.cpp | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 1044
🏁 Script executed:
# Check wxButton usage patterns in the codebase to see if buttons typically get labels
rg "wxButton.*wxEmptyString" source/ -A 2 | head -30Repository: karolak6612/remeres-map-editor-redux
Length of output: 1824
🏁 Script executed:
# Look for any accessibility setup or configuration in the project
fd -t f -e cpp -e h source/ | xargs rg -l "accessible|Accessible|ARIA|a11y" 2>/dev/null | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 537
🏁 Script executed:
# Search for SetName() and SetAccessible() usage on buttons
rg "SetName\(|SetAccessible\(" source/ui/ -B 2 -A 2 | head -40Repository: karolak6612/remeres-map-editor-redux
Length of output: 1531
🏁 Script executed:
# Check if there's any accessibility documentation or standards in the project
find source -name "*.md" -o -name "README*" | xargs grep -l "access" 2>/dev/nullRepository: karolak6612/remeres-map-editor-redux
Length of output: 101
🏁 Script executed:
# Look at the position_toolbar example more closely to understand the pattern
cat -n source/ui/toolbar/position_toolbar.cpp | sed -n '1,50p' | grep -A 10 "go_button"Repository: karolak6612/remeres-map-editor-redux
Length of output: 1085
Give bitmap-only footer buttons accessible names.
All four buttons are created with empty labels, and the pagination buttons (previous_page_button_ and next_page_button_) have no tooltip fallback, making them inaccessible to screen reader users. Add either a visible label, explicit tooltip, or accessible name (via SetName() or similar wxWidgets accessibility API) for each button to ensure they're properly exposed to assistive technology.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/result_window.cpp` around lines 314 - 330, The footer buttons
(previous_page_button_, next_page_button_, export_button_, clear_button_) are
bitmap-only and lack accessible names for screen readers; update their creation
to provide accessibility by calling SetToolTip() and/or SetName() (or
SetLabel()) with descriptive text for each button (e.g., "Previous page", "Next
page", "Export current page", "Clear results"), and ensure previous_page_button_
and next_page_button_ at minimum get tooltips or names as a fallback so
assistive technology can expose them.
| void SearchResultWindow::activateRow(int index) { | ||
| if (index < 0 || index >= static_cast<int>(rows_.size())) { | ||
| return; | ||
| } | ||
|
|
||
| g_gui.SetScreenCenterPosition(rows_[static_cast<size_t>(index)].position); | ||
| } |
There was a problem hiding this comment.
Keep result activation bound to the map that produced the page.
The paged loaders in source/ui/menubar/search_handler.cpp are already tied to the originating Map*, but activateRow() recenters the current tab via g_gui.SetScreenCenterPosition(). If the user switches tabs before clicking a row, navigation jumps on the wrong map. Store the source map/tab with the result set and reactivate that tab before centering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/result_window.cpp` around lines 374 - 380,
SearchResultWindow::activateRow currently recenters the current tab regardless
of the result origin; attach the originating Map*/tab identifier to the result
set/rows_ (the paged loaders in search_handler.cpp already know the Map*) and,
inside SearchResultWindow::activateRow, first switch/reactivate that stored
map/tab before calling g_gui.SetScreenCenterPosition(rows_[index].position);
update the code paths that construct rows_ to populate the new source map/tab
field and use the app's existing API to activate that map/tab prior to centering
so the recenter happens on the correct map.
1 participant
Summary by CodeRabbit