Feat: Allow adding and deleting areas via GUI#16
Conversation
|
Thanks for working on this! Having GUI area management is a nice usability improvement. A few things I noticed: Bug: map changes won't be saved. After adding or removing an area, mpMap->setUnsaved(func) needs to be called (similar to how the Lua addAreaName and deleteArea do it in TLuaInterpreterMapper.cpp). Without this, users can add/delete areas via the GUI, close Mudlet, and silently lose all their changes. Bug: map display won't update after changes. The Lua equivalents also call mpMap->updateArea(id) after adding/removing areas (TLuaInterpreterMapper.cpp:1235 and :295), which triggers 2D/3D map refreshes and emits signal_areaChanged. Without this the map view won't update. Should allow deleting areas that contain rooms. Currently the dialog refuses to delete any area that has rooms, telling the user to move or delete them first. That's a lot of manual work. The Lua deleteArea already handles this - TRoomDB::removeArea() deletes the rooms along with the area. The GUI should allow it too, just with a confirmation that mentions the room count (e.g. "This will also delete 42 rooms. Continue?"). Replace message boxes with inline feedback. The QMessageBox pop-ups for errors ("area already exists", "cannot delete default area") are disruptive. Consider inline UI feedback instead - e.g. a status label in the dialog, or disabling the remove button when the default area is selected. Pop-up confirmations for destructive actions (deleting an area with rooms) are fine, but error/validation feedback should be inline. refreshAreas() is overly complex. It walks the parent widget hierarchy with findChildren<dlgMapper*>(), but since you already have mpMap, you can just use mpMap->mpMapper->updateAreaComboBox() directly - that's the same pointer the Lua API uses. UI uses absolute positioning. The .ui file sets hardcoded x/y/width/height geometry for widgets instead of using Qt layouts (QVBoxLayout, QHBoxLayout). This means the dialog won't resize, won't adapt to different DPI/font sizes, and will break with longer translated strings. Please use layouts instead. Unused includes. dlgConfigureAreas.cpp includes QElapsedTimer, QMenu, QPainter, QProgressDialog, and QListWidget, none of which are used. The coding standards ask to minimize includes. Missing newline at end of dlgConfigureAreas.h. The project requires all files to end with a newline character. Default area check could be simpler. checkDefaultArea() compares by name string, but the default area is always ID -1. Checking areaId == -1 is simpler and more robust (the default area name varies by translation). Also, currentAreaId() returns -1 when nothing is selected, which collides with the default area's actual ID - the areaId <= 0 guard in slot_removeArea happens to catch both but the overlap is confusing. Name trimming inconsistency. slot_addArea() checks name.trimmed().isEmpty() but passes the untrimmed name to addArea(), so area names with leading/trailing spaces would be accepted. The Lua equivalent addAreaName trims the name before using it (TLuaInterpreterMapper.cpp:272). |
|
Thanks for the detailed review, I have addressed most of the feedback, please let me know your thoughts on this revision. |
|
Hi @vadi2, could you please review these changes. |
|
Hi @vadi2, gentle ping on this whenever you get a chance. I’ve addressed most of the earlier feedback and would appreciate another review when convenient. Thanks! |
Brief overview of PR changes/additions
Motivation for adding to Mudlet
Other info (issues closed, discussion etc)
Attempts to close #8