Open
Conversation
GMCP Protocol Infrastructure: GmcpModule.h - Added MUME_TIME module type (module count 7 → 8) GmcpMessage.h - Added MUME_TIME_INFO message type (message count 30 → 31) Configuration: configuration.h - Added gmcpBroadcast (default: true) and gmcpBroadcastInterval (default: 2500ms) to MumeClockSettings configuration.cpp - Added read/write persistence for the two new settings Telnet Callback Pipeline: UserTelnet.h - Added onGmcpModuleEnabled() / virt_onGmcpModuleEnabled() to UserTelnetOutputs interface UserTelnet.cpp - Calls m_outputs.onGmcpModuleEnabled() when a supported module is enabled/disabled Proxy Broadcaster: proxy.h - Added m_clockBroadcastTimer, startClockBroadcaster(), stopClockBroadcaster(), broadcastClockInfo() proxy.cpp - Full implementation: virt_onGmcpModuleEnabled callback in LocalUserTelnetOutputs starts/stops broadcaster when MUME_TIME module is toggled startClockBroadcaster() creates a QTimer that fires at the configured interval broadcastClockInfo() builds a JSON payload with year, month, day, hour, minute, precision, season, timeOfDay, moonPhase, moonVisibility, moonLevel, dawnHour, duskHour, syncEpoch stopClockBroadcaster() in destructor for cleanup Config Dialog (Mume Protocol Page): mumeprotocolpage.h - Added slots for checkbox and spinbox mumeprotocolpage.cpp - Signal connections, config loading, and slot implementations mumeprotocolpage.ui - Added "GMCP Clock Broadcasting" group box with: Checkbox: "Broadcast clock to connected clients (GMCP MUME.Time)" SpinBox: Update interval 500-60000ms, step 500, default 2500 How it works: Client connects and negotiates GMCP, requesting MUME.Time 1 UserTelnet detects the module enable and calls onGmcpModuleEnabled(MUME_TIME, true) Proxy's startClockBroadcaster() creates a timer that fires every 2500ms (configurable) Each tick builds a JSON object from MumeClock data and sends it as MUME.Time.Info via GMCP to the user client
Reviewer's GuideImplements a GMCP-based MUME clock broadcaster tied to the MUME.Time module, with periodic JSON payloads sent via a new MUME.Time.Info GMCP message, configuration persistence, and UI controls for enabling and tuning the broadcast interval, plus a small Windows OpenGL prober include fix. Sequence diagram for GMCP MUME.Time enable and periodic clock broadcastingsequenceDiagram
actor Client
participant UserTelnet
participant Proxy
participant MumeClock
participant QTimer
Client->>UserTelnet: Negotiate GMCP, enable module MUME.Time
UserTelnet->>UserTelnet: receiveGmcpModule(MUME_TIME, true)
UserTelnet->>Proxy: onGmcpModuleEnabled(MUME_TIME, true)
Proxy->>Proxy: startClockBroadcaster()
Proxy->>Proxy: getConfig().mumeClock
Proxy->>Proxy: isUserGmcpModuleEnabled(MUME_TIME)
alt Broadcasting enabled in config
Proxy->>QTimer: create QTimer if null
Proxy->>QTimer: setInterval(gmcpBroadcastInterval)
Proxy->>QTimer: start()
Proxy->>Proxy: broadcastClockInfo() initial
Proxy->>MumeClock: getMumeMoment(), getPrecision(), getLastSyncEpoch()
Proxy->>Proxy: build JSON payload (season, timeOfDay, moon data, dawn/dusk)
Proxy->>Client: gmcpToUser(MUME.Time.Info JSON)
else Broadcasting disabled in config
Proxy-->>Proxy: Do not start timer
end
loop Every gmcpBroadcastInterval ms
QTimer->>Proxy: timeout
Proxy->>Proxy: broadcastClockInfo()
Proxy->>Proxy: isUserGmcpModuleEnabled(MUME_TIME)
alt Module still enabled
Proxy->>MumeClock: getMumeMoment(), getPrecision(), getLastSyncEpoch()
Proxy->>Proxy: rebuild JSON payload
Proxy->>Client: gmcpToUser(MUME.Time.Info JSON)
else Module disabled
Proxy-->>Proxy: return without sending
end
end
Client->>UserTelnet: Disable module MUME.Time
UserTelnet->>Proxy: onGmcpModuleEnabled(MUME_TIME, false)
Proxy->>Proxy: stopClockBroadcaster()
Proxy->>QTimer: stop()
Updated class diagram for GMCP clock broadcasting and configurationclassDiagram
class Proxy {
QTimer* m_clockBroadcastTimer
+gmcpToUser(GmcpMessage msg)
-startClockBroadcaster()
-stopClockBroadcaster()
-broadcastClockInfo()
-~Proxy()
}
class MumeClockSettings {
int64_t startEpoch
bool display
bool gmcpBroadcast
int gmcpBroadcastInterval
+read(QSettings conf)
+write(QSettings conf) const
}
class Configuration {
+MumeClockSettings mumeClock
}
class MumeProtocolPage {
+slot_loadConfig()
+slot_internalEditorRadioButtonChanged(bool value)
+slot_externalEditorCommandTextChanged(QString text)
+slot_externalEditorBrowseButtonClicked(bool checked)
+slot_gmcpBroadcastCheckBoxChanged(int state)
+slot_gmcpIntervalSpinBoxChanged(int value)
}
class UserTelnetOutputs {
+onAnalyzeUserStream(RawBytes bytes, bool fromUser)
+onRelayGmcpFromUserToMud(GmcpMessage msg)
+onRelayNawsFromUserToMud(int width, int height)
+onRelayTermTypeFromUserToMud(TelnetTermTypeBytes bytes)
+onGmcpModuleEnabled(GmcpModuleTypeEnum type, bool enabled)
<<abstract>>
-virt_onAnalyzeUserStream(RawBytes bytes, bool fromUser)
-virt_onRelayGmcpFromUserToMud(GmcpMessage msg)
-virt_onRelayNawsFromUserToMud(int width, int height)
-virt_onRelayTermTypeFromUserToMud(TelnetTermTypeBytes bytes)
-virt_onGmcpModuleEnabled(GmcpModuleTypeEnum type, bool enabled)
}
class UserTelnet {
+receiveGmcpModule(GmcpModule mod, bool enabled)
-UserTelnetOutputs m_outputs
}
class GmcpModuleTypeEnum {
<<enumeration>>
MUME_INFO
CHAR
CHAR_VITALS
MUME_CLIENT
ROOM_CHARS
ROOM
MUME_TIME
}
class GmcpMessageTypeEnum {
<<enumeration>>
CORE_HELLO
CORE_SUPPORTS_SET
CORE_SUPPORTS_ADD
CORE_SUPPORTS_REMOVE
CHAR_LOGIN
CHAR_NAME
ROOM_INFO
ROOM_CHARS_UPDATE
ROOM_UPDATE_EXITS
MUME_TIME_INFO
}
class GmcpMessage {
+GmcpMessage(GmcpMessageTypeEnum type, GmcpJson payload)
}
class MumeClock {
+getMumeMoment() MumeMoment
+getPrecision() MumeClockPrecisionEnum
+getLastSyncEpoch() int64_t
+static getDawnDusk(int month) std_pair_int_int
}
class MumeMoment {
int year
int month
int day
int hour
int minute
+toSeason() MumeSeasonEnum
+toTimeOfDay() MumeTimeEnum
+moonPhase() MumeMoonPhaseEnum
+moonVisibility() MumeMoonVisibilityEnum
+moonLevel() int
}
Proxy --> MumeClock : uses
Proxy --> GmcpMessage : sends
Proxy --> GmcpMessageTypeEnum : uses
Proxy --> Configuration : reads mumeClock
Configuration --> MumeClockSettings : aggregates
MumeProtocolPage --> Configuration : reads_writes
MumeProtocolPage --> MumeClockSettings : configures
UserTelnet --> UserTelnetOutputs : has
UserTelnetOutputs --> GmcpModuleTypeEnum : notifies
UserTelnet --> GmcpModuleTypeEnum : uses
GmcpMessage --> GmcpMessageTypeEnum : typed
GmcpModuleTypeEnum <.. Proxy : checks MUME_TIME
GmcpModuleTypeEnum <.. UserTelnetOutputs : reports MUME_TIME
MumeClock --> MumeMoment : returns
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 - I've found 1 issue, and left some high level feedback:
- When the GMCP broadcast checkbox or interval is changed in the preferences while a session is active, the existing broadcaster timer is not restarted or stopped, so consider wiring these config changes to start/stop the timer and update its interval immediately instead of only reacting to module enable/disable.
- The long series of switch blocks in broadcastClockInfo for mapping enums to string literals is verbose and error-prone to update; consider extracting reusable helper functions or lookup tables for these enum-to-string conversions to keep the logic more concise and centralized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When the GMCP broadcast checkbox or interval is changed in the preferences while a session is active, the existing broadcaster timer is not restarted or stopped, so consider wiring these config changes to start/stop the timer and update its interval immediately instead of only reacting to module enable/disable.
- The long series of switch blocks in broadcastClockInfo for mapping enums to string literals is verbose and error-prone to update; consider extracting reusable helper functions or lookup tables for these enum-to-string conversions to keep the logic more concise and centralized.
## Individual Comments
### Comment 1
<location path="src/proxy/proxy.cpp" line_range="906-924" />
<code_context>
+ QObject::connect(m_clockBroadcastTimer, &QTimer::timeout, this, &Proxy::broadcastClockInfo);
+ }
+
+ m_clockBroadcastTimer->setInterval(config.mumeClock.gmcpBroadcastInterval);
+ m_clockBroadcastTimer->start();
+
+ // Send initial update immediately
</code_context>
<issue_to_address>
**suggestion:** Consider validating or clamping the GMCP broadcast interval from configuration before using it as a QTimer interval.
If `gmcpBroadcastInterval` is 0 or negative (e.g., from manual config edits or legacy data), `QTimer` will either fire as fast as possible or not at all. It would be safer to clamp this to a sane min/max range before passing it to `setInterval()` instead of using the raw stored value.
```suggestion
if (!config.mumeClock.gmcpBroadcast
|| !isUserGmcpModuleEnabled(GmcpModuleTypeEnum::MUME_TIME)) {
return;
}
if (m_clockBroadcastTimer == nullptr) {
m_clockBroadcastTimer = new QTimer(this);
QObject::connect(m_clockBroadcastTimer, &QTimer::timeout, this, &Proxy::broadcastClockInfo);
}
// Clamp GMCP broadcast interval to a sane range before using it as a QTimer interval.
// This protects against 0/negative or excessively large values from configuration.
constexpr int kMinGmcpBroadcastIntervalMs = 1000; // 1 second
constexpr int kMaxGmcpBroadcastIntervalMs = 60000; // 60 seconds
int intervalMs = config.mumeClock.gmcpBroadcastInterval;
if (intervalMs < kMinGmcpBroadcastIntervalMs) {
intervalMs = kMinGmcpBroadcastIntervalMs;
} else if (intervalMs > kMaxGmcpBroadcastIntervalMs) {
intervalMs = kMaxGmcpBroadcastIntervalMs;
}
m_clockBroadcastTimer->setInterval(intervalMs);
m_clockBroadcastTimer->start();
// Send initial update immediately
broadcastClockInfo();
}
void Proxy::stopClockBroadcaster()
{
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- broadcastClockInfo() only checks whether the module is enabled; consider also checking the gmcpBroadcast setting there so that disabling broadcasting in the configuration immediately stops sending updates even if the timer is still running.
- broadcastClockInfo() has several long switch blocks that map enums to string literals; consider extracting these mappings into small helper functions or lookup tables to reduce repetition and make it easier to keep the string representations consistent across the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- broadcastClockInfo() only checks whether the module is enabled; consider also checking the gmcpBroadcast setting there so that disabling broadcasting in the configuration immediately stops sending updates even if the timer is still running.
- broadcastClockInfo() has several long switch blocks that map enums to string literals; consider extracting these mappings into small helper functions or lookup tables to reduce repetition and make it easier to keep the string representations consistent across the codebase.
## Individual Comments
### Comment 1
<location path="src/proxy/proxy.cpp" line_range="916" />
<code_context>
+ QObject::connect(m_clockBroadcastTimer, &QTimer::timeout, this, &Proxy::broadcastClockInfo);
+ }
+
+ m_clockBroadcastTimer->setInterval(config.mumeClock.gmcpBroadcastInterval);
+ m_clockBroadcastTimer->start();
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against very small/zero GMCP broadcast intervals to avoid a potential tight timer loop.
If `gmcpBroadcastInterval` is zero or extremely small (e.g. from a malformed/edited config), `QTimer::setInterval` will effectively fire as fast as the event loop allows, causing a busy loop and high CPU usage. Consider clamping to a sane minimum (e.g. `qMax(100, gmcpBroadcastInterval)` or at least `1`), or enforcing this when loading the configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Interval clamping (proxy.cpp:983-987): std::clamp bounds the interval to 500-60000ms, preventing tight timer loops from corrupt/manual config edits. Runtime config check (proxy.cpp:1005): broadcastClockInfo() now checks gmcpBroadcast config setting, so disabling broadcast in preferences immediately halts updates even if the timer is still running. Stop on config disable (proxy.cpp:908): startClockBroadcaster() now calls stopClockBroadcaster() when the config is disabled, so re-evaluating the broadcaster properly stops it. Enum-to-string helpers (proxy.cpp:902-968): Extracted 5 constexpr toJsonString() overloads in an anonymous namespace, replacing ~80 lines of verbose switch/break blocks with clean one-liners in broadcastClockInfo().
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
GMCP Protocol Infrastructure:
GmcpModule.h - Added MUME_TIME module type (module count 7 → 8) GmcpMessage.h - Added MUME_TIME_INFO message type (message count 30 → 31)
Configuration:
configuration.h - Added gmcpBroadcast (default: true) and gmcpBroadcastInterval (default: 2500ms) to MumeClockSettings configuration.cpp - Added read/write persistence for the two new settings
Telnet Callback Pipeline:
UserTelnet.h - Added onGmcpModuleEnabled() / virt_onGmcpModuleEnabled() to UserTelnetOutputs interface UserTelnet.cpp - Calls m_outputs.onGmcpModuleEnabled() when a supported module is enabled/disabled
Proxy Broadcaster:
proxy.h - Added m_clockBroadcastTimer, startClockBroadcaster(), stopClockBroadcaster(), broadcastClockInfo() proxy.cpp - Full implementation:
virt_onGmcpModuleEnabled callback in LocalUserTelnetOutputs starts/stops broadcaster when MUME_TIME module is toggled startClockBroadcaster() creates a QTimer that fires at the configured interval broadcastClockInfo() builds a JSON payload with year, month, day, hour, minute, precision, season, timeOfDay, moonPhase, moonVisibility, moonLevel, dawnHour, duskHour, syncEpoch stopClockBroadcaster() in destructor for cleanup
Config Dialog (Mume Protocol Page):
mumeprotocolpage.h - Added slots for checkbox and spinbox mumeprotocolpage.cpp - Signal connections, config loading, and slot implementations mumeprotocolpage.ui - Added "GMCP Clock Broadcasting" group box with: Checkbox: "Broadcast clock to connected clients (GMCP MUME.Time)" SpinBox: Update interval 500-60000ms, step 500, default 2500
How it works:
Client connects and negotiates GMCP, requesting MUME.Time 1 UserTelnet detects the module enable and calls onGmcpModuleEnabled(MUME_TIME, true) Proxy's startClockBroadcaster() creates a timer that fires every 2500ms (configurable) Each tick builds a JSON object from MumeClock data and sends it as MUME.Time.Info via GMCP to the user client
Summary by Sourcery
Introduce configurable GMCP-based broadcasting of MUME clock information to connected clients and wire it into the GMCP module negotiation flow.
New Features:
Enhancements: