From edf7ce1ae283cd57c237b7aad202f7a3faa8b2a6 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Sun, 26 Apr 2026 17:57:26 +0200 Subject: [PATCH] Review comments --- src/CrossPointSettings.cpp | 4 +- src/CrossPointSettings.h | 2 +- src/activities/ActivityManager.cpp | 2 +- src/activities/reader/EpubReaderActivity.cpp | 61 ++++++++++---------- src/activities/reader/MdReaderActivity.cpp | 4 ++ src/activities/reader/ReaderUtils.h | 12 +++- src/activities/reader/TxtReaderActivity.cpp | 21 ++++--- src/activities/reader/XtcReaderActivity.cpp | 24 ++++++-- src/activities/settings/SettingsActivity.cpp | 1 - src/main.cpp | 2 + 10 files changed, 85 insertions(+), 48 deletions(-) diff --git a/src/CrossPointSettings.cpp b/src/CrossPointSettings.cpp index 4f9932d6..e6719399 100644 --- a/src/CrossPointSettings.cpp +++ b/src/CrossPointSettings.cpp @@ -184,8 +184,8 @@ bool CrossPointSettings::loadFromBinaryFile() { readAndValidate(inputFile, hideBatteryPercentage, HIDE_BATTERY_PERCENTAGE_COUNT); if (++settingsRead >= fileSettingsCount) break; { - uint8_t _unused; - serialization::readPod(inputFile, _unused); + uint8_t ignored; + serialization::readPod(inputFile, ignored); } // was longPressChapterSkip if (++settingsRead >= fileSettingsCount) break; serialization::readPod(inputFile, hyphenationEnabled); diff --git a/src/CrossPointSettings.h b/src/CrossPointSettings.h index d81c8a4b..75212422 100644 --- a/src/CrossPointSettings.h +++ b/src/CrossPointSettings.h @@ -313,7 +313,7 @@ class CrossPointSettings { // Get singleton instance static CrossPointSettings& getInstance() { return instance; } - uint16_t getPowerButtonDuration() const { return 400; } + static constexpr uint16_t getPowerButtonDuration() { return 400; } int getReaderFontId() const; // If count_only is true, returns the number of settings items that would be written. diff --git a/src/activities/ActivityManager.cpp b/src/activities/ActivityManager.cpp index 47b67a56..5fe41c90 100644 --- a/src/activities/ActivityManager.cpp +++ b/src/activities/ActivityManager.cpp @@ -390,7 +390,7 @@ bool ActivityManager::isReaderActivity() const { return currentActivity && curre bool ActivityManager::skipLoopDelay() const { return currentActivity && currentActivity->skipLoopDelay(); } void ActivityManager::dispatchButtonAction(const CrossPointSettings::BUTTON_ACTION action) { - if (currentActivity) { + if (currentActivity && currentActivity->isReaderActivity()) { currentActivity->onButtonAction(action); } } diff --git a/src/activities/reader/EpubReaderActivity.cpp b/src/activities/reader/EpubReaderActivity.cpp index d80e9729..96019f1b 100644 --- a/src/activities/reader/EpubReaderActivity.cpp +++ b/src/activities/reader/EpubReaderActivity.cpp @@ -1813,6 +1813,7 @@ void EpubReaderActivity::onButtonAction(const CrossPointSettings::BUTTON_ACTION const int spineIdx = currentSpineIndex; const int tocIdx = section ? section->getTocIndexForPage(section->currentPage) : epub->getTocIndexForSpineIndex(currentSpineIndex); + ReaderUtils::enforceExitFullRefresh(renderer); startActivityForResult(std::make_unique(renderer, mappedInput, epub, epub->getPath(), spineIdx, tocIdx), [this](const ActivityResult& result) { @@ -1837,39 +1838,41 @@ void EpubReaderActivity::onButtonAction(const CrossPointSettings::BUTTON_ACTION case BA::BTN_NEXT_SECTION: case BA::BTN_PREV_SECTION: { const bool forward = (action == BA::BTN_NEXT_SECTION); - RenderLock lock(*this); - if (section && section->pageCount > 0) { - const int curTocIndex = section->getTocIndexForPage(section->currentPage); - const int nextTocIndex = forward ? curTocIndex + 1 : curTocIndex - 1; - if (curTocIndex < 0) { + { + RenderLock lock(*this); + if (section && section->pageCount > 0) { + const int curTocIndex = section->getTocIndexForPage(section->currentPage); + const int nextTocIndex = forward ? curTocIndex + 1 : curTocIndex - 1; + if (curTocIndex < 0) { + nextPageNumber = 0; + currentSpineIndex = forward ? currentSpineIndex + 1 : currentSpineIndex - 1; + section.reset(); + } else if (nextTocIndex >= 0 && nextTocIndex < epub->getTocItemsCount()) { + const int newSpineIndex = epub->getSpineIndexForTocIndex(nextTocIndex); + if (newSpineIndex == currentSpineIndex) { + if (const auto resolvedPage = section->getPageForTocIndex(nextTocIndex)) { + section->currentPage = *resolvedPage; + } + } else { + pendingTocIndex = nextTocIndex; + nextPageNumber = 0; + currentSpineIndex = newSpineIndex; + section.reset(); + } + } else if (forward) { + nextPageNumber = 0; + currentSpineIndex = epub->getSpineItemsCount(); + section.reset(); + } else { + nextPageNumber = 0; + currentSpineIndex = epub->getTocItem(curTocIndex).spineIndex - 1; + section.reset(); + } + } else { nextPageNumber = 0; currentSpineIndex = forward ? currentSpineIndex + 1 : currentSpineIndex - 1; section.reset(); - } else if (nextTocIndex >= 0 && nextTocIndex < epub->getTocItemsCount()) { - const int newSpineIndex = epub->getSpineIndexForTocIndex(nextTocIndex); - if (newSpineIndex == currentSpineIndex) { - if (const auto resolvedPage = section->getPageForTocIndex(nextTocIndex)) { - section->currentPage = *resolvedPage; - } - } else { - pendingTocIndex = nextTocIndex; - nextPageNumber = 0; - currentSpineIndex = newSpineIndex; - section.reset(); - } - } else if (forward) { - nextPageNumber = 0; - currentSpineIndex = epub->getSpineItemsCount(); - section.reset(); - } else { - nextPageNumber = 0; - currentSpineIndex = epub->getTocItem(curTocIndex).spineIndex - 1; - section.reset(); } - } else { - nextPageNumber = 0; - currentSpineIndex = forward ? currentSpineIndex + 1 : currentSpineIndex - 1; - section.reset(); } requestUpdate(); break; diff --git a/src/activities/reader/MdReaderActivity.cpp b/src/activities/reader/MdReaderActivity.cpp index 98aaa51e..5310a938 100644 --- a/src/activities/reader/MdReaderActivity.cpp +++ b/src/activities/reader/MdReaderActivity.cpp @@ -897,6 +897,10 @@ void MdReaderActivity::savePageIndexCache() const { void MdReaderActivity::onButtonAction(const CrossPointSettings::BUTTON_ACTION action) { using BA = CrossPointSettings::BUTTON_ACTION; auto clampPage = [this]() { + if (totalPages == 0) { + currentPage = 0; + return; + } if (currentPage < 0) currentPage = 0; if (currentPage >= totalPages) currentPage = totalPages - 1; }; diff --git a/src/activities/reader/ReaderUtils.h b/src/activities/reader/ReaderUtils.h index da5da0b5..1e326803 100644 --- a/src/activities/reader/ReaderUtils.h +++ b/src/activities/reader/ReaderUtils.h @@ -62,10 +62,16 @@ struct PageTurnResult { }; inline PageTurnResult detectPageTurn(const MappedInputManager& input) { + // Only treat wasReleased as a page turn when the button's short-press action is default. + // Non-default short-press actions are dispatched by the global dispatcher in main.cpp; + // counting wasReleased as well would double-fire the action. + using BA = CrossPointSettings::BUTTON_ACTION; const bool prev = - input.wasReleased(MappedInputManager::Button::PageBack) || input.wasReleased(MappedInputManager::Button::Left); - const bool next = input.wasReleased(MappedInputManager::Button::PageForward) || - input.wasReleased(MappedInputManager::Button::Right); + (SETTINGS.btnShortPageBack == BA::BTN_DEFAULT && input.wasReleased(MappedInputManager::Button::PageBack)) || + (SETTINGS.btnShortLeft == BA::BTN_DEFAULT && input.wasReleased(MappedInputManager::Button::Left)); + const bool next = + (SETTINGS.btnShortPageForward == BA::BTN_DEFAULT && input.wasReleased(MappedInputManager::Button::PageForward)) || + (SETTINGS.btnShortRight == BA::BTN_DEFAULT && input.wasReleased(MappedInputManager::Button::Right)); return {prev, next}; } diff --git a/src/activities/reader/TxtReaderActivity.cpp b/src/activities/reader/TxtReaderActivity.cpp index 4e87e54d..14a88241 100644 --- a/src/activities/reader/TxtReaderActivity.cpp +++ b/src/activities/reader/TxtReaderActivity.cpp @@ -806,15 +806,22 @@ void TxtReaderActivity::onButtonAction(const CrossPointSettings::BUTTON_ACTION a bookmarkStore.toggle(0, static_cast(currentPage)); requestUpdate(); break; - case BA::BTN_NEXT_SECTION: - currentPage += 10; - clampPage(); - requestUpdate(); + case BA::BTN_OPEN_BOOKMARKS: + if (!bookmarkStore.isEmpty()) { + ReaderUtils::enforceExitFullRefresh(renderer); + startActivityForResult(std::make_unique(renderer, mappedInput, bookmarkStore), + [this](const ActivityResult& result) { + if (!result.isCancelled) { + const auto& starred = std::get(result.data); + currentPage = starred.pageNumber; + requestUpdate(); + } + }); + } break; + case BA::BTN_NEXT_SECTION: case BA::BTN_PREV_SECTION: - currentPage -= 10; - clampPage(); - requestUpdate(); + // TXT files have no headings/chapters; treat as unsupported (no-op). break; case BA::BTN_EXIT_READER: ReaderUtils::enforceExitFullRefresh(renderer); diff --git a/src/activities/reader/XtcReaderActivity.cpp b/src/activities/reader/XtcReaderActivity.cpp index 14b4e832..d65bab38 100644 --- a/src/activities/reader/XtcReaderActivity.cpp +++ b/src/activities/reader/XtcReaderActivity.cpp @@ -460,12 +460,28 @@ void XtcReaderActivity::onButtonAction(const CrossPointSettings::BUTTON_ACTION a requestUpdate(); break; case BA::BTN_NEXT_SECTION: - currentPage = (currentPage + 10 < pageCount) ? currentPage + 10 : pageCount - 1; - requestUpdate(); + if (xtc->hasChapters()) { + const auto& chapters = xtc->getChapters(); + for (const auto& ch : chapters) { + if (ch.startPage > currentPage) { + currentPage = ch.startPage; + requestUpdate(); + break; + } + } + } break; case BA::BTN_PREV_SECTION: - currentPage = (currentPage >= 10) ? currentPage - 10 : 0; - requestUpdate(); + if (xtc->hasChapters()) { + const auto& chapters = xtc->getChapters(); + for (int i = static_cast(chapters.size()) - 1; i >= 0; i--) { + if (chapters[i].startPage < currentPage) { + currentPage = chapters[i].startPage; + requestUpdate(); + break; + } + } + } break; case BA::BTN_EXIT_READER: ReaderUtils::enforceExitFullRefresh(renderer); diff --git a/src/activities/settings/SettingsActivity.cpp b/src/activities/settings/SettingsActivity.cpp index 8f3726bb..b8e3ffba 100644 --- a/src/activities/settings/SettingsActivity.cpp +++ b/src/activities/settings/SettingsActivity.cpp @@ -82,7 +82,6 @@ void SettingsActivity::onEnter() { controlsSettings.insert(controlsSettings.begin(), SettingInfo::Action(StrId::STR_REMAP_FRONT_BUTTONS, SettingAction::RemapFrontButtons)); controlsSettings.insert(controlsSettings.begin(), SettingInfo::Separator(StrId::STR_MENU_BTN_PHYSICAL)); - lastControlsSub = StrId::STR_MENU_BTN_PHYSICAL; addToMoved(readerSettings, lastReaderSub, SettingInfo::Action(StrId::STR_CUSTOMISE_STATUS_BAR, SettingAction::CustomiseStatusBar)); diff --git a/src/main.cpp b/src/main.cpp index 3a5d7d03..f857808e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -350,6 +350,8 @@ void loop() { // Track power button hold for sleep. We require a fresh press edge (wasPressed) // before starting to measure hold time, so that a hold carried over from boot // (wake-up press) is never misinterpreted as a "go to sleep" press. + // The power button long-press is not user-remappable, so this path always owns it. + // Sleep mapped to other buttons is handled by the dispatcher's BTN_SLEEP case below. static unsigned long powerHoldStart = 0; if (gpio.wasPressed(HalGPIO::BTN_POWER)) { powerHoldStart = millis();