From 04242fa221233c17e0d1312156451bf94f2c1529 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Sun, 1 Mar 2026 02:54:58 +0100 Subject: [PATCH] refactor: Simplify new setting introduction (#1086) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary * **What is the goal of this PR?** Eliminate the 3-file / 4-location overhead for adding a new setting. Previously, every new setting required manually editing JsonSettingsIO.cpp in two places (save + load), duplicating knowledge already present in SettingsList.h. After this PR, JsonSettingsIO.cpp never needs to be touched again for standard settings. * **What changes are included?** * `SettingInfo` (in `SettingsActivity.h`) gains one new field: `bool obfuscated` (base64 save/load for passwords), with a fluent builder method `.withObfuscated()`. The previously proposed `defaultValue`/`withDefault()` approach was dropped in favour of reading the struct field's own initializer value as the fallback (see below). * `SettingsList.h` entries are annotated with `.withObfuscated()` on the OPDS password entry. The list is now returned as a `static const` singleton (`const std::vector&`), so it is constructed exactly once. A missing `key`/`category` on the `statusBarProgressBarThickness` entry was also fixed — it was previously skipped by the generic save loop, so changes were silently lost on restart. * `JsonSettingsIO::saveSettings` and `loadSettings` replace their ~90 lines of manual per-field code with a single generic loop over `getSettingsList()`. The loop uses `info.key`, `info.valuePtr`/`info.stringOffset`+`info.stringMaxLen` (for char-array string fields), `info.enumValues.size()` (for enum clamping), and `info.obfuscated`. * **Default values**: instead of a duplicated `defaultValue` field in `SettingInfo`, `loadSettings` reads `s.*(info.valuePtr)` *before* overwriting it. Because `CrossPointSettings` is default-constructed before `loadSettings` is called, this captures each field's struct initializer value as the JSON-absent fallback. The single source of truth for defaults is `CrossPointSettings.h`. * One post-loop special case remains explicitly: the four `frontButton*` remap fields (managed by the RemapFrontButtons sub-activity, not in SettingsList) and `validateFrontButtonMapping()`. * One pre-loop migration guard handles legacy settings files that predate the status bar refactor: if `statusBarChapterPageCount` is absent from the JSON, `applyLegacyStatusBarSettings()` is called first so the generic loop picks up the migrated values as defaults and applies its normal clamping. * OPDS password backward-compat migration (plain `opdsPassword` → obfuscated `opdsPassword_obf`) is preserved inside the generic obfuscated-string path. ## Additional Context Say we want to add a new `bookmarkStyle` enum setting with options `DOT`, `LINE`, `NONE` and a default of `DOT`: 1. `src/CrossPointSettings.h` — add enum and member: ```cpp enum BOOKMARK_STYLE { BOOKMARK_DOT = 0, BOOKMARK_LINE = 1, BOOKMARK_NONE = 2 }; uint8_t bookmarkStyle = BOOKMARK_DOT; ``` 2. `lib/I18n/translations/english.yaml` — add display strings: ```yaml STR_BOOKMARK_STYLE: "Bookmark Style" STR_BOOKMARK_DOT: "Dot" STR_BOOKMARK_LINE: "Line" ``` (Other language files will fall back to English if not translated. Run `gen_i18n.py` to regenerate `I18nKeys.h`.) 3. `src/SettingsList.h` — add one entry in the appropriate category: ```cpp SettingInfo::Enum(StrId::STR_BOOKMARK_STYLE, &CrossPointSettings::bookmarkStyle, {StrId::STR_BOOKMARK_DOT, StrId::STR_BOOKMARK_LINE, StrId::STR_NONE_OPT}, "bookmarkStyle", StrId::STR_CAT_READER), ``` That's it — no default annotation needed anywhere, because `bookmarkStyle = BOOKMARK_DOT` in the struct already provides the fallback. The setting will automatically persist to JSON on save, load with clamping on boot, appear in the device settings UI under the Reader category, and be exposed via the web API — all with no further changes. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**< PARTIALLY>**_ --- src/JsonSettingsIO.cpp | 161 +++++++++---------- src/SettingsList.h | 12 +- src/activities/settings/SettingsActivity.cpp | 10 +- src/activities/settings/SettingsActivity.h | 13 +- src/network/CrossPointWebServer.cpp | 17 +- 5 files changed, 103 insertions(+), 110 deletions(-) diff --git a/src/JsonSettingsIO.cpp b/src/JsonSettingsIO.cpp index ce9df3bc..4a563142 100644 --- a/src/JsonSettingsIO.cpp +++ b/src/JsonSettingsIO.cpp @@ -6,11 +6,13 @@ #include #include +#include #include "CrossPointSettings.h" #include "CrossPointState.h" #include "KOReaderCredentialStore.h" #include "RecentBooksStore.h" +#include "SettingsList.h" #include "WifiCredentialStore.h" // Convert legacy settings. @@ -96,41 +98,28 @@ bool JsonSettingsIO::loadState(CrossPointState& s, const char* json) { bool JsonSettingsIO::saveSettings(const CrossPointSettings& s, const char* path) { JsonDocument doc; - doc["sleepScreen"] = s.sleepScreen; - doc["sleepScreenCoverMode"] = s.sleepScreenCoverMode; - doc["sleepScreenCoverFilter"] = s.sleepScreenCoverFilter; - doc["statusBar"] = s.statusBar; - doc["extraParagraphSpacing"] = s.extraParagraphSpacing; - doc["textAntiAliasing"] = s.textAntiAliasing; - doc["shortPwrBtn"] = s.shortPwrBtn; - doc["orientation"] = s.orientation; - doc["sideButtonLayout"] = s.sideButtonLayout; + for (const auto& info : getSettingsList()) { + if (!info.key) continue; + // Dynamic entries (KOReader etc.) are stored in their own files — skip. + if (!info.valuePtr && !info.stringOffset) continue; + + if (info.stringOffset) { + const char* strPtr = (const char*)&s + info.stringOffset; + if (info.obfuscated) { + doc[std::string(info.key) + "_obf"] = obfuscation::obfuscateToBase64(strPtr); + } else { + doc[info.key] = strPtr; + } + } else { + doc[info.key] = s.*(info.valuePtr); + } + } + + // Front button remap — managed by RemapFrontButtons sub-activity, not in SettingsList. doc["frontButtonBack"] = s.frontButtonBack; doc["frontButtonConfirm"] = s.frontButtonConfirm; doc["frontButtonLeft"] = s.frontButtonLeft; doc["frontButtonRight"] = s.frontButtonRight; - doc["fontFamily"] = s.fontFamily; - doc["fontSize"] = s.fontSize; - doc["lineSpacing"] = s.lineSpacing; - doc["paragraphAlignment"] = s.paragraphAlignment; - doc["sleepTimeout"] = s.sleepTimeout; - doc["refreshFrequency"] = s.refreshFrequency; - doc["screenMargin"] = s.screenMargin; - doc["opdsServerUrl"] = s.opdsServerUrl; - doc["opdsUsername"] = s.opdsUsername; - doc["opdsPassword_obf"] = obfuscation::obfuscateToBase64(s.opdsPassword); - doc["hideBatteryPercentage"] = s.hideBatteryPercentage; - doc["longPressChapterSkip"] = s.longPressChapterSkip; - doc["hyphenationEnabled"] = s.hyphenationEnabled; - doc["uiTheme"] = s.uiTheme; - doc["fadingFix"] = s.fadingFix; - doc["embeddedStyle"] = s.embeddedStyle; - doc["statusBarChapterPageCount"] = s.statusBarChapterPageCount; - doc["statusBarBookProgressPercentage"] = s.statusBarBookProgressPercentage; - doc["statusBarProgressBar"] = s.statusBarProgressBar; - doc["statusBarTitle"] = s.statusBarTitle; - doc["statusBarBattery"] = s.statusBarBattery; - doc["statusBarProgressBarThickness"] = s.statusBarProgressBarThickness; String json; serializeJson(doc, json); @@ -146,21 +135,61 @@ bool JsonSettingsIO::loadSettings(CrossPointSettings& s, const char* json, bool* return false; } - using S = CrossPointSettings; auto clamp = [](uint8_t val, uint8_t maxVal, uint8_t def) -> uint8_t { return val < maxVal ? val : def; }; - s.sleepScreen = clamp(doc["sleepScreen"] | (uint8_t)S::DARK, S::SLEEP_SCREEN_MODE_COUNT, S::DARK); - s.sleepScreenCoverMode = - clamp(doc["sleepScreenCoverMode"] | (uint8_t)S::FIT, S::SLEEP_SCREEN_COVER_MODE_COUNT, S::FIT); - s.sleepScreenCoverFilter = - clamp(doc["sleepScreenCoverFilter"] | (uint8_t)S::NO_FILTER, S::SLEEP_SCREEN_COVER_FILTER_COUNT, S::NO_FILTER); - s.statusBar = clamp(doc["statusBar"] | (uint8_t)S::FULL, S::STATUS_BAR_MODE_COUNT, S::FULL); - s.extraParagraphSpacing = doc["extraParagraphSpacing"] | (uint8_t)1; - s.textAntiAliasing = doc["textAntiAliasing"] | (uint8_t)1; - s.shortPwrBtn = clamp(doc["shortPwrBtn"] | (uint8_t)S::IGNORE, S::SHORT_PWRBTN_COUNT, S::IGNORE); - s.orientation = clamp(doc["orientation"] | (uint8_t)S::PORTRAIT, S::ORIENTATION_COUNT, S::PORTRAIT); - s.sideButtonLayout = - clamp(doc["sideButtonLayout"] | (uint8_t)S::PREV_NEXT, S::SIDE_BUTTON_LAYOUT_COUNT, S::PREV_NEXT); + // Legacy migration: if statusBarChapterPageCount is absent this is a pre-refactor settings file. + // Populate s with migrated values now so the generic loop below picks them up as defaults and clamps them. + if (doc["statusBarChapterPageCount"].isNull()) { + applyLegacyStatusBarSettings(s); + } + + for (const auto& info : getSettingsList()) { + if (!info.key) continue; + // Dynamic entries (KOReader etc.) are stored in their own files — skip. + if (!info.valuePtr && !info.stringOffset) continue; + + if (info.stringOffset) { + const char* strPtr = (const char*)&s + info.stringOffset; + const std::string fieldDefault = strPtr; // current buffer = struct-initializer default + std::string val; + if (info.obfuscated) { + bool ok = false; + val = obfuscation::deobfuscateFromBase64(doc[std::string(info.key) + "_obf"] | "", &ok); + if (!ok || val.empty()) { + val = doc[info.key] | fieldDefault; + if (val != fieldDefault && needsResave) *needsResave = true; + } + } else { + val = doc[info.key] | fieldDefault; + } + char* destPtr = (char*)&s + info.stringOffset; + if (info.stringMaxLen == 0) { + LOG_ERR("CPS", "Misconfigured SettingInfo: stringMaxLen is 0 for key '%s'", info.key); + destPtr[0] = '\0'; + if (needsResave) *needsResave = true; + continue; + } + strncpy(destPtr, val.c_str(), info.stringMaxLen - 1); + destPtr[info.stringMaxLen - 1] = '\0'; + } else { + const uint8_t fieldDefault = s.*(info.valuePtr); // struct-initializer default, read before we overwrite it + uint8_t v = doc[info.key] | fieldDefault; + if (info.type == SettingType::ENUM) { + v = clamp(v, (uint8_t)info.enumValues.size(), fieldDefault); + } else if (info.type == SettingType::TOGGLE) { + v = clamp(v, (uint8_t)2, fieldDefault); + } else if (info.type == SettingType::VALUE) { + if (v < info.valueRange.min) + v = info.valueRange.min; + else if (v > info.valueRange.max) + v = info.valueRange.max; + } + s.*(info.valuePtr) = v; + } + } + + // Front button remap — managed by RemapFrontButtons sub-activity, not in SettingsList. + using S = CrossPointSettings; s.frontButtonBack = clamp(doc["frontButtonBack"] | (uint8_t)S::FRONT_HW_BACK, S::FRONT_BUTTON_HARDWARE_COUNT, S::FRONT_HW_BACK); s.frontButtonConfirm = clamp(doc["frontButtonConfirm"] | (uint8_t)S::FRONT_HW_CONFIRM, S::FRONT_BUTTON_HARDWARE_COUNT, @@ -170,53 +199,9 @@ bool JsonSettingsIO::loadSettings(CrossPointSettings& s, const char* json, bool* s.frontButtonRight = clamp(doc["frontButtonRight"] | (uint8_t)S::FRONT_HW_RIGHT, S::FRONT_BUTTON_HARDWARE_COUNT, S::FRONT_HW_RIGHT); CrossPointSettings::validateFrontButtonMapping(s); - s.fontFamily = clamp(doc["fontFamily"] | (uint8_t)S::BOOKERLY, S::FONT_FAMILY_COUNT, S::BOOKERLY); - s.fontSize = clamp(doc["fontSize"] | (uint8_t)S::MEDIUM, S::FONT_SIZE_COUNT, S::MEDIUM); - s.lineSpacing = clamp(doc["lineSpacing"] | (uint8_t)S::NORMAL, S::LINE_COMPRESSION_COUNT, S::NORMAL); - s.paragraphAlignment = - clamp(doc["paragraphAlignment"] | (uint8_t)S::JUSTIFIED, S::PARAGRAPH_ALIGNMENT_COUNT, S::JUSTIFIED); - s.sleepTimeout = clamp(doc["sleepTimeout"] | (uint8_t)S::SLEEP_10_MIN, S::SLEEP_TIMEOUT_COUNT, S::SLEEP_10_MIN); - s.refreshFrequency = - clamp(doc["refreshFrequency"] | (uint8_t)S::REFRESH_15, S::REFRESH_FREQUENCY_COUNT, S::REFRESH_15); - s.screenMargin = doc["screenMargin"] | (uint8_t)5; - s.hideBatteryPercentage = - clamp(doc["hideBatteryPercentage"] | (uint8_t)S::HIDE_NEVER, S::HIDE_BATTERY_PERCENTAGE_COUNT, S::HIDE_NEVER); - s.longPressChapterSkip = doc["longPressChapterSkip"] | (uint8_t)1; - s.hyphenationEnabled = doc["hyphenationEnabled"] | (uint8_t)0; - s.uiTheme = doc["uiTheme"] | (uint8_t)S::LYRA; - s.fadingFix = doc["fadingFix"] | (uint8_t)0; - s.embeddedStyle = doc["embeddedStyle"] | (uint8_t)1; - const char* url = doc["opdsServerUrl"] | ""; - strncpy(s.opdsServerUrl, url, sizeof(s.opdsServerUrl) - 1); - s.opdsServerUrl[sizeof(s.opdsServerUrl) - 1] = '\0'; - - const char* user = doc["opdsUsername"] | ""; - strncpy(s.opdsUsername, user, sizeof(s.opdsUsername) - 1); - s.opdsUsername[sizeof(s.opdsUsername) - 1] = '\0'; - - bool passOk = false; - std::string pass = obfuscation::deobfuscateFromBase64(doc["opdsPassword_obf"] | "", &passOk); - if (!passOk || pass.empty()) { - pass = doc["opdsPassword"] | ""; - if (!pass.empty() && needsResave) *needsResave = true; - } - strncpy(s.opdsPassword, pass.c_str(), sizeof(s.opdsPassword) - 1); - s.opdsPassword[sizeof(s.opdsPassword) - 1] = '\0'; LOG_DBG("CPS", "Settings loaded from file"); - if (doc.containsKey("statusBarChapterPageCount")) { - s.statusBarChapterPageCount = doc["statusBarChapterPageCount"]; - s.statusBarBookProgressPercentage = doc["statusBarBookProgressPercentage"]; - s.statusBarProgressBar = doc["statusBarProgressBar"]; - s.statusBarTitle = doc["statusBarTitle"]; - s.statusBarBattery = doc["statusBarBattery"]; - } else { - applyLegacyStatusBarSettings(s); - } - - s.statusBarProgressBarThickness = doc["statusBarProgressBarThickness"] | (uint8_t)S::PROGRESS_BAR_NORMAL; - return true; } diff --git a/src/SettingsList.h b/src/SettingsList.h index dac00035..c10d58c6 100644 --- a/src/SettingsList.h +++ b/src/SettingsList.h @@ -11,8 +11,8 @@ // Shared settings list used by both the device settings UI and the web settings API. // Each entry has a key (for JSON API) and category (for grouping). // ACTION-type entries and entries without a key are device-only. -inline std::vector getSettingsList() { - return { +inline const std::vector& getSettingsList() { + static const std::vector list = { // --- Display --- SettingInfo::Enum(StrId::STR_SLEEP_SCREEN, &CrossPointSettings::sleepScreen, {StrId::STR_DARK, StrId::STR_LIGHT, StrId::STR_CUSTOM, StrId::STR_COVER, StrId::STR_NONE_OPT, @@ -113,8 +113,8 @@ inline std::vector getSettingsList() { SettingInfo::String(StrId::STR_USERNAME, SETTINGS.opdsUsername, sizeof(SETTINGS.opdsUsername), "opdsUsername", StrId::STR_OPDS_BROWSER), SettingInfo::String(StrId::STR_PASSWORD, SETTINGS.opdsPassword, sizeof(SETTINGS.opdsPassword), "opdsPassword", - StrId::STR_OPDS_BROWSER), - + StrId::STR_OPDS_BROWSER) + .withObfuscated(), // --- Status Bar Settings (web-only, uses StatusBarSettingsActivity) --- SettingInfo::Toggle(StrId::STR_CHAPTER_PAGE_COUNT, &CrossPointSettings::statusBarChapterPageCount, "statusBarChapterPageCount", StrId::STR_CUSTOMISE_STATUS_BAR), @@ -124,11 +124,13 @@ inline std::vector getSettingsList() { {StrId::STR_BOOK, StrId::STR_CHAPTER, StrId::STR_HIDE}, "statusBarProgressBar", StrId::STR_CUSTOMISE_STATUS_BAR), SettingInfo::Enum(StrId::STR_PROGRESS_BAR_THICKNESS, &CrossPointSettings::statusBarProgressBarThickness, - {StrId::STR_PROGRESS_BAR_THIN, StrId::STR_PROGRESS_BAR_MEDIUM, StrId::STR_PROGRESS_BAR_THICK}), + {StrId::STR_PROGRESS_BAR_THIN, StrId::STR_PROGRESS_BAR_MEDIUM, StrId::STR_PROGRESS_BAR_THICK}, + "statusBarProgressBarThickness", StrId::STR_CUSTOMISE_STATUS_BAR), SettingInfo::Enum(StrId::STR_TITLE, &CrossPointSettings::statusBarTitle, {StrId::STR_BOOK, StrId::STR_CHAPTER, StrId::STR_HIDE}, "statusBarTitle", StrId::STR_CUSTOMISE_STATUS_BAR), SettingInfo::Toggle(StrId::STR_BATTERY, &CrossPointSettings::statusBarBattery, "statusBarBattery", StrId::STR_CUSTOMISE_STATUS_BAR), }; + return list; } diff --git a/src/activities/settings/SettingsActivity.cpp b/src/activities/settings/SettingsActivity.cpp index 5912a2d8..659fd3e5 100644 --- a/src/activities/settings/SettingsActivity.cpp +++ b/src/activities/settings/SettingsActivity.cpp @@ -29,16 +29,16 @@ void SettingsActivity::onEnter() { controlsSettings.clear(); systemSettings.clear(); - for (auto& setting : getSettingsList()) { + for (const auto& setting : getSettingsList()) { if (setting.category == StrId::STR_NONE_OPT) continue; if (setting.category == StrId::STR_CAT_DISPLAY) { - displaySettings.push_back(std::move(setting)); + displaySettings.push_back(setting); } else if (setting.category == StrId::STR_CAT_READER) { - readerSettings.push_back(std::move(setting)); + readerSettings.push_back(setting); } else if (setting.category == StrId::STR_CAT_CONTROLS) { - controlsSettings.push_back(std::move(setting)); + controlsSettings.push_back(setting); } else if (setting.category == StrId::STR_CAT_SYSTEM) { - systemSettings.push_back(std::move(setting)); + systemSettings.push_back(setting); } // Web-only categories (KOReader Sync, OPDS Browser) are skipped for device UI } diff --git a/src/activities/settings/SettingsActivity.h b/src/activities/settings/SettingsActivity.h index 75fefa43..9d5778a9 100644 --- a/src/activities/settings/SettingsActivity.h +++ b/src/activities/settings/SettingsActivity.h @@ -5,11 +5,10 @@ #include #include +#include "CrossPointSettings.h" #include "activities/Activity.h" #include "util/ButtonNavigator.h" -class CrossPointSettings; - enum class SettingType { TOGGLE, ENUM, ACTION, VALUE, STRING }; enum class SettingAction { @@ -40,9 +39,10 @@ struct SettingInfo { const char* key = nullptr; // JSON API key (nullptr for ACTION types) StrId category = StrId::STR_NONE_OPT; // Category for web UI grouping + bool obfuscated = false; // Save/load via base64 obfuscation (passwords) // Direct char[] string fields (for settings stored in CrossPointSettings) - char* stringPtr = nullptr; + size_t stringOffset = 0; size_t stringMaxLen = 0; // Dynamic accessors (for settings stored outside CrossPointSettings, e.g. KOReaderCredentialStore) @@ -51,6 +51,11 @@ struct SettingInfo { std::function stringGetter; std::function stringSetter; + SettingInfo& withObfuscated() { + obfuscated = true; + return *this; + } + static SettingInfo Toggle(StrId nameId, uint8_t CrossPointSettings::* ptr, const char* key = nullptr, StrId category = StrId::STR_NONE_OPT) { SettingInfo s; @@ -99,7 +104,7 @@ struct SettingInfo { SettingInfo s; s.nameId = nameId; s.type = SettingType::STRING; - s.stringPtr = ptr; + s.stringOffset = (size_t)ptr - (size_t)&SETTINGS; s.stringMaxLen = maxLen; s.key = key; s.category = category; diff --git a/src/network/CrossPointWebServer.cpp b/src/network/CrossPointWebServer.cpp index 9464f166..5578c59e 100644 --- a/src/network/CrossPointWebServer.cpp +++ b/src/network/CrossPointWebServer.cpp @@ -1033,7 +1033,7 @@ void CrossPointWebServer::handleSettingsPage() const { } void CrossPointWebServer::handleGetSettings() const { - auto settings = getSettingsList(); + const auto& settings = getSettingsList(); server->setContentLength(CONTENT_LENGTH_UNKNOWN); server->send(200, "application/json", ""); @@ -1087,8 +1087,8 @@ void CrossPointWebServer::handleGetSettings() const { doc["type"] = "string"; if (s.stringGetter) { doc["value"] = s.stringGetter(); - } else if (s.stringPtr) { - doc["value"] = s.stringPtr; + } else if (s.stringOffset > 0) { + doc["value"] = reinterpret_cast(&SETTINGS) + s.stringOffset; } break; } @@ -1129,10 +1129,10 @@ void CrossPointWebServer::handlePostSettings() { return; } - auto settings = getSettingsList(); + const auto& settings = getSettingsList(); int applied = 0; - for (auto& s : settings) { + for (const auto& s : settings) { if (!s.key) continue; if (!doc[s.key].is()) continue; @@ -1171,9 +1171,10 @@ void CrossPointWebServer::handlePostSettings() { const std::string val = doc[s.key].as(); if (s.stringSetter) { s.stringSetter(val); - } else if (s.stringPtr && s.stringMaxLen > 0) { - strncpy(s.stringPtr, val.c_str(), s.stringMaxLen - 1); - s.stringPtr[s.stringMaxLen - 1] = '\0'; + } else if (s.stringOffset > 0 && s.stringMaxLen > 0) { + char* ptr = reinterpret_cast(&SETTINGS) + s.stringOffset; + strncpy(ptr, val.c_str(), s.stringMaxLen - 1); + ptr[s.stringMaxLen - 1] = '\0'; } applied++; break;