Conversation
Agent-Logs-Url: https://github.com/MaximumTrainer/MaximumTrainer_Redux/sessions/2161e8d7-50ea-41f9-82ba-49730ec00d02 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
MaximumTrainer
requested review from
Copilot
and removed request for
MaximumTrainer
There was a problem hiding this comment.
Pull request overview
This PR changes the default logging configuration so file logging is enabled out-of-the-box on fresh installs (when no prior QSettings value exists), preventing early errors from being silently dropped.
Changes:
- Flip
Logger::loadConfig()default forlogging/file_enabledfromfalsetotrue. - Update documentation to reflect the new default (header comment + README).
- Add a unit test intended to validate the new “enabled by default” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/app/logger.cpp |
Changes the QSettings default for file_enabled to true when the key is missing. |
src/app/logger.h |
Updates the documented default for logging/file_enabled to true. |
README.md |
Rewords the Log Files section to state file logging is enabled by default on first launch. |
tests/logger/tst_logger.cpp |
Adds a new test around default file_enabled behavior (currently not exercising loadConfig()). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void TstLogger::testConfig_fileLoggingEnabledByDefault() | ||
| { | ||
| // Build a QSettings file that has NO file_enabled key to simulate a | ||
| // fresh install. loadConfig() must default to enabled == true. | ||
| QTemporaryFile iniFile; | ||
| iniFile.setFileTemplate(QDir::tempPath() + QStringLiteral("/logger_default_XXXXXX.ini")); | ||
| QVERIFY(iniFile.open()); | ||
| const QString iniPath = iniFile.fileName(); | ||
| iniFile.close(); | ||
|
|
||
| { | ||
| QSettings s(iniPath, QSettings::IniFormat); | ||
| s.beginGroup(QStringLiteral("logging")); | ||
| // Only write the level; deliberately omit file_enabled so the default kicks in. | ||
| s.setValue(QStringLiteral("level"), static_cast<int>(LogLevel::Info)); | ||
| s.endGroup(); | ||
| } | ||
|
|
||
| const bool savedFileEnabled = Logger::instance().isFileLoggingEnabled(); | ||
| const QString savedFilePath = Logger::instance().logFilePath(); | ||
| Logger::instance().setFileLogging(false); // start from a known disabled state | ||
|
|
||
| // Read the value that loadConfig() would use for file_enabled. | ||
| QSettings ws(iniPath, QSettings::IniFormat); | ||
| ws.beginGroup(QStringLiteral("logging")); | ||
| const bool defaultFileEnabled = ws.value(QStringLiteral("file_enabled"), true).toBool(); | ||
| ws.endGroup(); | ||
|
|
||
| QVERIFY2(defaultFileEnabled, | ||
| "file_enabled default must be true so logging is on for fresh installs"); |
There was a problem hiding this comment.
This test doesn’t actually exercise Logger::loadConfig(); it only asserts that QSettings::value("file_enabled", true) returns true when the key is missing, which will still pass even if Logger::loadConfig() regresses back to a false default. To validate the behavior change, call Logger::instance().loadConfig() against the temp settings and assert Logger::instance().isFileLoggingEnabled() becomes true when file_enabled is absent (using a test setup that makes loadConfig() read from the temp INI).
| const bool savedFileEnabled = Logger::instance().isFileLoggingEnabled(); | ||
| const QString savedFilePath = Logger::instance().logFilePath(); | ||
| Logger::instance().setFileLogging(false); // start from a known disabled state | ||
|
|
||
| // Read the value that loadConfig() would use for file_enabled. | ||
| QSettings ws(iniPath, QSettings::IniFormat); | ||
| ws.beginGroup(QStringLiteral("logging")); | ||
| const bool defaultFileEnabled = ws.value(QStringLiteral("file_enabled"), true).toBool(); | ||
| ws.endGroup(); | ||
|
|
||
| QVERIFY2(defaultFileEnabled, | ||
| "file_enabled default must be true so logging is on for fresh installs"); | ||
|
|
||
| // Restore previous file logging state. | ||
| Logger::instance().setFileLogging(savedFileEnabled, savedFilePath); | ||
| } |
There was a problem hiding this comment.
savedFileEnabled/savedFilePath restoration is not exception/early-return safe: if any QVERIFY* fails after setFileLogging(false), the test returns immediately and the logger state won’t be restored, which can make later tests flaky. Use an RAII rollback (e.g., a small guard object or QScopeGuard) to restore file logging state regardless of assertion outcomes.
|
@copilot apply changes based on the comments in this thread |
…() with RAII guards Agent-Logs-Url: https://github.com/MaximumTrainer/MaximumTrainer_Redux/sessions/be36561c-c943-47c3-9bd6-fba36b896431 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
Applied both changes in f197f0f:
|
File logging was opt-in, meaning errors were silently dropped on fresh installs unless the user manually enabled it in Preferences. This changes the out-of-the-box default to enabled.
Changes
src/app/logger.cpp—Logger::loadConfig()default forfile_enabledflipped fromfalse→true; existing saved preferences are unaffectedsrc/app/logger.h— header comment updated to document new defaultREADME.md— Log Files section reworded to reflect always-on defaulttests/logger/tst_logger.cpp— new testtestConfig_fileLoggingEnabledByDefaultasserts that a QSettings file with nofile_enabledkey resolves totrueScope
Only the first-run/no-config path is affected. Users with an existing
file_enabled=falsesaved in QSettings retain that preference. Log level default remainsInfo(capturesWarn+Errorwithout debug noise). WASM builds are unchanged —setFileLogging()is already a no-op there.