Add comprehensive hotkey customization system #425
Conversation
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
Hotkey System: - Added HotkeysPage preference dialog for configuring all shortcuts - Support for customizing File, Edit, View, Panel, Mouse, and Room operations - Named hotkey configuration with defaults (e.g., Ctrl+O for Open) - Integration with MainWindow for applying hotkeys dynamically - Global shortcut registration system Features: - 50+ configurable keyboard shortcuts - Organized by category (File, Edit, View, Panels, Modes, Rooms) - Empty defaults for advanced shortcuts (user can customize) - Immediate application of hotkey changes - Persistent storage in configuration Configuration Infrastructure: - Added Hotkeys configuration struct with NamedConfig fields - All hotkeys stored with descriptive names (e.g., "HOTKEY_FILE_OPEN") - Read/write support for hotkey persistence - Change callbacks for live updates UI Integration: - Hotkeys page added to preferences dialog - Icon integration (hotkeys.png from PR-A) - Tab order and accessibility support This enables users to fully customize MMapper's keyboard shortcuts according to their preferences and workflow.
Reviewer's GuideIntroduces a configurable hotkey system wired through configuration, preferences UI, and MainWindow so that most map, panel, and room operations can have dynamic, persisted shortcuts, while also adding related canvas visibility/texture options and a new communications panel with configurable settings. Sequence diagram for applying updated hotkeys from preferencessequenceDiagram
actor User
participant MainWindow
participant ConfigDialog
participant HotkeysPage
participant Configuration
User->>MainWindow: Open Preferences (Edit -> Preferences)
MainWindow->>MainWindow: slot_onPreferences()
MainWindow->>ConfigDialog: create if needed
MainWindow->>ConfigDialog: show()
ConfigDialog-->>HotkeysPage: sig_loadConfig
HotkeysPage->>Configuration: loadSettings() from getConfig().hotkeys
Configuration-->>HotkeysPage: current hotkey values
User->>HotkeysPage: Edit one or more hotkey fields
HotkeysPage->>HotkeysPage: QKeySequenceEdit.editingFinished
HotkeysPage->>Configuration: saveSettings() to setConfig().hotkeys
HotkeysPage-->>ConfigDialog: sig_hotkeysChanged
ConfigDialog-->>MainWindow: sig_hotkeysChanged
MainWindow->>MainWindow: applyHotkeys()
MainWindow->>Configuration: read current hotkeys
Configuration-->>MainWindow: NamedConfig values
MainWindow->>MainWindow: set QAction shortcuts
User->>MainWindow: Use updated shortcut (e.g. Ctrl+O)
MainWindow->>MainWindow: QAction triggered
MainWindow->>MainWindow: connected slot executed (e.g. slot_open())
Updated class diagram for configuration hotkeys, canvas visibility, and comms settingsclassDiagram
class Configuration {
+GeneralSettings general
+CanvasSettings canvas
+Hotkeys hotkeys
+CommsSettings comms
+MumeClockSettings mumeClock
}
class NamedConfig~T~ {
+T get()
+void set(T value)
+QString getName()
}
class CanvasSettings {
+bool trilinearFiltering
+bool softwareOpenGL
+QString resourcesDirectory
+TextureSetEnum textureSet
+bool enableSeasonalTextures
+float layerTransparency
+bool enableRadialTransparency
+Advanced advanced
+VisibilityFilter visibilityFilter
+void read(QSettings conf)
+void write(QSettings conf)
}
class Advanced {
+NamedConfig~bool~ enableHiddenLayers
+NamedConfig~bool~ autoTilt
+NamedConfig~bool~ printPerfStats
+bool useBackgroundImage
+QString backgroundImagePath
+int backgroundFitMode
+float backgroundOpacity
+float backgroundFocusedScale
+float backgroundFocusedOffsetX
+float backgroundFocusedOffsetY
+FixedPoint~1~ fov
+FixedPoint~1~ verticalAngle
+FixedPoint~1~ horizontalAngle
+FixedPoint~1~ layerHeight
+void registerChangeCallback(ChangeMonitor_Lifetime lifetime, ChangeMonitor_Function callback)
}
class VisibilityFilter {
+NamedConfig~bool~ generic
+NamedConfig~bool~ herb
+NamedConfig~bool~ river
+NamedConfig~bool~ place
+NamedConfig~bool~ mob
+NamedConfig~bool~ comment
+NamedConfig~bool~ road
+NamedConfig~bool~ object
+NamedConfig~bool~ action
+NamedConfig~bool~ locality
+NamedConfig~bool~ connections
+bool isVisible(InfomarkClassEnum markerClass)
+void setVisible(InfomarkClassEnum markerClass, bool visible)
+bool isConnectionsVisible()
+void setConnectionsVisible(bool visible)
+void showAll()
+void hideAll()
+void registerChangeCallback(ChangeMonitor_Lifetime lifetime, ChangeMonitor_Function callback)
}
class Hotkeys {
+NamedConfig~QString~ fileOpen
+NamedConfig~QString~ fileSave
+NamedConfig~QString~ fileReload
+NamedConfig~QString~ fileQuit
+NamedConfig~QString~ editUndo
+NamedConfig~QString~ editRedo
+NamedConfig~QString~ editPreferences
+NamedConfig~QString~ editPreferencesAlt
+NamedConfig~QString~ editFindRooms
+NamedConfig~QString~ editRoom
+NamedConfig~QString~ viewZoomIn
+NamedConfig~QString~ viewZoomOut
+NamedConfig~QString~ viewZoomReset
+NamedConfig~QString~ viewLayerUp
+NamedConfig~QString~ viewLayerDown
+NamedConfig~QString~ viewLayerReset
+NamedConfig~QString~ viewRadialTransparency
+NamedConfig~QString~ viewStatusBar
+NamedConfig~QString~ viewScrollBars
+NamedConfig~QString~ viewMenuBar
+NamedConfig~QString~ viewAlwaysOnTop
+NamedConfig~QString~ panelLog
+NamedConfig~QString~ panelClient
+NamedConfig~QString~ panelGroup
+NamedConfig~QString~ panelRoom
+NamedConfig~QString~ panelAdventure
+NamedConfig~QString~ panelDescription
+NamedConfig~QString~ panelComms
+NamedConfig~QString~ modeMoveMap
+NamedConfig~QString~ modeRaypick
+NamedConfig~QString~ modeSelectRooms
+NamedConfig~QString~ modeSelectMarkers
+NamedConfig~QString~ modeSelectConnection
+NamedConfig~QString~ modeCreateMarker
+NamedConfig~QString~ modeCreateRoom
+NamedConfig~QString~ modeCreateConnection
+NamedConfig~QString~ modeCreateOnewayConnection
+NamedConfig~QString~ roomCreate
+NamedConfig~QString~ roomMoveUp
+NamedConfig~QString~ roomMoveDown
+NamedConfig~QString~ roomMergeUp
+NamedConfig~QString~ roomMergeDown
+NamedConfig~QString~ roomDelete
+NamedConfig~QString~ roomConnectNeighbors
+NamedConfig~QString~ roomMoveToSelected
+NamedConfig~QString~ roomUpdateSelected
+void read(QSettings conf)
+void write(QSettings conf)
}
class CommsSettings {
+NamedConfig~QColor~ tellColor
+NamedConfig~QColor~ whisperColor
+NamedConfig~QColor~ groupColor
+NamedConfig~QColor~ askColor
+NamedConfig~QColor~ sayColor
+NamedConfig~QColor~ emoteColor
+NamedConfig~QColor~ socialColor
+NamedConfig~QColor~ yellColor
+NamedConfig~QColor~ narrateColor
+NamedConfig~QColor~ prayColor
+NamedConfig~QColor~ shoutColor
+NamedConfig~QColor~ singColor
+NamedConfig~QColor~ backgroundColor
+NamedConfig~QColor~ talkerYouColor
+NamedConfig~QColor~ talkerPlayerColor
+NamedConfig~QColor~ talkerNpcColor
+NamedConfig~QColor~ talkerAllyColor
+NamedConfig~QColor~ talkerNeutralColor
+NamedConfig~QColor~ talkerEnemyColor
+NamedConfig~bool~ yellAllCaps
+NamedConfig~bool~ whisperItalic
+NamedConfig~bool~ emoteItalic
+NamedConfig~bool~ showTimestamps
+NamedConfig~bool~ saveLogOnExit
+NamedConfig~QString~ logDirectory
+NamedConfig~bool~ muteDirectTab
+NamedConfig~bool~ muteLocalTab
+NamedConfig~bool~ muteGlobalTab
+void read(QSettings conf)
+void write(QSettings conf)
}
class MumeClockSettings {
+int64_t startEpoch
+bool display
+NamedConfig~bool~ gmcpBroadcast
+NamedConfig~int~ gmcpBroadcastInterval
+MumeClockSettings()
+void read(QSettings conf)
+void write(QSettings conf)
}
class QSettings
Configuration --> CanvasSettings : has
Configuration --> Hotkeys : has
Configuration --> CommsSettings : has
Configuration --> MumeClockSettings : has
CanvasSettings --> Advanced : has
CanvasSettings --> VisibilityFilter : has
Hotkeys --> QSettings : read_write
CommsSettings --> QSettings : read_write
CanvasSettings --> QSettings : read_write
MumeClockSettings --> QSettings : read_write
Hotkeys --> NamedConfig : uses
CommsSettings --> NamedConfig : uses
VisibilityFilter --> NamedConfig : uses
Advanced --> NamedConfig : uses
MumeClockSettings --> NamedConfig : uses
Updated class diagram for MainWindow hotkey wiring and communications integrationclassDiagram
class MainWindow {
+QDockWidget* m_dockDialogLog
+QDockWidget* m_dockDialogClient
+QDockWidget* m_dockDialogGroup
+QDockWidget* m_dockDialogRoom
+QDockWidget* m_dockDialogAdventure
+QDockWidget* m_dockDialogDescription
+QDockWidget* m_dockDialogComms
+QDockWidget* m_dockDialogVisibleMarkers
+unique_ptr~GameObserver~ m_gameObserver
+AutoLogger* m_logger
+CommsManager* m_commsManager
+CommsWidget* m_commsWidget
+VisibilityFilterWidget* m_visibilityFilterWidget
+MumeClock* m_mumeClock
+QAction* openAct
+QAction* saveAct
+QAction* reloadAct
+QAction* exitAct
+QAction* m_undoAction
+QAction* m_redoAction
+QAction* preferencesAct
+QAction* findRoomsAct
+QAction* editRoomSelectionAct
+QAction* zoomInAct
+QAction* zoomOutAct
+QAction* zoomResetAct
+QAction* layerUpAct
+QAction* layerDownAct
+QAction* layerResetAct
+QAction* radialTransparencyAct
+QAction* showStatusBarAct
+QAction* showScrollBarsAct
+QAction* showMenuBarAct
+QAction* alwaysOnTopAct
+QAction* saveLogAct
+QAction* saveCommsLogAct
+void createActions()
+void setupMenuBar()
+void setupToolBars()
+void setupStatusBar()
+void wireConnections()
+void applyHotkeys()
+void registerGlobalShortcuts()
+void slot_onPreferences()
+void slot_alwaysOnTop()
+void slot_setRadialTransparency()
+bool eventFilter(QObject* obj, QEvent* event)
+void closeEvent(QCloseEvent* event)
}
class ConfigDialog {
+signal sig_graphicsSettingsChanged()
+signal sig_textureSettingsChanged()
+signal sig_groupSettingsChanged()
+signal sig_hotkeysChanged()
+signal sig_commsSettingsChanged()
+signal sig_loadConfig()
}
class HotkeysPage {
+QKeySequenceEdit* m_fileOpen
+QKeySequenceEdit* m_fileSave
+QKeySequenceEdit* m_fileReload
+QKeySequenceEdit* m_fileQuit
+QKeySequenceEdit* m_editUndo
+QKeySequenceEdit* m_editRedo
+QKeySequenceEdit* m_editPreferences
+QKeySequenceEdit* m_editPreferencesAlt
+QKeySequenceEdit* m_editFindRooms
+QKeySequenceEdit* m_editRoom
+QKeySequenceEdit* m_viewZoomIn
+QKeySequenceEdit* m_viewZoomOut
+QKeySequenceEdit* m_viewZoomReset
+QKeySequenceEdit* m_viewLayerUp
+QKeySequenceEdit* m_viewLayerDown
+QKeySequenceEdit* m_viewLayerReset
+QKeySequenceEdit* m_viewRadialTransparency
+QKeySequenceEdit* m_viewStatusBar
+QKeySequenceEdit* m_viewScrollBars
+QKeySequenceEdit* m_viewMenuBar
+QKeySequenceEdit* m_viewAlwaysOnTop
+QKeySequenceEdit* m_panelLog
+QKeySequenceEdit* m_panelClient
+QKeySequenceEdit* m_panelGroup
+QKeySequenceEdit* m_panelRoom
+QKeySequenceEdit* m_panelAdventure
+QKeySequenceEdit* m_panelComms
+QKeySequenceEdit* m_panelDescription
+QKeySequenceEdit* m_modeMoveMap
+QKeySequenceEdit* m_modeRaypick
+QKeySequenceEdit* m_modeSelectRooms
+QKeySequenceEdit* m_modeSelectMarkers
+QKeySequenceEdit* m_modeSelectConnection
+QKeySequenceEdit* m_modeCreateMarker
+QKeySequenceEdit* m_modeCreateRoom
+QKeySequenceEdit* m_modeCreateConnection
+QKeySequenceEdit* m_modeCreateOnewayConnection
+QKeySequenceEdit* m_roomCreate
+QKeySequenceEdit* m_roomMoveUp
+QKeySequenceEdit* m_roomMoveDown
+QKeySequenceEdit* m_roomMergeUp
+QKeySequenceEdit* m_roomMergeDown
+QKeySequenceEdit* m_roomDelete
+QKeySequenceEdit* m_roomConnectNeighbors
+QKeySequenceEdit* m_roomMoveToSelected
+QKeySequenceEdit* m_roomUpdateSelected
+QPushButton* m_resetButton
+HotkeysPage(QWidget* parent)
+void slot_loadConfig()
+signal sig_hotkeysChanged()
-void setupUI()
-void connectSignals()
-void loadSettings()
-void saveSettings()
-void resetToDefaults()
-QWidget* createHotkeyRow(QString label, QKeySequenceEdit** editor, QPushButton** clearBtn)
-void clearShortcut(QKeySequenceEdit* editor)
}
class CommsManager {
+signal sig_log(QString message)
+void slot_parseGmcpInput(GmcpMessage gmcp)
+void slot_parseRawGameText(QString text)
}
class CommsWidget {
+CommsWidget(CommsManager& commsManager, AutoLogger* logger, QWidget* parent)
+void slot_saveLog()
+void slot_saveLogOnExit()
+void slot_loadSettings()
}
class VisibilityFilterWidget {
+signal sig_visibilityChanged()
+signal sig_connectionsVisibilityChanged()
}
class GameObserver {
+signal sig2_sentToUserGmcp(GmcpMessage gmcp)
+signal sig2_rawGameText(QString text)
}
class MapWindow {
+MapCanvas* getCanvas()
+void slot_graphicsSettingsChanged()
}
class MapCanvas {
+void infomarksChanged()
+void slot_onSeasonChanged()
+void slot_reloadTextures()
}
class MumeClock {
+signal sig_log(QString message)
+signal sig_seasonChanged()
+MumeMoment getMumeMoment()
}
class AutoLogger {
+void slot_saveLogOnExit()
}
MainWindow --> ConfigDialog : creates
ConfigDialog --> HotkeysPage : contains
HotkeysPage --> Configuration : reads_writes_hotkeys
ConfigDialog ..> MainWindow : sig_hotkeysChanged --> applyHotkeys
HotkeysPage ..> ConfigDialog : sig_hotkeysChanged
MainWindow --> CommsManager : owns
MainWindow --> CommsWidget : owns
MainWindow --> VisibilityFilterWidget : owns
GameObserver ..> CommsManager : sig2_sentToUserGmcp --> slot_parseGmcpInput
GameObserver ..> CommsManager : sig2_rawGameText --> slot_parseRawGameText
CommsManager ..> MainWindow : sig_log --> slot_log
MainWindow ..> CommsWidget : saveCommsLogAct --> slot_saveLog
MainWindow ..> CommsWidget : closeEvent --> slot_saveLogOnExit
ConfigDialog ..> CommsWidget : sig_commsSettingsChanged --> slot_loadSettings
VisibilityFilterWidget ..> MapCanvas : sig_visibilityChanged --> infomarksChanged
VisibilityFilterWidget ..> MapCanvas : sig_connectionsVisibilityChanged --> update
MumeClock ..> MapCanvas : sig_seasonChanged --> slot_onSeasonChanged
MainWindow ..> MumeClock : construct_with_startEpoch
MumeClock ..> MainWindow : sig_log --> slot_log
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The default hotkey values are hard‑coded both in
Configuration::Hotkeys(NamedConfig defaults) and again inHotkeysPage::resetToDefaults(), which risks them getting out of sync; consider deriving the reset values directly from the NamedConfig defaults or a shared table so there is a single source of truth. - The
HotkeysPageimplementation is very repetitive (manual members, UI rows, connections, load/save code for each hotkey); you could simplify and make future additions safer by driving this from a data structure (e.g., a list of {category, label, getter/setter} descriptors) and generating the widgets, connections, and load/save logic in a loop. - Similarly,
MainWindow::applyHotkeys()andregisterGlobalShortcuts()manually enumerate each action, which is easy to overlook when adding/removing hotkeys; a shared mapping between configuration keys and actions (e.g., a static table) would reduce duplication and make it harder to forget to wire a new hotkey to both the config page and the QAction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default hotkey values are hard‑coded both in `Configuration::Hotkeys` (NamedConfig defaults) and again in `HotkeysPage::resetToDefaults()`, which risks them getting out of sync; consider deriving the reset values directly from the NamedConfig defaults or a shared table so there is a single source of truth.
- The `HotkeysPage` implementation is very repetitive (manual members, UI rows, connections, load/save code for each hotkey); you could simplify and make future additions safer by driving this from a data structure (e.g., a list of {category, label, getter/setter} descriptors) and generating the widgets, connections, and load/save logic in a loop.
- Similarly, `MainWindow::applyHotkeys()` and `registerGlobalShortcuts()` manually enumerate each action, which is easy to overlook when adding/removing hotkeys; a shared mapping between configuration keys and actions (e.g., a static table) would reduce duplication and make it harder to forget to wire a new hotkey to both the config page and the QAction.
## Individual Comments
### Comment 1
<location> `src/configuration/configuration.cpp:56-65` </location>
<code_context>
}
}
+NODISCARD TextureSetEnum intToTextureSet(int value)
+{
+ switch (value) {
+ case 0:
+ return TextureSetEnum::CLASSIC;
+ case 1:
+ return TextureSetEnum::MODERN;
+ case 2:
+ return TextureSetEnum::CUSTOM;
+ default:
+ return TextureSetEnum::MODERN; // Default to Modern
+ }
+}
+
+NODISCARD int textureSetToInt(TextureSetEnum value)
+{
+ return static_cast<int>(value);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Encoding/decoding texture set as raw ints couples config format to enum ordinal values
The manual `int` mapping and `static_cast<int>` assume `TextureSetEnum` will always be `{CLASSIC=0, MODERN=1, CUSTOM=2}`. Any future reordering or insertion would silently break existing configs. Either persist stable string identifiers instead, or assign explicit numeric values in the enum and clearly document that these helpers depend on that mapping.
Suggested implementation:
```cpp
/**
* NOTE: These helpers rely on TextureSetEnum having stable, explicitly-assigned
* integer values that are part of the persisted configuration format.
* Do not change the underlying numeric values of TextureSetEnum without
* providing a migration path for existing configs.
*/
NODISCARD TextureSetEnum intToTextureSet(int value)
{
switch (value) {
case static_cast<int>(TextureSetEnum::CLASSIC):
return TextureSetEnum::CLASSIC;
case static_cast<int>(TextureSetEnum::MODERN):
return TextureSetEnum::MODERN;
case static_cast<int>(TextureSetEnum::CUSTOM):
return TextureSetEnum::CUSTOM;
default:
// Default to Modern to preserve previous behaviour; also guards
// against unknown values from older/newer configs.
return TextureSetEnum::MODERN;
}
}
NODISCARD int textureSetToInt(TextureSetEnum value)
{
// This intentionally encodes the enum's explicit numeric value into config.
return static_cast<int>(value);
}
```
To fully address the coupling concern, you should also:
1. Update the `TextureSetEnum` definition (likely in `configuration.h` or a related header) to assign explicit numeric values, e.g.:
```cpp
enum class TextureSetEnum {
CLASSIC = 0,
MODERN = 1,
CUSTOM = 2,
};
```
2. Add a comment near the `TextureSetEnum` definition reiterating that its underlying values are part of the serialized configuration format and must remain stable.
3. If any other serialization/deserialization code assumes ordinal values (e.g. direct `static_cast<int>(TextureSetEnum)` without this helper), consider routing it through `textureSetToInt`/`intToTextureSet` or updating it to rely on the explicit enum values with similar documentation.
</issue_to_address>
### Comment 2
<location> `src/configuration/configuration.cpp:1364-1373` </location>
<code_context>
+Configuration::CanvasSettings::VisibilityFilter::VisibilityFilter() = default;
+
+bool Configuration::CanvasSettings::VisibilityFilter::isVisible(InfomarkClassEnum markerClass) const
+{
+ switch (markerClass) {
+ case InfomarkClassEnum::GENERIC:
+ return generic.get();
+ case InfomarkClassEnum::HERB:
+ return herb.get();
+ case InfomarkClassEnum::RIVER:
+ return river.get();
+ case InfomarkClassEnum::PLACE:
+ return place.get();
+ case InfomarkClassEnum::MOB:
+ return mob.get();
+ case InfomarkClassEnum::COMMENT:
+ return comment.get();
+ case InfomarkClassEnum::ROAD:
+ return road.get();
+ case InfomarkClassEnum::OBJECT:
+ return object.get();
+ case InfomarkClassEnum::ACTION:
+ return action.get();
+ case InfomarkClassEnum::LOCALITY:
+ return locality.get();
+ }
+ return true; // Default to visible for unknown types
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** VisibilityFilter silently treats unknown marker classes as visible, but setVisible ignores them
`isVisible` returns `true` for any `InfomarkClassEnum` not handled in the switch, while `setVisible` (elsewhere) ignores unknown values. If new enum values are added but not wired here, they’ll be permanently visible and not controllable via the UI. Consider making both methods fail loudly for unhandled enum values (e.g., `assert(false)`, logging + conservative default) and keeping their behavior consistent.
</issue_to_address>
### Comment 3
<location> `src/preferences/hotkeyspage.cpp:48-57` </location>
<code_context>
+void HotkeysPage::setupUI()
</code_context>
<issue_to_address>
**suggestion:** HotkeysPage builds a very tall form without any scrolling support
On smaller displays or lower resolutions, this many grouped rows will likely exceed the dialog’s vertical space and get clipped. Since you already forward-declare `QScrollArea`, consider making the main content widget the scroll area’s child and using that as the page to ensure the layout remains usable on constrained heights.
Suggested implementation:
```cpp
void HotkeysPage::setupUI()
{
auto *outerLayout = new QVBoxLayout(this);
auto *scrollArea = new QScrollArea(this);
scrollArea->setWidgetResizable(true);
auto *contentWidget = new QWidget(scrollArea);
auto *mainLayout = new QVBoxLayout(contentWidget);
scrollArea->setWidget(contentWidget);
outerLayout->addWidget(scrollArea);
// File Operations Group
```
```cpp
auto *fileGroup = new QGroupBox(tr("File Operations"));
auto *fileLayout = new QVBoxLayout(fileGroup);
```
1. Ensure `#include <QScrollArea>` and `#include <QWidget>` are present in `hotkeyspage.cpp` (or included transitively) since we are now instantiating `QScrollArea` and `QWidget` in the implementation file.
2. If there are any direct calls elsewhere in `setupUI()` that assumed the layout on `this` was `mainLayout` (e.g., `mainLayout->addStretch()` at the end), they should continue to work without changes since `mainLayout` is still used for the content, and the scroll area is the single child of the outer layout. If there is code that specifically refers to `layout()` on `this`, it now returns `outerLayout`; adapt such usages if necessary.
</issue_to_address>
### Comment 4
<location> `src/mainwindow/mainwindow.cpp:2315-2324` </location>
<code_context>
+void MainWindow::applyHotkeys()
</code_context>
<issue_to_address>
**nitpick:** applyHotkeys() emits quite verbose qDebug output for every shortcut assignment
This per-action `qDebug()` will generate a lot of console noise whenever hotkeys are applied, particularly from the preferences dialog. If it’s only for development, consider wrapping it in a debug-only guard (e.g. `#if IS_DEBUG_BUILD` or `QT_NO_DEBUG_OUTPUT`) or replacing it with a single summary log line.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+56
to
+65
| NODISCARD TextureSetEnum intToTextureSet(int value) | ||
| { | ||
| switch (value) { | ||
| case 0: | ||
| return TextureSetEnum::CLASSIC; | ||
| case 1: | ||
| return TextureSetEnum::MODERN; | ||
| case 2: | ||
| return TextureSetEnum::CUSTOM; | ||
| default: |
There was a problem hiding this comment.
suggestion (bug_risk): Encoding/decoding texture set as raw ints couples config format to enum ordinal values
The manual int mapping and static_cast<int> assume TextureSetEnum will always be {CLASSIC=0, MODERN=1, CUSTOM=2}. Any future reordering or insertion would silently break existing configs. Either persist stable string identifiers instead, or assign explicit numeric values in the enum and clearly document that these helpers depend on that mapping.
Suggested implementation:
/**
* NOTE: These helpers rely on TextureSetEnum having stable, explicitly-assigned
* integer values that are part of the persisted configuration format.
* Do not change the underlying numeric values of TextureSetEnum without
* providing a migration path for existing configs.
*/
NODISCARD TextureSetEnum intToTextureSet(int value)
{
switch (value) {
case static_cast<int>(TextureSetEnum::CLASSIC):
return TextureSetEnum::CLASSIC;
case static_cast<int>(TextureSetEnum::MODERN):
return TextureSetEnum::MODERN;
case static_cast<int>(TextureSetEnum::CUSTOM):
return TextureSetEnum::CUSTOM;
default:
// Default to Modern to preserve previous behaviour; also guards
// against unknown values from older/newer configs.
return TextureSetEnum::MODERN;
}
}
NODISCARD int textureSetToInt(TextureSetEnum value)
{
// This intentionally encodes the enum's explicit numeric value into config.
return static_cast<int>(value);
}
To fully address the coupling concern, you should also:
- Update the
TextureSetEnumdefinition (likely inconfiguration.hor a related header) to assign explicit numeric values, e.g.:enum class TextureSetEnum { CLASSIC = 0, MODERN = 1, CUSTOM = 2, };
- Add a comment near the
TextureSetEnumdefinition reiterating that its underlying values are part of the serialized configuration format and must remain stable. - If any other serialization/deserialization code assumes ordinal values (e.g. direct
static_cast<int>(TextureSetEnum)without this helper), consider routing it throughtextureSetToInt/intToTextureSetor updating it to rely on the explicit enum values with similar documentation.
Comment on lines
+1364
to
+1373
| bool Configuration::CanvasSettings::VisibilityFilter::isVisible(InfomarkClassEnum markerClass) const | ||
| { | ||
| switch (markerClass) { | ||
| case InfomarkClassEnum::GENERIC: | ||
| return generic.get(); | ||
| case InfomarkClassEnum::HERB: | ||
| return herb.get(); | ||
| case InfomarkClassEnum::RIVER: | ||
| return river.get(); | ||
| case InfomarkClassEnum::PLACE: |
There was a problem hiding this comment.
issue (bug_risk): VisibilityFilter silently treats unknown marker classes as visible, but setVisible ignores them
isVisible returns true for any InfomarkClassEnum not handled in the switch, while setVisible (elsewhere) ignores unknown values. If new enum values are added but not wired here, they’ll be permanently visible and not controllable via the UI. Consider making both methods fail loudly for unhandled enum values (e.g., assert(false), logging + conservative default) and keeping their behavior consistent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Hotkey System:
Features:
Configuration Infrastructure:
UI Integration:
This enables users to fully customize MMapper's keyboard shortcuts according to their preferences and workflow.
Summary by Sourcery
Introduce a configurable hotkey system, new communications and visibility tooling, and extended canvas/clock settings, wiring them through configuration, main window, and preferences UI.
New Features:
Enhancements:
Build: