From f0fa90da0caef474bc971e18662bb9c1c19bfbac Mon Sep 17 00:00:00 2001 From: Eunchurn Park Date: Fri, 9 Jan 2026 23:51:24 +0900 Subject: [PATCH] refactor(home): Address PR review feedback for Continue Reading cover - Invert bookmark icon logic: show bookmark only when no cover image (as visual decoration), hide when cover is displayed to show more art - Replace GfxRenderer::storeBwBuffer usage with HomeActivity's own buffer management (storeCoverBuffer/restoreCoverBuffer/freeCoverBuffer) - Remove copyStoredBwBuffer() and freeStoredBwBuffer() from GfxRenderer as they enabled misuse of the grayscale buffer storage mechanism --- lib/GfxRenderer/GfxRenderer.cpp | 32 ------- lib/GfxRenderer/GfxRenderer.h | 6 +- src/activities/home/HomeActivity.cpp | 134 +++++++++++++++------------ src/activities/home/HomeActivity.h | 4 + 4 files changed, 80 insertions(+), 96 deletions(-) diff --git a/lib/GfxRenderer/GfxRenderer.cpp b/lib/GfxRenderer/GfxRenderer.cpp index d2581b77..b0e4c1f8 100644 --- a/lib/GfxRenderer/GfxRenderer.cpp +++ b/lib/GfxRenderer/GfxRenderer.cpp @@ -577,38 +577,6 @@ void GfxRenderer::restoreBwBuffer() { Serial.printf("[%lu] [GFX] Restored and freed BW buffer chunks\n", millis()); } -/** - * Copy stored BW buffer to framebuffer without freeing the stored chunks. - * Use this when you want to restore the buffer but keep it for later reuse. - * Returns true if buffer was copied successfully. - */ -bool GfxRenderer::copyStoredBwBuffer() { - // Check if all chunks are allocated - for (const auto& bwBufferChunk : bwBufferChunks) { - if (!bwBufferChunk) { - return false; - } - } - - uint8_t* frameBuffer = einkDisplay.getFrameBuffer(); - if (!frameBuffer) { - return false; - } - - for (size_t i = 0; i < BW_BUFFER_NUM_CHUNKS; i++) { - const size_t offset = i * BW_BUFFER_CHUNK_SIZE; - memcpy(frameBuffer + offset, bwBufferChunks[i], BW_BUFFER_CHUNK_SIZE); - } - - return true; -} - -/** - * Free the stored BW buffer chunks manually. - * Use this when you no longer need the stored buffer. - */ -void GfxRenderer::freeStoredBwBuffer() { freeBwBufferChunks(); } - /** * Cleanup grayscale buffers using the current frame buffer. * Use this when BW buffer was re-rendered instead of stored/restored. diff --git a/lib/GfxRenderer/GfxRenderer.h b/lib/GfxRenderer/GfxRenderer.h index 6500a263..97663ab9 100644 --- a/lib/GfxRenderer/GfxRenderer.h +++ b/lib/GfxRenderer/GfxRenderer.h @@ -90,10 +90,8 @@ class GfxRenderer { void copyGrayscaleLsbBuffers() const; void copyGrayscaleMsbBuffers() const; void displayGrayBuffer() const; - bool storeBwBuffer(); // Returns true if buffer was stored successfully - void restoreBwBuffer(); // Restore and free the stored buffer - bool copyStoredBwBuffer(); // Copy stored buffer to framebuffer without freeing - void freeStoredBwBuffer(); // Free the stored buffer manually + bool storeBwBuffer(); // Returns true if buffer was stored successfully + void restoreBwBuffer(); // Restore and free the stored buffer void cleanupGrayscaleWithFrameBuffer() const; // Low level functions diff --git a/src/activities/home/HomeActivity.cpp b/src/activities/home/HomeActivity.cpp index 98d87f1d..27f9f74e 100644 --- a/src/activities/home/HomeActivity.cpp +++ b/src/activities/home/HomeActivity.cpp @@ -134,10 +134,49 @@ void HomeActivity::onExit() { renderingMutex = nullptr; // Free the stored cover buffer if any - if (coverBufferStored) { - renderer.freeStoredBwBuffer(); - coverBufferStored = false; + freeCoverBuffer(); +} + +bool HomeActivity::storeCoverBuffer() { + uint8_t* frameBuffer = renderer.getFrameBuffer(); + if (!frameBuffer) { + return false; } + + // Free any existing buffer first + freeCoverBuffer(); + + const size_t bufferSize = GfxRenderer::getBufferSize(); + coverBuffer = static_cast(malloc(bufferSize)); + if (!coverBuffer) { + return false; + } + + memcpy(coverBuffer, frameBuffer, bufferSize); + return true; +} + +bool HomeActivity::restoreCoverBuffer() { + if (!coverBuffer) { + return false; + } + + uint8_t* frameBuffer = renderer.getFrameBuffer(); + if (!frameBuffer) { + return false; + } + + const size_t bufferSize = GfxRenderer::getBufferSize(); + memcpy(frameBuffer, coverBuffer, bufferSize); + return true; +} + +void HomeActivity::freeCoverBuffer() { + if (coverBuffer) { + free(coverBuffer); + coverBuffer = nullptr; + } + coverBufferStored = false; } void HomeActivity::loop() { @@ -193,7 +232,7 @@ void HomeActivity::displayTaskLoop() { void HomeActivity::render() { // If we have a stored cover buffer, restore it instead of clearing - const bool bufferRestored = coverBufferStored && renderer.copyStoredBwBuffer(); + const bool bufferRestored = coverBufferStored && restoreCoverBuffer(); if (!bufferRestored) { renderer.clearScreen(); } @@ -252,85 +291,60 @@ void HomeActivity::render() { // Draw border around the card renderer.drawRect(bookX, bookY, bookWidth, bookHeight); - // Draw bookmark ribbon immediately after cover - const int notchDepth = bookmarkHeight / 3; - const int centerX = bookmarkX + bookmarkWidth / 2; + // No bookmark ribbon when cover is shown - it would just cover the art - const int xPoints[5] = { - bookmarkX, // top-left - bookmarkX + bookmarkWidth, // top-right - bookmarkX + bookmarkWidth, // bottom-right - centerX, // center notch point - bookmarkX // bottom-left - }; - const int yPoints[5] = { - bookmarkY, // top-left - bookmarkY, // top-right - bookmarkY + bookmarkHeight, // bottom-right - bookmarkY + bookmarkHeight - notchDepth, // center notch point - bookmarkY + bookmarkHeight // bottom-left - }; - - // Draw bookmark ribbon (white normally, will be inverted if selected) - renderer.fillPolygon(xPoints, yPoints, 5, false); - - // Store the buffer with cover image AND bookmark for fast navigation - coverBufferStored = renderer.storeBwBuffer(); + // Store the buffer with cover image for fast navigation + coverBufferStored = storeCoverBuffer(); coverRendered = true; // First render: if selected, draw selection indicators now if (bookSelected) { renderer.drawRect(bookX + 1, bookY + 1, bookWidth - 2, bookHeight - 2); renderer.drawRect(bookX + 2, bookY + 2, bookWidth - 4, bookHeight - 4); - // Invert bookmark to black - renderer.fillPolygon(xPoints, yPoints, 5, true); } } file.close(); } } else if (!bufferRestored && !coverRendered) { - // No cover image: draw border or fill + // No cover image: draw border or fill, plus bookmark as visual flair if (bookSelected) { renderer.fillRect(bookX, bookY, bookWidth, bookHeight); } else { renderer.drawRect(bookX, bookY, bookWidth, bookHeight); } + + // Draw bookmark ribbon when no cover image (visual decoration) + if (hasContinueReading) { + const int notchDepth = bookmarkHeight / 3; + const int centerX = bookmarkX + bookmarkWidth / 2; + + const int xPoints[5] = { + bookmarkX, // top-left + bookmarkX + bookmarkWidth, // top-right + bookmarkX + bookmarkWidth, // bottom-right + centerX, // center notch point + bookmarkX // bottom-left + }; + const int yPoints[5] = { + bookmarkY, // top-left + bookmarkY, // top-right + bookmarkY + bookmarkHeight, // bottom-right + bookmarkY + bookmarkHeight - notchDepth, // center notch point + bookmarkY + bookmarkHeight // bottom-left + }; + + // Draw bookmark ribbon (inverted if selected) + renderer.fillPolygon(xPoints, yPoints, 5, !bookSelected); + } } // If buffer was restored, draw selection indicators if needed if (bufferRestored && bookSelected && coverRendered) { - // Draw selection border + // Draw selection border (no bookmark inversion needed since cover has no bookmark) renderer.drawRect(bookX + 1, bookY + 1, bookWidth - 2, bookHeight - 2); renderer.drawRect(bookX + 2, bookY + 2, bookWidth - 4, bookHeight - 4); - - // Invert bookmark color when selected (draw black over the white bookmark) - const int notchDepth = bookmarkHeight / 3; - const int centerX = bookmarkX + bookmarkWidth / 2; - - const int xPoints[5] = { - bookmarkX, // top-left - bookmarkX + bookmarkWidth, // top-right - bookmarkX + bookmarkWidth, // bottom-right - centerX, // center notch point - bookmarkX // bottom-left - }; - const int yPoints[5] = { - bookmarkY, // top-left - bookmarkY, // top-right - bookmarkY + bookmarkHeight, // bottom-right - bookmarkY + bookmarkHeight - notchDepth, // center notch point - bookmarkY + bookmarkHeight // bottom-left - }; - - // Draw black filled bookmark ribbon (inverted) - renderer.fillPolygon(xPoints, yPoints, 5, true); - } else if (!coverRendered) { - // No cover: draw border for non-cover case - renderer.drawRect(bookX, bookY, bookWidth, bookHeight); - if (bookSelected) { - renderer.drawRect(bookX + 1, bookY + 1, bookWidth - 2, bookHeight - 2); - renderer.drawRect(bookX + 2, bookY + 2, bookWidth - 4, bookHeight - 4); - } + } else if (!coverRendered && !bufferRestored) { + // Selection border already handled above in the no-cover case } } diff --git a/src/activities/home/HomeActivity.h b/src/activities/home/HomeActivity.h index a9c95233..b35dd334 100644 --- a/src/activities/home/HomeActivity.h +++ b/src/activities/home/HomeActivity.h @@ -16,6 +16,7 @@ class HomeActivity final : public Activity { bool hasCoverImage = false; bool coverRendered = false; // Track if cover has been rendered once bool coverBufferStored = false; // Track if cover buffer is stored + uint8_t* coverBuffer = nullptr; // HomeActivity's own buffer for cover image std::string lastBookTitle; std::string lastBookAuthor; std::string coverBmpPath; @@ -28,6 +29,9 @@ class HomeActivity final : public Activity { [[noreturn]] void displayTaskLoop(); void render(); int getMenuItemCount() const; + bool storeCoverBuffer(); // Store frame buffer for cover image + bool restoreCoverBuffer(); // Restore frame buffer from stored cover + void freeCoverBuffer(); // Free the stored cover buffer public: explicit HomeActivity(GfxRenderer& renderer, MappedInputManager& mappedInput,