Skip to content

refactor(ui): extract MapPropertiesDialog into MapPropertiesPanel #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
glaszczkoldre merged 2 commits into from
Jun 3, 2026
Merged

refactor(ui): extract MapPropertiesDialog into MapPropertiesPanel #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7631cd
refactor(ui): extract MapPropertiesDialog into MapPropertiesPanel
karolak6612 Jun 3, 2026
6a79be9
refactor(ui): streamline description inputs and version display
karolak6612 Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ImguiMapEditor/Application/CallbackMediator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ void CallbackMediator::wireAll(Context &ctx) {
});
}

// === Map Properties dialog — inject version registry once ===
if (ctx.map_properties && ctx.versions) {
ctx.map_properties->initialize(ctx.versions);
}

wirePlatformCallbacks(ctx);
wireTabCallbacks(ctx);
wireMapOperationCallbacks(ctx);
Expand Down
1 change: 1 addition & 0 deletions ImguiMapEditor/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ set(UI_SOURCES
UI/Map/MapViewCamera.cpp
UI/Map/SelectionMenu.cpp
UI/Panels/NewMapPanel.cpp
UI/Panels/MapPropertiesPanel.cpp
UI/Panels/BrushSizePanel.cpp
UI/PreferencesDialog.cpp
UI/Ribbon/Panels/BrushesPanel.cpp
Expand Down
26 changes: 21 additions & 5 deletions ImguiMapEditor/UI/Dialogs/NewMapDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,25 @@ void NewMapDialog::render() {
ImGui::SetNextWindowSize(
ImVec2(Config::UI::NEW_MAP_DIALOG_W, Config::UI::NEW_MAP_DIALOG_H),
ImGuiCond_Appearing);
ImGui::SetNextWindowSizeConstraints(
ImVec2(Config::UI::NEW_MAP_DIALOG_W, Config::UI::NEW_MAP_DIALOG_H),
ImVec2(FLT_MAX, FLT_MAX));

ImGui::PushStyleVar(ImGuiStyleVar_PopupRounding, 8.0f);
ImGui::PushStyleVar(ImGuiStyleVar_ChildRounding, 6.0f);
ImGui::PushStyleVar(ImGuiStyleVar_FrameRounding, 4.0f);

if (ImGui::BeginPopupModal("New Map##EditorModal", nullptr,
ImGuiWindowFlags_NoResize)) {
ImGuiWindowFlags_None)) {

// Content area: fill all space minus footer
float footer_h = ImGui::GetFrameHeightWithSpacing() + ImGui::GetStyle().ItemSpacing.y * 2;
float content_h = ImGui::GetContentRegionAvail().y - footer_h;
if (content_h < 100.0f) content_h = 100.0f;

// === TABS + CONTENT ===
ImGui::BeginChild("##content", ImVec2(0, content_h), ImGuiChildFlags_None);

// === TABS ===
if (ImGui::BeginTabBar("##NewMapTabs")) {
if (ImGui::BeginTabItem("OTBM")) {
ImGui::Spacing();
Expand All @@ -60,11 +70,11 @@ void NewMapDialog::render() {
ImGui::EndTabBar();
}

ImGui::Spacing();
ImGui::EndChild();

// === FOOTER: always visible at the bottom ===
ImGui::Separator();
ImGui::Spacing();

// === FOOTER ===
float button_width = Config::UI::MODAL_BUTTON_W;
float total_width = button_width * 2 + 10.0f;
ImGui::SetCursorPosX((ImGui::GetWindowWidth() - total_width) / 2.0f);
Expand Down Expand Up @@ -110,6 +120,12 @@ void NewMapDialog::render() {
}
}

if (ImGui::IsKeyPressed(ImGuiKey_Escape)) {

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Good addition of the Escape key handler to improve UX consistency across dialogs.

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Great addition! Adding Escape key support improves the UX consistency for modal dialogs.

visible_ = false;
state_ = {};
ImGui::CloseCurrentPopup();
}

ImGui::EndPopup();
}

Expand Down
219 changes: 81 additions & 138 deletions ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
#include "MapPropertiesDialog.h"
#include "Core/Config.h"
#include "Presentation/NotificationHelper.h"
#include <IconsFontAwesome6.h>
#include <algorithm>
#include "UI/Core/Theme.h"
#include "ext/fontawesome6/IconsFontAwesome6.h"
#include <filesystem>

Check warning on line 6 in ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

ImguiMapEditor/UI/Dialogs/Properties/MapPropertiesDialog.cpp#L6 

Include file: <filesystem> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <imgui.h>

namespace MapEditor {
namespace UI {

namespace SC = SemanticColors;

void MapPropertiesDialog::initialize(Services::ClientVersionRegistry *registry) {
panel_.initialize(registry);
}

void MapPropertiesDialog::show(Domain::ChunkedMap *map) {
if (!map)
return;

map_ = map;
state_ = panel_.loadFromMap(map);
should_open_ = true;
loadFromMap();
}

MapPropertiesDialog::Result MapPropertiesDialog::render() {
Expand All @@ -25,114 +33,77 @@
is_open_ = true;
}

// Center dialog
ImVec2 center = ImGui::GetMainViewport()->GetCenter();
ImGui::SetNextWindowPos(center, ImGuiCond_Appearing, ImVec2(0.5f, 0.5f));
ImGui::SetNextWindowSize(ImVec2(450, 420), ImGuiCond_Appearing);

if (ImGui::BeginPopupModal("Map Properties###MapPropertiesDialog", nullptr,
ImGuiWindowFlags_NoResize)) {

// === Description ===
ImGui::Text(ICON_FA_FILE_LINES " Description:");
ImGui::SetNextItemWidth(-1);
ImGui::InputTextMultiline("##Description", description_buffer_,
sizeof(description_buffer_), ImVec2(-1, 80));

ImGui::Separator();

// === Dimensions ===
ImGui::Text(ICON_FA_RULER_COMBINED " Map Size:");

ImGui::Text("Width:");
ImGui::SameLine(80);
ImGui::SetNextItemWidth(100);
ImGui::InputInt("##Width", &width_, 0, 0);
if (ImGui::IsItemHovered()) {
ImGui::SetTooltip("Map width in tiles (Min: 256, Max: 65535)");
}
width_ = std::clamp(width_, 256, 65535);

ImGui::SameLine();
ImGui::Text("Height:");
ImGui::SameLine();
ImGui::SetNextItemWidth(100);
ImGui::InputInt("##Height", &height_, 0, 0);
if (ImGui::IsItemHovered()) {
ImGui::SetTooltip("Map height in tiles (Min: 256, Max: 65535)");
}
height_ = std::clamp(height_, 256, 65535);

ImGui::Separator();

// === Version Info (read-only for now) ===
ImGui::Text(ICON_FA_CODE_BRANCH " Version Information:");
ImGui::TextDisabled("(Version conversion coming in future update)");
if (ImGui::IsItemHovered()) {
ImGui::SetTooltip("To change map version, create a new map and copy "
"content.\nDirect conversion is not yet supported.");
}

ImGui::Text("OTBM Version:");
ImGui::SameLine(120);
ImGui::Text("%u", otbm_version_);

ImGui::Text("Client Version:");
ImGui::SameLine(120);
ImGui::Text("%u", client_version_);

ImGui::Separator();

// === External Files ===
ImGui::Text(ICON_FA_LINK " External Files:");

ImGui::Text("House File:");
ImGui::SameLine(100);
ImGui::SetNextItemWidth(-1);
ImGui::InputText("##HouseFile", house_filename_, sizeof(house_filename_));
if (ImGui::IsItemHovered()) {
ImGui::SetTooltip(
"External XML file for house data (e.g., map-houses.xml)");
}

ImGui::Text("Spawn File:");
ImGui::SameLine(100);
ImGui::SetNextItemWidth(-1);
ImGui::InputText("##SpawnFile", spawn_filename_, sizeof(spawn_filename_));
if (ImGui::IsItemHovered()) {
ImGui::SetTooltip(
"External XML file for spawn data (e.g., map-spawns.xml)");
}
constexpr float PROPERTIES_DIALOG_H = Config::UI::NEW_MAP_DIALOG_H + 50.0f;

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

ImGui::SetNextWindowSize(ImVec2(Config::UI::NEW_MAP_DIALOG_W, PROPERTIES_DIALOG_H), ImGuiCond_Appearing);
ImGui::SetNextWindowSizeConstraints(ImVec2(Config::UI::NEW_MAP_DIALOG_W, PROPERTIES_DIALOG_H),
ImVec2(FLT_MAX, FLT_MAX));

ImGui::Separator();
ImGui::PushStyleVar(ImGuiStyleVar_PopupRounding, 8.0f);
ImGui::PushStyleVar(ImGuiStyleVar_ChildRounding, 6.0f);
ImGui::PushStyleVar(ImGuiStyleVar_FrameRounding, 4.0f);

// === OK / Cancel ===
float button_width = 120.0f;
float spacing = ImGui::GetStyle().ItemSpacing.x;
float total_width = button_width * 2 + spacing;
float start_x = (ImGui::GetContentRegionAvail().x - total_width) * 0.5f;

ImGui::SetCursorPosX(ImGui::GetCursorPosX() + start_x);

if (ImGui::Button(ICON_FA_CHECK " OK", ImVec2(button_width, 0))) {
applyToMap();
result = Result::Applied;
Presentation::showSuccess("Map properties updated!");
ImGui::CloseCurrentPopup();
is_open_ = false;
}
if (ImGui::IsItemHovered()) ImGui::SetTooltip("Apply changes and close");

ImGui::SameLine();

if (ImGui::Button(ICON_FA_BAN " Cancel", ImVec2(button_width, 0))) {
result = Result::Cancelled;
ImGui::CloseCurrentPopup();
is_open_ = false;
if (ImGui::BeginPopupModal("Map Properties###MapPropertiesDialog", nullptr,
ImGuiWindowFlags_None)) {

bool is_otbm = map_ &&
std::filesystem::path(map_->getFilename()).extension() == ".otbm";

@codacy-production codacy-production Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.


if (!is_otbm) {
ImGui::Spacing();
ImGui::TextColored(SC::TextDim(),
ICON_FA_TRIANGLE_EXCLAMATION
" Map properties are not supported for SEC maps.");
ImGui::Spacing();
ImGui::Separator();
ImGui::Spacing();

float button_width = Config::UI::MODAL_BUTTON_W;
ImGui::SetCursorPosX((ImGui::GetWindowWidth() - button_width) / 2.0f);
if (ImGui::Button("Close", ImVec2(button_width, 0))) {
result = Result::Cancelled;
ImGui::CloseCurrentPopup();
is_open_ = false;

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

}
} else {
// Content area: fill all space minus footer

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 The addition of this check for SEC maps is a good safety measure since they don't support metadata properties.

float footer_h = ImGui::GetFrameHeightWithSpacing() + ImGui::GetStyle().ItemSpacing.y * 2;
float content_h = ImGui::GetContentRegionAvail().y - footer_h;
if (content_h < 100.0f) content_h = 100.0f;

ImGui::BeginChild("##content", ImVec2(0, content_h), ImGuiChildFlags_None);
panel_.render(state_);
ImGui::EndChild();

// Footer: always visible at the bottom
ImGui::Separator();
float button_width = Config::UI::MODAL_BUTTON_W;
float total_width = button_width * 2 + 10.0f;
ImGui::SetCursorPosX((ImGui::GetWindowWidth() - total_width) / 2.0f);

if (ImGui::Button("Cancel", ImVec2(button_width, 0))) {
result = Result::Cancelled;
ImGui::CloseCurrentPopup();
is_open_ = false;
}

ImGui::SameLine(0, 10.0f);

ImGui::PushStyleColor(ImGuiCol_Button, SC::INFO);
ImGui::PushStyleColor(ImGuiCol_ButtonHovered, SC::Lighten(SC::INFO));

if (ImGui::Button(ICON_FA_CHECK " OK", ImVec2(button_width, 0))) {
applyToMap();
result = Result::Applied;
Presentation::showSuccess("Map properties updated!");
ImGui::CloseCurrentPopup();
is_open_ = false;
}

ImGui::PopStyleColor(2);
}
if (ImGui::IsItemHovered()) ImGui::SetTooltip("Discard changes (Esc)");

// Escape to close
if (ImGui::IsKeyPressed(ImGuiKey_Escape)) {
result = Result::Cancelled;
ImGui::CloseCurrentPopup();
Expand All @@ -141,50 +112,22 @@

ImGui::EndPopup();
} else if (is_open_) {
// Popup was closed externally
is_open_ = false;
result = Result::Cancelled;
}

return result;
}

void MapPropertiesDialog::loadFromMap() {
if (!map_)
return;
ImGui::PopStyleVar(3);

// Description
strncpy(description_buffer_, map_->getDescription().c_str(),
sizeof(description_buffer_) - 1);
description_buffer_[sizeof(description_buffer_) - 1] = '\0';

// Dimensions
width_ = map_->getWidth();
height_ = map_->getHeight();

// Version info
const auto &version = map_->getVersion();
otbm_version_ = version.otbm_version;
client_version_ = version.client_version;

// External files
strncpy(house_filename_, map_->getHouseFile().c_str(),
sizeof(house_filename_) - 1);
house_filename_[sizeof(house_filename_) - 1] = '\0';

strncpy(spawn_filename_, map_->getSpawnFile().c_str(),
sizeof(spawn_filename_) - 1);
spawn_filename_[sizeof(spawn_filename_) - 1] = '\0';
return result;

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Regression: Map width and height are no longer applied to the map. Previously, map_->setSize was called here. Is this removal intentional?

Suggested change
return result;
map_->setDescription(state_.description);
map_->setSize(state_.map_width, state_.map_height);
map_->setHouseFile(state_.house_file);

}

void MapPropertiesDialog::applyToMap() {
if (!map_)
return;

map_->setDescription(description_buffer_);
map_->setSize(static_cast<uint16_t>(width_), static_cast<uint16_t>(height_));
map_->setHouseFile(house_filename_);
map_->setSpawnFile(spawn_filename_);
map_->setDescription(state_.description);

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

Comment on lines 125 to +128

@codacy-production codacy-production Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

map_->setHouseFile(state_.house_file);
map_->setSpawnFile(state_.spawn_file);
}

} // namespace UI
Expand Down
Loading
Loading