Page numbers in Kindle popup (#1147)

Co-authored-by: Clang Robot <[email protected]>
Co-authored-by: John Boiles <[email protected]>
Co-authored-by: John Boiles <[email protected]>

Author: 3 people

src/UI/KindleReadableScreen.cpp

@@ -237,7 +237,7 @@ KindleReadableScreen::KindleReadableScreen(DisplaySystem &display,
     popupMenuScreen_ = std::make_shared<NewReadablePopupMenuScreen>(
         display, prefs_, fileSystem_, popupMenuReadable_, lightingController_, renderer_,
         telemetry_, localStore_, stream);
-    popupMenuScreen_->setShowPageIndicators(false);
+    popupMenuScreen_->setShowPageIndicators(true);
 
     popupMenuScreen_->addTab("Popup", popupNavigationFactory, 8);
     popupMenuScreen_->addTab("Style", popupStyleFactory, 8);
@@ -870,6 +870,19 @@ void KindleReadableScreen::showPopupMenu() {
     }
 
     if (popupMenuScreen_) {
+        // For Kindle content, suppress fallback page calculation when we don't have real pages
+        popupMenuScreen_->setSuppressFallbackPageNumbers(true);
+
+        // Only set page numbers if we have valid ones from the server
+        // Note: -1 means the server hasn't provided page numbers (only locations)
+        if (kindleBook_->progressCurrentPage > 0 && kindleBook_->progressTotalPages > 0) {
+            popupMenuScreen_->setExplicitPageNumbers(kindleBook_->progressCurrentPage,
+                                                     kindleBook_->progressTotalPages);
+        } else {
+            // Clear any previous page info when we don't have valid server page numbers
+            popupMenuScreen_->setExplicitPageNumbers(-1, -1);
+        }
+
         popupMenuScreen_->setUpPopupMenu();
         popupMenuScreen_->regenerateCurrentTab();
     }

src/UI/NewReadablePopupMenuScreen.cpp

@@ -64,9 +64,18 @@ void NewReadablePopupMenuScreen::setUpPopupMenu() {
                   chapterHref.c_str());
         }
     } else {
-        size_t lengthOfFile = renderer_.size();
-        placeStringForBottom = StringFormat("%d of %d", SOL_PAGE_NUMBER(renderer_.getTxtOffset()),
-                                            SOL_PAGE_NUMBER(lengthOfFile));
+        // Use explicit page numbers if provided
+        if (hasExplicitPageNumbers()) {
+            placeStringForBottom =
+                StringFormat("%d of %d", explicitCurrentPage_, explicitTotalPages_);
+        } else if (!suppressFallbackPageNumbers_) {
+            // Calculate page numbers from offset if not suppressed
+            size_t lengthOfFile = renderer_.size();
+            placeStringForBottom =
+                StringFormat("%d of %d", SOL_PAGE_NUMBER(renderer_.getTxtOffset()),
+                             SOL_PAGE_NUMBER(lengthOfFile));
+        }
+        // If fallback is suppressed and no explicit pages, don't show anything
     }
 
     if (showPageIndicators_) {

src/UI/NewReadablePopupMenuScreen.hpp

@@ -47,6 +47,15 @@ class NewReadablePopupMenuScreen : public CarouselMenuScreen {
 
     void setShowPageIndicators(bool show) { showPageIndicators_ = show; }
 
+    // Set explicit page numbers (e.g., from server) instead of calculating from offset
+    void setExplicitPageNumbers(int currentPage, int totalPages) {
+        explicitCurrentPage_ = currentPage;
+        explicitTotalPages_ = totalPages;
+    }
+
+    // When true, don't show calculated page numbers as fallback
+    void setSuppressFallbackPageNumbers(bool suppress) { suppressFallbackPageNumbers_ = suppress; }
+
 protected:
     DisplaySystem &display_;
     PreferencesStore &prefs_;
@@ -69,4 +78,13 @@ class NewReadablePopupMenuScreen : public CarouselMenuScreen {
 
     ChapterIdx currentChapterIdx_ = -1;
     bool showPageIndicators_ = true;
+
+    // Generic page number overrides (not Kindle-specific)
+    int explicitCurrentPage_ = -1;
+    int explicitTotalPages_ = -1;
+    bool suppressFallbackPageNumbers_ = false;
+
+    auto hasExplicitPageNumbers() const -> bool {
+        return explicitCurrentPage_ > 0 && explicitTotalPages_ > 0;
+    }
 };

test/CatchTests/KindleTests.cpp

@@ -20,9 +20,7 @@
 #include "Misc/Dispatch.hpp"
 #include "Misc/Platform.hpp"
 #include "Misc/Telemetry.hpp"
-#include "Models/Json.hpp"
 #include "Models/KindleBook.hpp"
-#include "Models/KindleChaptersResponse.hpp"
 #include "Models/KindlePageFlipper.hpp"
 #include "Sensors/LightingController.hpp"
 #include "Storage/LocalStore.hpp"
@@ -706,31 +704,31 @@ TEST_CASE("Kindle table of contents highlights current chapter", "[.][no-kindle]
 
     const std::string midChapter =
         R"({"success":true,"progress":{"current_page":35,"total_pages":1000},"chapters":[{"title":"Intro","uuid":"c1","page":1},{"title":"Middle","uuid":"c2","page":20},{"title":"Later","uuid":"c3","page":40}]})";
-    auto midChapterResponse = std::make_shared<KindleChaptersResponse>();
-    JsonDocument doc;
-    deserializeJson(doc, midChapter);
-    midChapterResponse->fromJson(doc.as<JsonObject>());
-    tocScreen.handleTableOfContentsResponse(midChapterResponse);
+    auto midResp = std::make_shared<KindleChaptersResponse>();
+    SpiRamJsonDocument doc1(2048);
+    deserializeJson(doc1, midChapter);
+    midResp->fromJson(doc1.as<JsonObject>());
+    tocScreen.handleTableOfContentsResponse(midResp);
     REQUIRE(tocScreen.menuView_.getSelectedIndex() == 1);
     REQUIRE(book->progressPercent == 4);
 
     const std::string earlyChapter =
         R"({"success":true,"progress":{"current_page":0,"total_pages":1000},"chapters":[{"title":"Intro","uuid":"c1","page":5},{"title":"Next","uuid":"c2","page":15}]})";
-    auto earlyChapterResponse = std::make_shared<KindleChaptersResponse>();
-    JsonDocument doc2;
+    auto earlyResp = std::make_shared<KindleChaptersResponse>();
+    SpiRamJsonDocument doc2(2048);
     deserializeJson(doc2, earlyChapter);
-    earlyChapterResponse->fromJson(doc2.as<JsonObject>());
-    tocScreen.handleTableOfContentsResponse(earlyChapterResponse);
+    earlyResp->fromJson(doc2.as<JsonObject>());
+    tocScreen.handleTableOfContentsResponse(earlyResp);
     REQUIRE(tocScreen.menuView_.getSelectedIndex() == 0);
     REQUIRE(book->progressPercent == 0);
 
     const std::string lateChapter =
         R"({"success":true,"progress":{"current_page":200,"total_pages":1000},"chapters":[{"title":"Intro","uuid":"c1","page":5},{"title":"Next","uuid":"c2","page":115},{"title":"Last","uuid":"c3","page":190}]})";
-    auto lateChapterResponse = std::make_shared<KindleChaptersResponse>();
-    JsonDocument doc3;
+    auto lateResp = std::make_shared<KindleChaptersResponse>();
+    SpiRamJsonDocument doc3(2048);
     deserializeJson(doc3, lateChapter);
-    lateChapterResponse->fromJson(doc3.as<JsonObject>());
-    tocScreen.handleTableOfContentsResponse(lateChapterResponse);
+    lateResp->fromJson(doc3.as<JsonObject>());
+    tocScreen.handleTableOfContentsResponse(lateResp);
     REQUIRE(tocScreen.menuView_.getSelectedIndex() == 2);
     REQUIRE(book->progressPercent == 20);
 }