refactor MumeClock to align its second timer with MUME #438
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
Reviewer's GuideRefactors the MUME clock so that a central MumeClock drives periodic tick updates into GameObserver, which now tracks and broadcasts time-of-day, moon, season, weather, and fog state to observers such as MumeClockWidget, while also persisting the computed MUME epoch and extending tests and build wiring accordingly. Sequence diagram for periodic MumeClock tick driving GameObserver and MumeClockWidgetsequenceDiagram
participant QtTimer as QTimer_m_timer
participant MClock as MumeClock
participant GObs as GameObserver
participant ClockW as MumeClockWidget
QtTimer->>MClock: timeout()
activate MClock
MClock->>MClock: slot_tick()
MClock->>MClock: getMumeMoment()
MClock-->>MClock: MumeMoment moment
MClock->>GObs: observeTick(moment)
activate GObs
GObs->>GObs: sig2_tick.invoke(moment)
deactivate GObs
MClock->>MClock: updateObserver(moment)
alt timeOfDay changed
MClock->>GObs: observeTimeOfDay(timeOfDay)
activate GObs
GObs->>GObs: m_timeOfDay = timeOfDay
GObs->>GObs: sig2_timeOfDayChanged.invoke(timeOfDay)
deactivate GObs
end
alt moonPhase changed
MClock->>GObs: observeMoonPhase(moonPhase)
activate GObs
GObs->>GObs: sig2_moonPhaseChanged.invoke(moonPhase)
deactivate GObs
end
alt moonVisibility changed
MClock->>GObs: observeMoonVisibility(moonVisibility)
activate GObs
GObs->>GObs: sig2_moonVisibilityChanged.invoke(moonVisibility)
deactivate GObs
end
alt season changed
MClock->>GObs: observeSeason(season)
activate GObs
GObs->>GObs: sig2_seasonChanged.invoke(season)
deactivate GObs
end
deactivate MClock
rect rgb(235,235,255)
GObs-->>ClockW: sig2_timeOfDayChanged(timeOfDay)
activate ClockW
ClockW->>ClockW: updateTime(timeOfDay)
deactivate ClockW
GObs-->>ClockW: sig2_moonPhaseChanged(phase)
activate ClockW
ClockW->>ClockW: updateMoonPhase(phase)
deactivate ClockW
GObs-->>ClockW: sig2_moonVisibilityChanged(visibility)
activate ClockW
ClockW->>ClockW: updateMoonVisibility(visibility)
deactivate ClockW
GObs-->>ClockW: sig2_seasonChanged(season)
activate ClockW
ClockW->>ClockW: updateSeason(season)
deactivate ClockW
GObs-->>ClockW: sig2_tick(moment)
activate ClockW
ClockW->>ClockW: updateCountdown(moment)
deactivate ClockW
end
Sequence diagram for GMCP weather parsing and MumeClock synchronizationsequenceDiagram
participant Mud as MumeServer
participant Conn as ProxyConnection
participant Proxy as Proxy
participant Parser as MumeXmlParser
participant GObs as GameObserver
participant Clock as MumeClock
Mud-->>Conn: GMCP char.vitals JSON
Conn-->>Proxy: GmcpMessage
Proxy-->>Parser: parseGmcpCharVitals(obj)
activate Parser
Parser->>Parser: promptFlags = PromptFlags(obj)
Parser->>Parser: fog = promptFlags.getFogType()
alt fog changed
Parser->>GObs: observeFog(fog)
activate GObs
GObs->>GObs: m_fog = fog
GObs->>GObs: sig2_fogChanged.invoke(fog)
deactivate GObs
end
Parser->>Parser: weather = promptFlags.getWeatherType()
alt weather changed
Parser->>GObs: observeWeather(weather)
activate GObs
GObs->>GObs: m_weather = weather
GObs->>GObs: sig2_weatherChanged.invoke(weather)
deactivate GObs
end
deactivate Parser
Note over Proxy,Clock: Separately, MumeClock listens to GMCP and parses time
Proxy-->>Clock: onUserGmcp(gmcp)
activate Clock
Clock->>Clock: parseWeather(time, secsSinceEpoch)
Clock->>Clock: m_timer.start(1000)
Clock->>Clock: setLastSyncEpoch(secsSinceEpoch)
Clock->>Clock: compute MumeMoment moment
alt synchronized tick
Clock->>Clock: m_precision = MINUTE
Clock->>Clock: updateObserver(moment)
end
deactivate Clock
Flow diagram for central GameObserver clock and environment stateflowchart LR
subgraph ServerSide
Mud[MUME server]
end
subgraph ClientCore
Conn[Proxy connection]
Parser[MumeXmlParser]
GObs[GameObserver]
Clock[MumeClock]
end
subgraph UI
MainWin[MainWindow]
ClockW[MumeClockWidget]
end
Mud --> Conn
Conn --> Parser
Parser --> GObs
Conn --> Clock
Clock --> GObs
MainWin --> ClockW
GObs --> ClockW
Clock --> ClockW
ClockW -->|user click| Clock
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 4 issues, and left some high level feedback:
- MumeClock::m_timer is a raw, uninitialized pointer in the default GameObserver-only constructor; consider initializing it to nullptr in the header and guarding all uses (parseWeather, etc.), or better yet reinstating a single creation path (e.g., always constructing the QTimer in a common init method or using a smart pointer) to avoid potential null/UB access.
- Since MumeClockWidget no longer owns its own QTimer and visibility is now controlled in updateCountdown via getConfig().mumeClock.display, you may want to reconsider whether the MumeClock QTimer should keep ticking at 1s resolution when the clock widget is hidden, or add a mechanism to slow/stop the clock updates when the UI is not displayed if that was an intended optimization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- MumeClock::m_timer is a raw, uninitialized pointer in the default GameObserver-only constructor; consider initializing it to nullptr in the header and guarding all uses (parseWeather, etc.), or better yet reinstating a single creation path (e.g., always constructing the QTimer in a common init method or using a smart pointer) to avoid potential null/UB access.
- Since MumeClockWidget no longer owns its own QTimer and visibility is now controlled in updateCountdown via getConfig().mumeClock.display, you may want to reconsider whether the MumeClock QTimer should keep ticking at 1s resolution when the clock widget is hidden, or add a mechanism to slow/stop the clock updates when the UI is not displayed if that was an intended optimization.
## Individual Comments
### Comment 1
<location> `src/clock/mumeclock.h:88` </location>
<code_context>
+ MumeSeasonEnum m_season = MumeSeasonEnum::UNKNOWN;
GameObserver &m_observer;
Signal2Lifetime m_lifetime;
+ QTimer *m_timer;
public:
</code_context>
<issue_to_address>
**issue (bug_risk):** m_timer is a raw pointer and not guaranteed to be initialized in all constructors, which risks null dereferences
`m_timer` is only initialized in `MumeClock::MumeClock(int64_t, GameObserver &, QObject *)`, but not in `MumeClock::MumeClock(GameObserver &observer)`. Since `parseWeather()` can be called on objects constructed via either ctor and now does `m_timer->start(1000);`, this introduces a null-dereference crash path.
Please ensure `m_timer` is always initialized before any use (e.g., default to `nullptr` at declaration and guard accesses, or factor out a common init routine that both constructors call).
</issue_to_address>
### Comment 2
<location> `src/clock/mumeclock.cpp:120-122` </location>
<code_context>
{
m_observer.sig2_sentToUserGmcp.connect(m_lifetime,
[this](const GmcpMessage &gmcp) { onUserGmcp(gmcp); });
+ m_timer = new QTimer(this);
+ connect(m_timer, &QTimer::timeout, this, &MumeClock::slot_tick);
+ m_timer->start(1000);
}
</code_context>
<issue_to_address>
**suggestion:** Timer ownership and lifecycle could be simplified and made safer by avoiding manual new
Since `MumeClock` derives from `QObject`, you can avoid `new` and a raw pointer by either:
- Making `m_timer` a value member (`QTimer m_timer;`) and initializing/connecting it in the constructor, or
- Using a smart/guarded pointer (e.g. `QPointer<QTimer>`) or at minimum initializing `m_timer` to `nullptr` and checking before use.
A value member is simplest and removes manual allocation and null-checks while still integrating with the Qt event loop.
Suggested implementation:
```cpp
{
m_observer.sig2_sentToUserGmcp.connect(m_lifetime,
[this](const GmcpMessage &gmcp) { onUserGmcp(gmcp); });
connect(&m_timer, &QTimer::timeout, this, &MumeClock::slot_tick);
m_timer.start(1000);
}
MumeClock::MumeClock(GameObserver &observer)
void MumeClock::parseWeather(const MumeTimeEnum time, int64_t secsSinceEpoch)
{
// Restart the timer to sync with the game's tick
m_timer.start(1000);
// Update last sync timestamp
setLastSyncEpoch(secsSinceEpoch);
```
To fully implement the suggestion, you'll also need to update the header and constructor:
1. In the corresponding header (likely `src/clock/mumeclock.h`), change the timer member from a pointer to a value:
- Replace `QTimer *m_timer;` (or similar) with `QTimer m_timer;`.
2. In `MumeClock`'s constructor definition (in `mumeclock.cpp`), add `m_timer(this)` to the initializer list so the timer has the correct parent:
- For example: `MumeClock::MumeClock(GameObserver &observer) : QObject(&observer), m_observer(observer), m_timer(this) { ... }`
- Keep the existing initializers you already have; just add `m_timer(this)` alongside them.
These changes remove the manual allocation, ensure the timer is always valid, and keep it properly integrated with Qt's object hierarchy.
</issue_to_address>
### Comment 3
<location> `src/proxy/MudTelnet.cpp:501-510` </location>
<code_context>
obj["os"] = mmqt::toQStringUtf8(getOs());
obj["arch"] = QSysInfo::currentCpuArchitecture();
obj["package"] = mmqt::toQStringUtf8(getPackage());
+ obj["charset"] = mmqt::toQStringUtf8(std::invoke([this]() {
+ switch (m_charset) {
+ case CharacterEncodingEnum::LATIN1:
+ return ENCODING_LATIN_1;
+ case CharacterEncodingEnum::UTF8:
+ return ENCODING_UTF_8;
+ case CharacterEncodingEnum::ASCII:
+ return ENCODING_US_ASCII;
+ default:
+ abort();
+ }
+ }));
</code_context>
<issue_to_address>
**issue:** Using abort() on unexpected charset values is harsh; prefer a safer fallback or assertion
The `switch (m_charset)` in `virt_onGmcpEnabled()` calls `abort()` in the default branch, which will terminate the client if an unexpected or new `CharacterEncodingEnum` value appears. For a user-facing client this is too severe.
Instead, use a safer strategy: e.g. log the unknown value and fall back to UTF-8 (or omit the field), or use `assert(false)` in debug builds and a safe default in release builds so unknown values don’t crash the process.
</issue_to_address>
### Comment 4
<location> `tests/testclock.cpp:248-251` </location>
<code_context>
clock.onUserGmcp(GmcpMessage::fromRawBytes(R"(Event.Moon {"what":"rise"})"));
QCOMPARE(clock.toMumeTime(clock.getMumeMoment()), expectedTime);
+
+ QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
+ static_cast<int>(MumeMoonVisibilityEnum::BRIGHT));
+
+ QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER));
}
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `parseWeatherTest` to assert all observer fields updated via `MumeClock::updateObserver`
`updateObserver` also sets `timeOfDay` and `moonPhase`. This test currently only checks moon visibility and season, so it doesn’t fully validate the observer state. Please extend this test (or add another) to also assert `observer.getTimeOfDay()` and `observer.getMoonPhase()` after the same GMCP events to better cover the integration and catch regressions in those fields.
Suggested implementation:
```cpp
QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER));
// Make sure updateObserver has also updated time-of-day and moon phase
QCOMPARE(static_cast<int>(observer.getTimeOfDay()),
static_cast<int>(MumeTimeOfDayEnum::NIGHT));
QCOMPARE(static_cast<int>(observer.getMoonPhase()),
static_cast<int>(MumeMoonPhaseEnum::FULL));
}
```
1. The exact expected enum values (`MumeTimeOfDayEnum::NIGHT`, `MumeMoonPhaseEnum::FULL`) may need to be adjusted to match how `MumeClock::updateObserver` interprets the specific GMCP event sequence used in `parseWeatherTest`. Please confirm against the implementation and update the expected values if necessary.
2. If your project uses different enum names (e.g. `MumeTimeOfDayEnum::DAYTIME`, `MumeMoonPhaseEnum::WAXING_GIBBOUS`), update the assertions accordingly to match existing definitions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/proxy/MudTelnet.cpp
Outdated
Comment on lines
501
to
510
| obj["charset"] = mmqt::toQStringUtf8(std::invoke([this]() { | ||
| switch (m_charset) { | ||
| case CharacterEncodingEnum::LATIN1: | ||
| return ENCODING_LATIN_1; | ||
| case CharacterEncodingEnum::UTF8: | ||
| return ENCODING_UTF_8; | ||
| case CharacterEncodingEnum::ASCII: | ||
| return ENCODING_US_ASCII; | ||
| default: | ||
| abort(); |
There was a problem hiding this comment.
issue: Using abort() on unexpected charset values is harsh; prefer a safer fallback or assertion
The switch (m_charset) in virt_onGmcpEnabled() calls abort() in the default branch, which will terminate the client if an unexpected or new CharacterEncodingEnum value appears. For a user-facing client this is too severe.
Instead, use a safer strategy: e.g. log the unknown value and fall back to UTF-8 (or omit the field), or use assert(false) in debug builds and a safe default in release builds so unknown values don’t crash the process.
Comment on lines
+248
to
+260
| QCOMPARE(static_cast<int>(observer.getMoonVisibility()), | ||
| static_cast<int>(MumeMoonVisibilityEnum::BRIGHT)); | ||
|
|
||
| QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER)); |
There was a problem hiding this comment.
suggestion (testing): Extend parseWeatherTest to assert all observer fields updated via MumeClock::updateObserver
updateObserver also sets timeOfDay and moonPhase. This test currently only checks moon visibility and season, so it doesn’t fully validate the observer state. Please extend this test (or add another) to also assert observer.getTimeOfDay() and observer.getMoonPhase() after the same GMCP events to better cover the integration and catch regressions in those fields.
Suggested implementation:
QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER));
// Make sure updateObserver has also updated time-of-day and moon phase
QCOMPARE(static_cast<int>(observer.getTimeOfDay()),
static_cast<int>(MumeTimeOfDayEnum::NIGHT));
QCOMPARE(static_cast<int>(observer.getMoonPhase()),
static_cast<int>(MumeMoonPhaseEnum::FULL));
}
- The exact expected enum values (
MumeTimeOfDayEnum::NIGHT,MumeMoonPhaseEnum::FULL) may need to be adjusted to match howMumeClock::updateObserverinterprets the specific GMCP event sequence used inparseWeatherTest. Please confirm against the implementation and update the expected values if necessary. - If your project uses different enum names (e.g.
MumeTimeOfDayEnum::DAYTIME,MumeMoonPhaseEnum::WAXING_GIBBOUS), update the assertions accordingly to match existing definitions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 66.48% 74.16% +7.67%
==========================================
Files 85 87 +2
Lines 4186 4021 -165
Branches 255 274 +19
==========================================
+ Hits 2783 2982 +199
+ Misses 1403 1039 -364 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nschimme
force-pushed
the
jahara-wip-2025-12-27a
branch
from
5b4dea6 to
f8927ee
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In MumeClockWidget::updateStatusTips, the comment for the UNSET precision case says there should be no status tip, but the later
if (precision != MumeClockPrecisionEnum::MINUTE)unconditionally overwritesstatusTip, so you may want to guard that second condition (e.g.,if (precision != MINUTE && precision > UNSET)) to match the intended behavior. - The previous MumeClockWidget::slot_updateLabel updated
setConfig().mumeClock.startEpochfromm_clock->getMumeStartEpoch(), but this writeback to config is now gone; if any other code still relies onmumeClock.startEpochbeing kept in sync, consider reintroducing that update in an appropriate place (e.g., when (re)syncing the clock).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In MumeClockWidget::updateStatusTips, the comment for the UNSET precision case says there should be no status tip, but the later `if (precision != MumeClockPrecisionEnum::MINUTE)` unconditionally overwrites `statusTip`, so you may want to guard that second condition (e.g., `if (precision != MINUTE && precision > UNSET)`) to match the intended behavior.
- The previous MumeClockWidget::slot_updateLabel updated `setConfig().mumeClock.startEpoch` from `m_clock->getMumeStartEpoch()`, but this writeback to config is now gone; if any other code still relies on `mumeClock.startEpoch` being kept in sync, consider reintroducing that update in an appropriate place (e.g., when (re)syncing the clock).
## Individual Comments
### Comment 1
<location> `src/proxy/AbstractTelnet.cpp:740` </location>
<code_context>
if (m_textCodec.supports(name)) {
accepted = true;
m_textCodec.setEncodingForName(name);
+ receiveCharset(m_textCodec.getEncoding());
// Reply to server that we accepted this encoding
</code_context>
<issue_to_address>
**question (bug_risk):** Server-side charset negotiation isn’t propagated into MudTelnet’s m_charset; is that intentional?
Because `MudTelnet` doesn’t override `virt_receiveCharset`, only user-side negotiation (via `UserTelnet::virt_receiveCharset` → `MudTelnet::onRelayCharset`) updates `m_charset`. Direct CHARSET negotiation with the mud won’t affect `m_charset`, so GMCP `client.info["charset"]` may not match the encoding actually negotiated on the mud connection.
If the goal is for GMCP to expose the mud connection’s effective charset, consider overriding `virt_receiveCharset` in `MudTelnet` to keep `m_charset` in sync, e.g.:
```cpp
void MudTelnet::virt_receiveCharset(const CharacterEncodingEnum cs)
{
m_charset = cs;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/proxy/AbstractTelnet.cpp
Outdated
| if (m_textCodec.supports(name)) { | ||
| accepted = true; | ||
| m_textCodec.setEncodingForName(name); | ||
| receiveCharset(m_textCodec.getEncoding()); |
There was a problem hiding this comment.
question (bug_risk): Server-side charset negotiation isn’t propagated into MudTelnet’s m_charset; is that intentional?
Because MudTelnet doesn’t override virt_receiveCharset, only user-side negotiation (via UserTelnet::virt_receiveCharset → MudTelnet::onRelayCharset) updates m_charset. Direct CHARSET negotiation with the mud won’t affect m_charset, so GMCP client.info["charset"] may not match the encoding actually negotiated on the mud connection.
If the goal is for GMCP to expose the mud connection’s effective charset, consider overriding virt_receiveCharset in MudTelnet to keep m_charset in sync, e.g.:
void MudTelnet::virt_receiveCharset(const CharacterEncodingEnum cs)
{
m_charset = cs;
}
nschimme
force-pushed
the
jahara-wip-2025-12-27a
branch
2 times, most recently
from
f2e4494 to
9f4a643
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/clock/mumeclock.cpp:397-400` </location>
<code_context>
m_precision = precision;
}
+void MumeClock::setLastSyncEpoch(int64_t epoch)
+{
+ m_lastSyncEpoch = epoch;
+ setConfig().mumeClock.startEpoch = epoch;
+}
+
</code_context>
<issue_to_address>
**question (bug_risk):** setLastSyncEpoch now writes to mumeClock.startEpoch, changing the semantics of that config field compared to previous behavior.
This used to persist the MUME start epoch via `mumeClock.startEpoch`, but now writes the last sync epoch to that same field. If other code or existing configs interpret `startEpoch` as the MUME epoch, they may misbehave after restart. If you need to persist the last sync time, consider a separate field (e.g. `lastSyncEpoch`) or a coordinated rename/migration of all uses of `startEpoch`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nschimme
force-pushed
the
jahara-wip-2025-12-27a
branch
from
9f4a643 to
0c325d1
Compare
Contributor
Author
|
@sourcery-ai review |
nschimme
changed the title
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In MumeClock::parseWeather(), updateObserver(moment) is called before you adjust the moment’s hour based on dawn/dusk, so observers will briefly see a time-of-day/moon/season derived from the uncorrected hour; consider moving updateObserver() after the hour correction so published state always reflects the final normalized moment.
- MumeClockWidget::updateCountdown() toggles visibility on every tick by re-reading getConfig().mumeClock.display; wiring this to a configuration-change callback (or a ChangeMonitor as noted in the FIXME) would avoid redundant work on each second and keep UI state changes localized to config updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In MumeClock::parseWeather(), updateObserver(moment) is called before you adjust the moment’s hour based on dawn/dusk, so observers will briefly see a time-of-day/moon/season derived from the uncorrected hour; consider moving updateObserver() after the hour correction so published state always reflects the final normalized moment.
- MumeClockWidget::updateCountdown() toggles visibility on every tick by re-reading getConfig().mumeClock.display; wiring this to a configuration-change callback (or a ChangeMonitor as noted in the FIXME) would avoid redundant work on each second and keep UI state changes localized to config updates.
## Individual Comments
### Comment 1
<location> `src/clock/mumeclock.cpp:269-276` </location>
<code_context>
void MumeClock::parseWeather(const MumeTimeEnum time, int64_t secsSinceEpoch)
{
+ // Restart the timer to sync with the game's tick
</code_context>
<issue_to_address>
**issue (bug_risk):** updateObserver is called before parseWeather finishes adjusting the moment, which can desync observer state from the final computed time
In `parseWeather`, `updateObserver(moment)` is currently called right after constructing `MumeMoment` from `secsSinceEpoch` and zeroing minutes, but before the dawn/dusk-based adjustments to hour and related fields. This lets observers read `timeOfDay`, moon, season, etc. from an intermediate state instead of the final corrected `moment`. Please move `updateObserver(moment)` to the end of `parseWeather`, after all `moment` adjustments, so observer state always matches the final clock state.
</issue_to_address>
### Comment 2
<location> `src/clock/mumeclockwidget.cpp:178-158` </location>
<code_context>
+void MumeClockWidget::updateStatusTips(const MumeMoment &moment)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Status tip updates are now tied to hover/click only, so tips can become stale while the clock runs
After the refactor, `updateStatusTips` is only called from `mousePressEvent` and `event(QEvent*)` on `HoverEnter`, instead of on every tick like `slot_updateLabel` did. This means the time/moon/season tooltips stop updating while the cursor is already over the widget, even though the countdown and precision continue to change. To keep the tooltip accurate, consider calling `updateStatusTips` from the same path as `updateCountdown` (e.g. on `sig2_tick`) or at least whenever `updateTime` runs.
</issue_to_address>
### Comment 3
<location> `tests/testclock.cpp:252-260` </location>
<code_context>
clock.onUserGmcp(GmcpMessage::fromRawBytes(R"(Event.Moon {"what":"rise"})"));
QCOMPARE(clock.toMumeTime(clock.getMumeMoment()), expectedTime);
+
+ QCOMPARE(static_cast<int>(observer.getTimeOfDay()), static_cast<int>(MumeTimeEnum::NIGHT));
+
+ QCOMPARE(static_cast<int>(observer.getMoonPhase()),
+ static_cast<int>(MumeMoonPhaseEnum::WANING_GIBBOUS));
+
+ QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
+ static_cast<int>(MumeMoonVisibilityEnum::BRIGHT));
+
+ QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER));
}
</code_context>
<issue_to_address>
**suggestion (testing):** Extend observer-related coverage to include tick events and initial/default observer state
To fully exercise the new observer integration, consider adding tests for:
1. Tick events: construct a `MumeClock` with a test `GameObserver`, trigger `slot_tick()` (or use a `QSignalSpy` on `observer.sig2_tick`), and assert that `sig2_tick` is emitted and the observer’s derived state (`getTimeOfDay`, `getMoonPhase`, etc.) matches the emitted `MumeMoment`.
2. Initial/default state: add a small test that verifies the initial values of the new state fields (e.g., `UNKNOWN` for time/moon/season, `NICE` for weather, `NO_FOG` for fog) to lock in these defaults and catch regressions.
Suggested implementation:
```cpp
#include "../src/observer/gameobserver.h"
#include "../src/proxy/GmcpMessage.h"
#include <QDebug>
#include <QtTest/QtTest>
#include <QSignalSpy>
```
```cpp
TestClock::TestClock()
{
}
void TestClock::testObserverInitialState()
{
GameObserver observer;
QCOMPARE(static_cast<int>(observer.getTimeOfDay()),
static_cast<int>(MumeTimeEnum::UNKNOWN));
QCOMPARE(static_cast<int>(observer.getMoonPhase()),
static_cast<int>(MumeMoonPhaseEnum::UNKNOWN));
QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
static_cast<int>(MumeMoonVisibilityEnum::UNKNOWN));
QCOMPARE(static_cast<int>(observer.getSeason()),
static_cast<int>(MumeSeasonEnum::UNKNOWN));
QCOMPARE(static_cast<int>(observer.getWeather()),
static_cast<int>(MumeWeatherEnum::NICE));
QCOMPARE(static_cast<int>(observer.getFog()),
static_cast<int>(MumeFogEnum::NO_FOG));
}
void TestClock::testObserverTickEvent()
{
Configuration config;
GameObserver observer;
MumeClock clock(&observer, &config);
QSignalSpy tickSpy(&observer, &GameObserver::sig2_tick);
clock.slot_tick();
QCOMPARE(tickSpy.count(), 1);
const QList<QVariant> arguments = tickSpy.takeFirst();
QVERIFY(arguments.size() == 1);
const MumeMoment moment = qvariant_cast<MumeMoment>(arguments.at(0));
QCOMPARE(clock.getMumeMoment(), moment);
const auto mumeTime = clock.toMumeTime(moment);
QCOMPARE(static_cast<int>(observer.getTimeOfDay()),
static_cast<int>(mumeTime.timeOfDay));
QCOMPARE(static_cast<int>(observer.getMoonPhase()),
static_cast<int>(mumeTime.moonPhase));
QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
static_cast<int>(mumeTime.moonVisibility));
QCOMPARE(static_cast<int>(observer.getSeason()),
static_cast<int>(mumeTime.season));
}
```
To integrate these tests fully you will also need to:
1. Declare the new test functions as Qt test slots in the `TestClock` class definition, for example in `tests/testclock.h` (or wherever `TestClock` is declared):
```cpp
private slots:
void testObserverInitialState();
void testObserverTickEvent();
```
2. Ensure `GameObserver` and `MumeClock` constructors used here match your actual API:
- If `Configuration` is a singleton or provided differently, replace `Configuration config;` / `&config` with the appropriate instance.
- Adjust the `MumeClock` constructor arguments if they differ from `(GameObserver*, Configuration*)`.
3. Confirm the names and enum fields used from `mumeTime` (`timeOfDay`, `moonPhase`, `moonVisibility`, `season`) match your `MumeTime` struct; rename as needed (e.g. `mumeTime.time_of_day`).
4. Make sure `MumeMoment` is registered with Qt’s meta-object system if not already:
```cpp
qRegisterMetaType<MumeMoment>("MumeMoment");
```
This can be done once in test initialization (e.g. in `initTestCase()`).
5. If `GameObserver::sig2_tick` has a different signature, update the `QSignalSpy` connection and argument extraction accordingly.
</issue_to_address>
### Comment 4
<location> `tests/testclock.cpp:254-255` </location>
<code_context>
+
+ QCOMPARE(static_cast<int>(observer.getTimeOfDay()), static_cast<int>(MumeTimeEnum::NIGHT));
+
+ QCOMPARE(static_cast<int>(observer.getMoonPhase()),
+ static_cast<int>(MumeMoonPhaseEnum::WANING_GIBBOUS));
+
+ QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for weather and fog propagation through `MumeXmlParser` into `GameObserver`
The new tests cover `MumeClock` → `GameObserver` astronomy updates, but not the new `MumeXmlParser` → `GameObserver` path for weather/fog.
Please add a test (here or in a `testparser`-style suite) that wires a `GameObserver` to a `MumeXmlParser`, feeds GMCP JSON with specific weather/fog `PromptFlags` (e.g., heavy fog, storm), and asserts that `observer.getWeather()` / `observer.getFog()` match the expected enums. Optionally also verify `sig2_weatherChanged` / `sig2_fogChanged` are emitted, to confirm downstream consumers can rely on the observer state.
Suggested implementation:
```cpp
#include "testclock.h"
#include "mumexmlparser.h"
#include <QSignalSpy>
```
```cpp
TestClock::~TestClock() = default;
```
```cpp
void TestClock::parseWeatherAndFogFromParserTest()
{
GameObserver observer;
MumeXmlParser parser;
// Wire parser → observer so that GMCP updates propagate into GameObserver
parser.setGameObserver(&observer);
QSignalSpy weatherSpy(&observer, &GameObserver::sig2_weatherChanged);
QSignalSpy fogSpy(&observer, &GameObserver::sig2_fogChanged);
// GMCP JSON with specific PromptFlags for weather/fog
// Adjust the JSON keys / flag strings to match the actual GMCP schema.
const QByteArray gmcpJson = R"({"promptFlags":["STORM","HEAVY_FOG"]})";
// Feed GMCP message into the parser; adjust message type if needed.
parser.onUserGmcp(GmcpMessage::fromRawBytes("Char.Prompt " + gmcpJson));
// Verify observer enums updated as expected
QCOMPARE(static_cast<int>(observer.getWeather()),
static_cast<int>(MumeWeatherEnum::STORM));
QCOMPARE(static_cast<int>(observer.getFog()),
static_cast<int>(MumeFogEnum::HEAVY_FOG));
// Verify that change signals were emitted so downstream consumers can react
QVERIFY(weatherSpy.count() > 0);
QVERIFY(fogSpy.count() > 0);
}
void TestClock::parseClockTimeTest()
```
1. Ensure `mumexmlparser.h` is the correct header for `MumeXmlParser`; if the actual header name differs, update the include accordingly.
2. Confirm that `MumeXmlParser` exposes `setGameObserver(GameObserver*)` and `onUserGmcp(const GmcpMessage&)` (or similar); if the API differs, adapt the calls in the new test.
3. Adjust the GMCP message type (`"Char.Prompt "`) and JSON structure (`"promptFlags"`, `"STORM"`, `"HEAVY_FOG"`) to match the real protocol and the existing parsing logic that sets weather/fog.
4. Replace `MumeWeatherEnum::STORM` and `MumeFogEnum::HEAVY_FOG` with the actual enum constants representing a storm and heavy fog in your codebase.
5. Add the declaration `void parseWeatherAndFogFromParserTest();` to `testclock.h` in the `private slots:` section so QtTest will discover and run the new test.
</issue_to_address>
### Comment 5
<location> `tests/testclock.cpp:260` </location>
<code_context>
+void TestClock::parseWeatherTest()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a focused test (or separate suite) for `MumeClockWidget`'s new observer-driven UI behavior
The new `MumeClockWidget` behavior is now entirely driven by `GameObserver` signals and hover events, but these paths aren’t covered by tests. Rather than extending `TestClock` directly, consider a small QtTest GUI test that:
- Instantiates `GameObserver`, `MumeClock`, and `MumeClockWidget` wired together.
- Triggers `observeTimeOfDay/observeMoonPhase/...` (or emits the corresponding signals) and asserts that `timeLabel`, `moonPhaseLabel`, and `seasonLabel` text/styles change appropriately (dawn/day/night colors, moon visibility backgrounds).
- Optionally simulates hover to exercise `updateStatusTips` and assert the status tips on the labels.
This would provide regression protection for the new observer-driven UI and styling logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| default: | ||
| break; | ||
| } | ||
| seasonLabel->setStyleSheet(styleSheet); |
There was a problem hiding this comment.
suggestion (bug_risk): Status tip updates are now tied to hover/click only, so tips can become stale while the clock runs
After the refactor, updateStatusTips is only called from mousePressEvent and event(QEvent*) on HoverEnter, instead of on every tick like slot_updateLabel did. This means the time/moon/season tooltips stop updating while the cursor is already over the widget, even though the countdown and precision continue to change. To keep the tooltip accurate, consider calling updateStatusTips from the same path as updateCountdown (e.g. on sig2_tick) or at least whenever updateTime runs.
nschimme
force-pushed
the
jahara-wip-2025-12-27a
branch
from
0c325d1 to
1383375
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In MumeClock::parseWeather, the early return on unsynchronized ticks was removed and updateObserver(moment) is always called; double-check whether you really want to propagate state updates to observers when the tick is detected as unsynchronized, or if the old behavior (ignoring those) was intentional.
- MumeClockWidget now updates status tips only on mouse press and HoverEnter; if time/moon/season changes while the user is already hovering, the tips can become stale—consider wiring updateStatusTips into the same observer/tick signals used for the labels so they stay in sync.
- The style-sheet strings for time and moon visibility are repeated in several branches; consider extracting them into small helpers or constants to avoid duplication and make it easier to tweak the visual scheme in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In MumeClock::parseWeather, the early return on unsynchronized ticks was removed and updateObserver(moment) is always called; double-check whether you really want to propagate state updates to observers when the tick is detected as unsynchronized, or if the old behavior (ignoring those) was intentional.
- MumeClockWidget now updates status tips only on mouse press and HoverEnter; if time/moon/season changes while the user is already hovering, the tips can become stale—consider wiring updateStatusTips into the same observer/tick signals used for the labels so they stay in sync.
- The style-sheet strings for time and moon visibility are repeated in several branches; consider extracting them into small helpers or constants to avoid duplication and make it easier to tweak the visual scheme in one place.
## Individual Comments
### Comment 1
<location> `src/clock/mumeclock.cpp:402-404` </location>
<code_context>
+ m_lastSyncEpoch = epoch;
+}
+
MumeClockPrecisionEnum MumeClock::getPrecision()
{
- const int64_t secsSinceEpoch = QDateTime::QDateTime::currentDateTimeUtc().toSecsSinceEpoch();
+ const int64_t secsSinceEpoch = QDateTime::QDateTime::currentSecsSinceEpoch();
if (m_precision >= MumeClockPrecisionEnum::HOUR
&& secsSinceEpoch - m_lastSyncEpoch > ONE_RL_DAY_IN_SECONDS) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The QDateTime qualifier in getPrecision is incorrect and will not compile.
`const int64_t secsSinceEpoch = QDateTime::QDateTime::currentSecsSinceEpoch();` uses an invalid `QDateTime::QDateTime::` qualifier and will not compile. This should be `QDateTime::currentSecsSinceEpoch();`, consistent with the other call sites.
</issue_to_address>
### Comment 2
<location> `tests/testclock.cpp:260` </location>
<code_context>
+ QCOMPARE(static_cast<int>(observer.getMoonVisibility()),
+ static_cast<int>(MumeMoonVisibilityEnum::BRIGHT));
+
+ QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER));
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider extending tests to cover GameObserver weather and fog updates from the GMCP prompt parser.
The prompt now calls `GameObserver::observeWeather` / `observeFog`, but the new assertions only cover time-of-day, moon phase/visibility, and season. Please add a test (if one doesn’t already exist) that verifies `GameObserver`’s `weather`/`fog` state and signals are updated correctly when GMCP prompt flags change via `MumeXmlParser::parseGmcpCharVitals`, so the new observer-facing weather/fog behavior is covered end-to-end.
</issue_to_address>
### Comment 3
<location> `tests/testclock.cpp:15-17` </location>
<code_context>
#include <QtTest/QtTest>
-TestClock::TestClock() = default;
+TestClock::TestClock()
+{
+ setEnteredMain();
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to verify that the calculated MUME start epoch is persisted into configuration when time is parsed.
This change updates the clock behavior, but there’s no test here asserting that `mumeClock.startEpoch` in the config is updated and reused. Please add a test that:
- Drives the code path to `parseMumeTime` (or the GMCP path that calls it),
- Verifies `getConfig().mumeClock.startEpoch` matches the clock’s `m_mumeStartEpoch`, and
- Optionally constructs a new `MumeClock` using the stored value to confirm it persists across restarts.
That will ensure the bugfix this PR targets is actually covered by tests.
Suggested implementation:
```cpp
TestClock::TestClock()
{
setEnteredMain();
}
void TestClock::testMumeStartEpochIsPersisted()
{
// Reset the persisted epoch to a known value
auto &config = getConfig();
config.mumeClock.startEpoch = 0;
// Drive the code path that parses MUME time and updates the config
MumeClock clock;
clock.parseMumeTime(
QStringLiteral("Mon Dec 21 12:34:56 2020") // sample MUME time; adjust to a valid format
);
// The config’s persisted epoch must match the clock’s calculated start epoch
QCOMPARE(config.mumeClock.startEpoch, clock.startEpoch());
// Construct a new clock instance and ensure it reuses the persisted epoch from config
MumeClock clockFromConfig;
QCOMPARE(clockFromConfig.startEpoch(), config.mumeClock.startEpoch);
}
```
To make this compile and correctly exercise the behavior, you may need to:
1. Ensure `TestClock` declares a corresponding private slot in the class definition, e.g.:
- `private slots: void testMumeStartEpochIsPersisted();`
2. Adjust `MumeClock` API usage to match your actual implementation:
- If the getter for the internal epoch is not `startEpoch()`, replace it with the correct method (for instance `mumeStartEpoch()` or similar).
- If `parseMumeTime(...)` has a different signature or requires a specific MUME time format, update the `QStringLiteral("Mon Dec 21 12:34:56 2020")` argument accordingly.
3. If `MumeClock` does not automatically read `mumeClock.startEpoch` from `getConfig()` in its default constructor, replace `MumeClock clockFromConfig;` with the appropriate constructor or initialization that uses the persisted epoch.
4. If your test harness uses a different macro than `QCOMPARE` or needs additional `QTEST_*` macros to register the new test method, add those in this file or the corresponding test runner file.
</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
402
to
+404
| MumeClockPrecisionEnum MumeClock::getPrecision() | ||
| { | ||
| const int64_t secsSinceEpoch = QDateTime::QDateTime::currentDateTimeUtc().toSecsSinceEpoch(); | ||
| const int64_t secsSinceEpoch = QDateTime::QDateTime::currentSecsSinceEpoch(); |
There was a problem hiding this comment.
issue (bug_risk): The QDateTime qualifier in getPrecision is incorrect and will not compile.
const int64_t secsSinceEpoch = QDateTime::QDateTime::currentSecsSinceEpoch(); uses an invalid QDateTime::QDateTime:: qualifier and will not compile. This should be QDateTime::currentSecsSinceEpoch();, consistent with the other call sites.
| QCOMPARE(static_cast<int>(observer.getMoonVisibility()), | ||
| static_cast<int>(MumeMoonVisibilityEnum::BRIGHT)); | ||
|
|
||
| QCOMPARE(static_cast<int>(observer.getSeason()), static_cast<int>(MumeSeasonEnum::SUMMER)); |
There was a problem hiding this comment.
suggestion (testing): Consider extending tests to cover GameObserver weather and fog updates from the GMCP prompt parser.
The prompt now calls GameObserver::observeWeather / observeFog, but the new assertions only cover time-of-day, moon phase/visibility, and season. Please add a test (if one doesn’t already exist) that verifies GameObserver’s weather/fog state and signals are updated correctly when GMCP prompt flags change via MumeXmlParser::parseGmcpCharVitals, so the new observer-facing weather/fog behavior is covered end-to-end.
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.
Summary by Sourcery
Align the in-game MUME clock and UI widget with the game’s tick and shared GameObserver state, and propagate time, weather, and season changes throughout the client.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: