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
Extracted map property editing logic into a reusable MapPropertiesPanel to unify UI patterns with NewMapPanel and added client version registry support. Improved dialog layout by using child containers for better space management and added support for escaping the dialog.
ImguiMapEditor/Application/CallbackMediator.cpp — injected version registry into map_properties dialog
ImguiMapEditor/CMakeLists.txt — added MapPropertiesPanel.cpp to build
ImguiMapEditor/UI/Dialogs/NewMapDialog.cpp — improved modal layout using BeginChild and added Escape key handler
ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp — refactored to use MapPropertiesPanel and added support for disabling on SEC maps
ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.h — updated to use MapPropertiesPanel component
ImguiMapEditor/UI/Panels/MapPropertiesPanel.cpp — implemented new panel for rendering and managing map properties
ImguiMapEditor/UI/Panels/MapPropertiesPanel.h — defined interface for MapPropertiesPanel
ImguiMapEditor/UI/Panels/NewMapPanel.cpp — updated child container borders and height configuration
Summary by CodeRabbit
New Features
Redesigned Map Properties UI with editable description, house/spawn fields, and clearer metadata display; unsupported map types show a clear message.
Bug Fixes
Map dialogs have improved sizing/constraints, consistent footer visibility, and Escape now closes dialogs reliably.
Refactor
Map Properties dialog now uses a dedicated panel/state model for more reliable editing and apply/OK behavior.
Chores
Build updated to include the new Map Properties panel; minor layout polish in the New Map panel.
Extracted map property editing logic into a reusable `MapPropertiesPanel` to unify UI patterns with `NewMapPanel` and added client version registry support. Improved dialog layout by using child containers for better space management and added support for escaping the dialog.
- `ImguiMapEditor/Application/CallbackMediator.cpp` — injected version registry into `map_properties` dialog
- `ImguiMapEditor/CMakeLists.txt` — added `MapPropertiesPanel.cpp` to build
- `ImguiMapEditor/UI/Dialogs/NewMapDialog.cpp` — improved modal layout using `BeginChild` and added Escape key handler
- `ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp` — refactored to use `MapPropertiesPanel` and added support for disabling on SEC maps
- `ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.h` — updated to use `MapPropertiesPanel` component
- `ImguiMapEditor/UI/Panels/MapPropertiesPanel.cpp` — implemented new panel for rendering and managing map properties
- `ImguiMapEditor/UI/Panels/MapPropertiesPanel.h` — defined interface for `MapPropertiesPanel`
- `ImguiMapEditor/UI/Panels/NewMapPanel.cpp` — updated child container borders and height configuration
Hello, 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 refactors the map property editing logic by introducing a dedicated MapPropertiesPanel. This change standardizes the UI across the editor, improves space management within dialogs, and adds necessary support for client versioning and better keyboard interaction.
Highlights
Refactoring: Extracted map property editing logic into a new reusable MapPropertiesPanel component to unify UI patterns across the application.
UI Improvements: Enhanced the Map Properties and New Map dialogs with improved layout management using child containers and added support for closing dialogs via the Escape key.
Feature Enhancement: Integrated client version registry support into the map properties dialog and added a safeguard to disable property editing for SEC maps.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
Feature
Command
Description
Code Review
/gemini review
Performs a code review for the current pull request in its current state.
Pull Request Summary
/gemini summary
Provides a summary of the current pull request in its current state.
Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help
/gemini help
Displays a list of available commands.
Customization
To customize the 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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.
Reviewing files that changed from the base of the PR and between e7631cd and 6a79be9.
📒 Files selected for processing (2)
ImguiMapEditor/UI/Panels/MapPropertiesPanel.cpp
ImguiMapEditor/UI/Panels/NewMapPanel.cpp
Walkthrough
This pull request refactors map-properties UI handling by extracting it into a reusable MapPropertiesPanel component. The new panel manages map metadata state (name, versions, dimensions, description, file paths) via a State struct and exposes initialization, loading, and rendering methods. MapPropertiesDialog is refactored to delegate UI/state management to this panel instead of maintaining internal buffers. The pull request also improves NewMapDialog layout structure with proper sizing constraints and footer separation, adjusts NewMapPanel sizing to use available space dynamically, integrates the new panel into the build system, and wires initialization through the callback mediator.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 10.00% 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 accurately summarizes the main change: extracting map properties dialog logic into a reusable panel component, which aligns with the core refactoring objective throughout the changeset.
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 map properties dialog to delegate property rendering to a new reusable MapPropertiesPanel (shared layout patterns with NewMapPanel), injects the client version registry for richer version display, and unifies modal layouts (child containers, fixed button footer, Escape handling) in the New Map and Map Properties dialogs while adding SEC-map restrictions.
File-Level Changes
Change
Details
Files
Introduce reusable MapPropertiesPanel component and wire it into MapPropertiesDialog with client version registry support.
Add MapPropertiesPanel class with State model, loadFromMap helper, and render method that presents read-only metadata and editable external file paths and description.
Update MapPropertiesDialog to hold a MapPropertiesPanel and State, initialize it with ClientVersionRegistry, and delegate loading and rendering of map properties to the panel.
Change MapPropertiesDialog::applyToMap to write description and external file paths from panel state back to the map instead of managing raw buffers and dimensions directly.
Align Map Properties and New Map dialogs with shared modal layout patterns and improved UX (child content regions, footer, Escape handling, theming).
Set explicit window sizes and constraints for MapPropertiesDialog using Config::UI dimensions and remove NoResize flag in both dialogs to allow constraints-based sizing.
Wrap dialog body content (tabs/panels) in BeginChild/EndChild regions sized to remaining vertical space, leaving a fixed-height footer with centered action buttons.
Add Escape key handling to both dialogs to cancel/close, and apply semantic colors and themed OK/Close buttons in MapPropertiesDialog.
Update NewMapPanel child panels to use full available height and draw ImGui child borders for clearer column separation.
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.
📋 Review Summary
The PR refactors the map properties UI logic into a reusable MapPropertiesPanel, which improves code modularity and aligns it with the NewMapPanel pattern. The addition of SEC map validation and the Escape key handler are great UX improvements.
🔍 General Feedback
Regression: The most critical issue is the loss of map resizing functionality. The UI fields for Width/Height are now disabled, and the code that applies these changes to the map has been removed.
C++20 Dependency: The use of std::format introduces a dependency on C++20. Please confirm if the project has officially moved to C++20.
Version Matching: Double-check the argument order in registry_->findBestMatch. It currently passes (minor, major), which is unconventional.
Overall, the refactoring is clean, but the regression in functionality should be addressed.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Regression: The map resizing functionality has been removed. In the previous implementation, map_->setSize was called in applyToMap. In the refactored version, the size fields are read-only in the UI and the call to setSize is missing.
If resizing is intended to be supported, these fields should be enabled in MapPropertiesPanel and the values applied here.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Compatibility: std::format is a C++20 feature. If the project is targeting an older standard (like C++17), this will cause build failures. Please ensure the compiler flags are updated to C++20 or use std::string concatenation / std::stringstream / fmt library if C++20 is not yet adopted.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The map size fields are marked as read-only here (BeginDisabled). As noted in the dialog's applyToMap, this prevents users from resizing existing maps, which was possible before this refactor.
The reason will be displayed to describe this comment to others. Learn more.
🟡 FLT_MAX requires <cfloat> or <float.h>. Consider using std::numeric_limits<float>::max() from <limits> for a more modern C++ approach, or ensure the header is included.
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the map properties dialog by introducing a reusable MapPropertiesPanel to handle the rendering and state management of map metadata, and updates the layout of the new map dialog to use child windows. Feedback on the changes highlights two key improvements in MapPropertiesPanel.cpp: first, avoiding string copies of the map description inside the render loop to prevent heap allocations, which violates the project's no-allocation zone guidelines; and second, ensuring a fallback client version name is set when the version registry is null to prevent empty fields in the UI.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The reason will be displayed to describe this comment to others. Learn more.
Avoid copying state.description on every frame to check for changes. This introduces unnecessary heap allocations in the render loop, violating the NO-ALLOCATION ZONE guideline. Instead, leverage the return value of ImGui::InputTextMultiline, which returns true when the text is modified.
if (ImGui::InputTextMultiline("##desc", &state.description,
ImVec2(-1, ImGui::GetContentRegionAvail().y-1),
ImGuiInputTextFlags_None)) {
changed= true;
}
References
The render loop is a no-allocation zone. Avoid heap allocations such as copying strings on every frame. (link)
The reason will be displayed to describe this comment to others. Learn more.
If registry_ is null, s.client_version_name is left empty, which will display as an empty read-only field in the UI. We should always provide a fallback value (e.g., Unknown (v...)) and simplify the logic.
s.client_version_name=std::format("Unknown (v{})", ver.client_version);
if (registry_) {
if (auto*cv=registry_->findBestMatch(ver.items_minor_version,
ver.items_major_version)) {
s.client_version_name=std::format("{} ({})", cv->getName(), cv->getVersion());
}
}
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors map property editing into a reusable panel but introduces a critical regression: map resizing (Width/Height) functionality has been removed. While the UI layout has been improved and new features like the Client Version Registry and Escape key support have been added, the implementation does not yet meet quality standards due to significant code duplication (9 new clones) and 7 new issues flagged by static analysis. Most notably, the intended unification of UI patterns resulted in duplicated code rather than shared logic between the new MapPropertiesPanel and NewMapPanel. These issues, particularly the regression in map resizing, should be addressed before merging.
About this PR
The refactor has removed the ability to resize maps. In the previous implementation, the Width and Height fields were editable and persisted via map_->setSize(). In the current version, these fields are read-only and the persistence logic is missing. This functionality must be restored or the removal explicitly justified.
No new or updated unit tests were included to verify the logic of the MapPropertiesPanel, the client version resolution, or the SEC map detection logic. Adding automated tests for these areas is recommended to prevent future regressions.
Test suggestions
Missing recommended test scenario: MapPropertiesPanel correctly populates its state from a ChunkedMap including version resolution via the registry.
Missing recommended test scenario: MapPropertiesPanel marks description and external file fields as editable while keeping version/size metadata read-only.
Missing recommended test scenario: MapPropertiesDialog correctly identifies SEC maps by file extension and displays the 'not supported' warning.
Missing recommended test scenario: Pressing the Escape key inside the NewMapDialog modal closes the popup and resets state.
Missing recommended test scenario: Clicking 'OK' in MapPropertiesDialog correctly persists description and external file paths back to the ChunkedMap instance.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: MapPropertiesPanel correctly populates its state from a ChunkedMap including version resolution via the registry.
2. Missing recommended test scenario: MapPropertiesPanel marks description and external file fields as editable while keeping version/size metadata read-only.
3. Missing recommended test scenario: MapPropertiesDialog correctly identifies SEC maps by file extension and displays the 'not supported' warning.
4. Missing recommended test scenario: Pressing the Escape key inside the NewMapDialog modal closes the popup and resets state.
5. Missing recommended test scenario: Clicking 'OK' in MapPropertiesDialog correctly persists description and external file paths back to the ChunkedMap instance.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
This refactor removes the map resizing feature. The width and height inputs are now disabled in the UI, and the corresponding map_->setSize() call was removed from applyToMap(). If this was not intentional, please re-enable the inputs and restore the call to setSize.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Map size editing (Width/Height) has been disabled in this refactor. If this was intentional, it should be noted in the PR description; otherwise, it is a regression from the previous dialog behavior.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The render method is becoming monolithic as it handles multiple distinct property groups (Metadata, Version, Sizes, External Files, and Description). Breaking this down into private helper methods for the left and right columns would improve testability and readability.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Creating a copy of the description string (desc_before) every frame is inefficient for potential performance and memory. Since ImGui::InputTextMultiline returns true when the input changes, you can use its return value directly to trigger updates instead of performing a manual string comparison.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The OTBM format check (checking file extension) is performed every frame during rendering. Since the map type does not change while the dialog is open, this should be calculated once in the show() method and stored in a boolean member variable to reduce overhead.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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/UI/Panels/MapPropertiesPanel.cpp`:
- Around line 37-47: The code only sets s.client_version_name when registry_ is
non-null and a match is found, leaving s.client_version_name empty if registry_
is null; add an else branch for the registry_ check that assigns
s.client_version_name (e.g., std::format("Unknown (v{})", ver.client_version))
so the ImGui InputText for state.client_version_name always has a fallback; keep
the existing findBestMatch(ver.items_minor_version, ver.items_major_version)
call as-is and update the block around registry_, findBestMatch,
s.client_version_name and ver.client_version accordingly.
🪄 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: 2afcf74a-190f-40c5-9382-27c3376aaf7a
📥 Commits
Reviewing files that changed from the base of the PR and between d6edc18 and e7631cd.
Simplified description input change detection in map panels and streamlined client version naming logic in properties. Replaced manual state comparison with return-value checking for ImGui inputs and consolidated version formatting defaults.
- `ImguiMapEditor/UI/Panels/MapPropertiesPanel.cpp` — initialized client_version_name before lookup, simplified InputTextMultiline change detection
- `ImguiMapEditor/UI/Panels/NewMapPanel.cpp` — simplified InputTextMultiline change detection using function return value
The reason will be displayed to describe this comment to others. Learn more.
🟠 Regression: Map size fields are now read-only. In the previous implementation, they were editable. If editing map dimensions should still be supported, these fields should be enabled and the state updated.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Potential UI Bug: ImGui::InputInt is used with a local temporary int w. If this field were enabled, changes would be lost as they are not written back to state.map_width. Also, since map_width is uint16_t, it's safer to use ImGui::InputScalar with ImGuiDataType_U16 or update the state explicitly after the call.
Since it's currently disabled, this is only a "dormant" bug.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Good safety check. Note that map_->getFilename() might return an empty string for a new map that hasn't been saved yet. However, since map properties are mostly relevant for loaded OTBM files, this is likely fine for now.
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: BuildChanges to CMake, vcpkg, CI/CD, or build configuration.Area: UIChanges to the wxWidgets GUI (palettes, dialogs, menus, toolbar).Type: BugInconsistencies or issues which will cause a problem for users.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}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?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}
Extracted map property editing logic into a reusable
MapPropertiesPanelto unify UI patterns withNewMapPaneland added client version registry support. Improved dialog layout by using child containers for better space management and added support for escaping the dialog.ImguiMapEditor/Application/CallbackMediator.cpp— injected version registry intomap_propertiesdialogImguiMapEditor/CMakeLists.txt— addedMapPropertiesPanel.cppto buildImguiMapEditor/UI/Dialogs/NewMapDialog.cpp— improved modal layout usingBeginChildand added Escape key handlerImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp— refactored to useMapPropertiesPaneland added support for disabling on SEC mapsImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.h— updated to useMapPropertiesPanelcomponentImguiMapEditor/UI/Panels/MapPropertiesPanel.cpp— implemented new panel for rendering and managing map propertiesImguiMapEditor/UI/Panels/MapPropertiesPanel.h— defined interface forMapPropertiesPanelImguiMapEditor/UI/Panels/NewMapPanel.cpp— updated child container borders and height configurationSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores