Review comments

2026-03-04 09:40:43 +01:00
parent 19b3bdf8ea
commit 9b819b99d4
1 changed files with 75 additions and 145 deletions
@@ -21,6 +21,61 @@ constexpr size_t CHUNK_SIZE = 8 * 1024;  // 8KB chunk for reading
 constexpr uint32_t CACHE_MAGIC = 0x54585449;  // "TXTI"
 constexpr uint8_t CACHE_VERSION = 2;          // Increment when cache format changes
 constexpr uint32_t MAX_CACHE_PAGES = 65535;   // Sanity cap to prevent unbounded reserve()
+
+// Parses and word-wraps lines from a file chunk into outLines.
+// Returns the number of bytes consumed from the start of buffer.
+size_t parseAndWrapLines(const uint8_t* buffer, size_t chunkSize, size_t fileOffset, size_t fileSize, int linesPerPage,
+                         GfxRenderer& renderer, int fontId, int vw, std::vector<std::string>& outLines) {
+  size_t pos = 0;
+  while (pos < chunkSize && static_cast<int>(outLines.size()) < linesPerPage) {
+    size_t lineEnd = pos;
+    while (lineEnd < chunkSize && buffer[lineEnd] != '\n') lineEnd++;
+    bool lineComplete = (lineEnd < chunkSize) || (fileOffset + lineEnd >= fileSize);
+    if (!lineComplete && !outLines.empty()) break;
+
+    size_t lineContentLen = lineEnd - pos;
+    bool hasCR = (lineContentLen > 0 && buffer[pos + lineContentLen - 1] == '\r');
+    size_t displayLen = hasCR ? lineContentLen - 1 : lineContentLen;
+    std::string line(reinterpret_cast<const char*>(buffer + pos), displayLen);
+    size_t lineBytePos = 0;
+
+    while (!line.empty() && static_cast<int>(outLines.size()) < linesPerPage) {
+      if (renderer.getTextWidth(fontId, line.c_str()) <= vw) {
+        outLines.push_back(line);
+        lineBytePos = displayLen;
+        line.clear();
+        break;
+      }
+      size_t breakPos = line.length();
+      while (breakPos > 0 && renderer.getTextWidth(fontId, line.substr(0, breakPos).c_str()) > vw) {
+        size_t spacePos = line.rfind(' ', breakPos - 1);
+        if (spacePos != std::string::npos && spacePos > 0) {
+          breakPos = spacePos;
+        } else {
+          breakPos--;
+          while (breakPos > 0 && (line[breakPos] & 0xC0) == 0x80) breakPos--;
+        }
+      }
+      if (breakPos == 0) breakPos = 1;
+      outLines.push_back(line.substr(0, breakPos));
+      size_t skipChars = breakPos;
+      if (breakPos < line.length() && line[breakPos] == ' ') skipChars++;
+      lineBytePos += skipChars;
+      line = line.substr(skipChars);
+    }
+
+    if (line.empty()) {
+      pos = lineEnd + 1;
+    } else {
+      pos = pos + lineBytePos;
+      break;
+    }
+  }
+  if (pos == 0 && !outLines.empty()) {
+    pos = 1;
+  }
+  return pos;
+}
 }  // namespace

 void TxtReaderActivity::onEnter() {
@@ -217,101 +272,9 @@ bool TxtReaderActivity::loadPageAtOffset(size_t offset, std::vector<std::string>
  }
  buffer[chunkSize] = '\0';

-  // Parse lines from buffer
-  size_t pos = 0;
-
-  while (pos < chunkSize && static_cast<int>(outLines.size()) < linesPerPage) {
-    // Find end of line
-    size_t lineEnd = pos;
-    while (lineEnd < chunkSize && buffer[lineEnd] != '\n') {
-      lineEnd++;
-    }
-
-    // Check if we have a complete line
-    bool lineComplete = (lineEnd < chunkSize) || (offset + lineEnd >= fileSize);
-
-    if (!lineComplete && static_cast<int>(outLines.size()) > 0) {
-      // Incomplete line and we already have some lines, stop here
-      break;
-    }
-
-    // Calculate the actual length of line content in the buffer (excluding newline)
-    size_t lineContentLen = lineEnd - pos;
-
-    // Check for carriage return
-    bool hasCR = (lineContentLen > 0 && buffer[pos + lineContentLen - 1] == '\r');
-    size_t displayLen = hasCR ? lineContentLen - 1 : lineContentLen;
-
-    // Extract line content for display (without CR/LF)
-    std::string line(reinterpret_cast<char*>(buffer + pos), displayLen);
-
-    // Track position within this source line (in bytes from pos)
-    size_t lineBytePos = 0;
-
-    // Word wrap if needed
-    while (!line.empty() && static_cast<int>(outLines.size()) < linesPerPage) {
-      int lineWidth = renderer.getTextWidth(cachedFontId, line.c_str());
-
-      if (lineWidth <= viewportWidth) {
-        outLines.push_back(line);
-        lineBytePos = displayLen;  // Consumed entire display content
-        line.clear();
-        break;
-      }
-
-      // Find break point
-      size_t breakPos = line.length();
-      while (breakPos > 0 && renderer.getTextWidth(cachedFontId, line.substr(0, breakPos).c_str()) > viewportWidth) {
-        // Try to break at space
-        size_t spacePos = line.rfind(' ', breakPos - 1);
-        if (spacePos != std::string::npos && spacePos > 0) {
-          breakPos = spacePos;
-        } else {
-          // Break at character boundary for UTF-8
-          breakPos--;
-          // Make sure we don't break in the middle of a UTF-8 sequence
-          while (breakPos > 0 && (line[breakPos] & 0xC0) == 0x80) {
-            breakPos--;
-          }
-        }
-      }
-
-      if (breakPos == 0) {
-        breakPos = 1;
-      }
-
-      outLines.push_back(line.substr(0, breakPos));
-
-      // Skip space at break point
-      size_t skipChars = breakPos;
-      if (breakPos < line.length() && line[breakPos] == ' ') {
-        skipChars++;
-      }
-      lineBytePos += skipChars;
-      line = line.substr(skipChars);
-    }
-
-    // Determine how much of the source buffer we consumed
-    if (line.empty()) {
-      // Fully consumed this source line, move past the newline
-      pos = lineEnd + 1;
-    } else {
-      // Partially consumed - page is full mid-line
-      // Move pos to where we stopped in the line (NOT past the line)
-      pos = pos + lineBytePos;
-      break;
-    }
-  }
-
-  // Ensure we make progress even if calculations go wrong
-  if (pos == 0 && !outLines.empty()) {
-    // Fallback: at minimum, consume something to avoid infinite loop
-    pos = 1;
-  }
-
+  size_t pos = parseAndWrapLines(buffer, chunkSize, offset, fileSize, linesPerPage, renderer, cachedFontId,
+                                 viewportWidth, outLines);
  nextOffset = offset + pos;
-
-  // Make sure we don't go past the file
  if (nextOffset > fileSize) {
    nextOffset = fileSize;
  }
@@ -557,6 +520,10 @@ bool TxtReaderActivity::loadPageIndexCache() {

  uint32_t numPages;
  serialization::readPod(f, numPages);
+  if (numPages > MAX_CACHE_PAGES) {
+    LOG_WRN("TRS", "Cache numPages %u exceeds cap %u, truncating", numPages, MAX_CACHE_PAGES);
+    numPages = MAX_CACHE_PAGES;
+  }

  // Read page offsets
  pageOffsets.clear();
@@ -698,16 +665,7 @@ bool TxtReaderActivity::drawCurrentPageToBuffer(const std::string& filePath, Gfx
    return false;
  }

-  std::vector<size_t> pageOffsets;
-  pageOffsets.reserve(numPages);
-  for (uint32_t i = 0; i < numPages; i++) {
-    uint32_t offset;
-    serialization::readPod(cacheFile, offset);
-    pageOffsets.push_back(offset);
-  }
-  cacheFile.close();
-
-  // Load saved page number from progress file
+  // Load saved page number before reading offsets
  int savedPage = 0;
  FsFile progFile;
  if (Storage.openFileForRead("SLP", txt.getCachePath() + "/progress.bin", progFile)) {
@@ -719,16 +677,26 @@ bool TxtReaderActivity::drawCurrentPageToBuffer(const std::string& filePath, Gfx
  }
  if (savedPage < 0 || savedPage >= static_cast<int>(numPages)) savedPage = 0;

+  // Read offsets sequentially, retaining only the one we need
+  size_t savedOffset = 0;
+  for (uint32_t i = 0; i < numPages; i++) {
+    uint32_t off;
+    serialization::readPod(cacheFile, off);
+    if (static_cast<int>(i) == savedPage) {
+      savedOffset = off;
+    }
+  }
+  cacheFile.close();
+
  // Load the page lines from file
  std::vector<std::string> pageLines;
  const size_t fileSize = txt.getFileSize();
-  size_t offset = pageOffsets[savedPage];
+  size_t offset = savedOffset;
  if (offset >= fileSize) {
    LOG_DBG("SLP", "TXT: page offset out of bounds");
    return false;
  }

-  // Replicate loadPageAtOffset() logic with local layout variables
  size_t chunkSize = std::min(CHUNK_SIZE, fileSize - offset);
  auto* buffer = static_cast<uint8_t*>(malloc(chunkSize + 1));
  if (!buffer) return false;
@@ -739,45 +707,7 @@ bool TxtReaderActivity::drawCurrentPageToBuffer(const std::string& filePath, Gfx
  }
  buffer[chunkSize] = '\0';

-  size_t pos = 0;
-  while (pos < chunkSize && static_cast<int>(pageLines.size()) < linesPerPage) {
-    size_t lineEnd = pos;
-    while (lineEnd < chunkSize && buffer[lineEnd] != '\n') lineEnd++;
-    bool lineComplete = (lineEnd < chunkSize) || (offset + lineEnd >= fileSize);
-    if (!lineComplete && !pageLines.empty()) break;
-
-    size_t lineContentLen = lineEnd - pos;
-    bool hasCR = (lineContentLen > 0 && buffer[pos + lineContentLen - 1] == '\r');
-    size_t displayLen = hasCR ? lineContentLen - 1 : lineContentLen;
-    std::string line(reinterpret_cast<char*>(buffer + pos), displayLen);
-    size_t lineBytePos = 0;
-
-    while (!line.empty() && static_cast<int>(pageLines.size()) < linesPerPage) {
-      if (renderer.getTextWidth(fontId, line.c_str()) <= vw) {
-        pageLines.push_back(line);
-        lineBytePos = displayLen;
-        line.clear();
-        break;
-      }
-      size_t breakPos = line.length();
-      while (breakPos > 0 && renderer.getTextWidth(fontId, line.substr(0, breakPos).c_str()) > vw) {
-        size_t spacePos = line.rfind(' ', breakPos - 1);
-        if (spacePos != std::string::npos && spacePos > 0) {
-          breakPos = spacePos;
-        } else {
-          breakPos--;
-          while (breakPos > 0 && (line[breakPos] & 0xC0) == 0x80) breakPos--;
-        }
-      }
-      if (breakPos == 0) breakPos = 1;
-      pageLines.push_back(line.substr(0, breakPos));
-      size_t skipChars = breakPos;
-      if (breakPos < line.length() && line[breakPos] == ' ') skipChars++;
-      lineBytePos += skipChars;
-      line = line.substr(skipChars);
-    }
-    pos = line.empty() ? lineEnd + 1 : pos + lineBytePos;
-  }
+  parseAndWrapLines(buffer, chunkSize, offset, fileSize, linesPerPage, renderer, fontId, vw, pageLines);
  free(buffer);

  if (pageLines.empty()) return false;