From 8a2bc9f4257b1f11d279b9813ba3ec2ac956af26 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Tue, 28 Apr 2026 15:47:51 +0200 Subject: [PATCH] Some review comments --- lib/EpdFont/SdCardFont.cpp | 24 ++++++++++++-- lib/EpdFont/SdCardFontManager.cpp | 10 +----- lib/EpdFont/SdCardFontRegistry.cpp | 15 +++++++++ lib/EpdFont/SdCardFontRegistry.h | 5 +++ lib/EpdFont/scripts/fontconvert_sdcard.py | 9 +++-- src/SdCardFontSystem.cpp | 11 ++----- .../reader/EpubReaderMenuActivity.cpp | 33 +++++++++++++++---- 7 files changed, 77 insertions(+), 30 deletions(-) diff --git a/lib/EpdFont/SdCardFont.cpp b/lib/EpdFont/SdCardFont.cpp index 38081970..2d29a13e 100644 --- a/lib/EpdFont/SdCardFont.cpp +++ b/lib/EpdFont/SdCardFont.cpp @@ -432,7 +432,14 @@ bool SdCardFont::load(const char* path) { return false; } - // Begin content hash: accumulate global header + // Begin content hash: accumulate global header. + // KNOWN LIMITATION: hash covers global header + per-style TOC only, not the + // payload sections (intervals / glyph metrics / kern / ligature / bitmap). A + // font edit that alters payload bytes without changing any TOC count would + // produce the same contentHash and could leave stale EPUB section caches. + // Acceptable in practice because generate-sd-fonts.sh regeneration almost + // always changes interval/glyph/kern counts; revisit if we see real-world + // mismatches. uint32_t hash = fnv1a(headerBuf, HEADER_SIZE); bool is2Bit = (readU16(headerBuf + 10) & 1) != 0; @@ -1214,7 +1221,12 @@ const EpdGlyph* SdCardFont::onGlyphMiss(void* ctx, uint32_t codepoint) { EpdGlyph tempGlyph; uint32_t glyphFileOff = s.glyphsFileOffset + static_cast(globalIdx) * sizeof(EpdGlyph); - file.seekSet(glyphFileOff); + if (!file.seekSet(glyphFileOff)) { + LOG_ERR("SDCF", "Overflow: seek failed for glyph metadata U+%04X style %u", codepoint, styleIdx); + file.close(); + if (!wasAtCapacity) self->overflowCount_--; + return nullptr; + } if (file.read(reinterpret_cast(&tempGlyph), sizeof(EpdGlyph)) != sizeof(EpdGlyph)) { LOG_ERR("SDCF", "Overflow: failed to read glyph metadata for U+%04X style %u", codepoint, styleIdx); file.close(); @@ -1232,7 +1244,13 @@ const EpdGlyph* SdCardFont::onGlyphMiss(void* ctx, uint32_t codepoint) { if (!wasAtCapacity) self->overflowCount_--; return nullptr; } - file.seekSet(s.bitmapFileOffset + tempGlyph.dataOffset); + if (!file.seekSet(s.bitmapFileOffset + tempGlyph.dataOffset)) { + LOG_ERR("SDCF", "Overflow: seek failed for bitmap U+%04X style %u", codepoint, styleIdx); + delete[] tempBitmap; + file.close(); + if (!wasAtCapacity) self->overflowCount_--; + return nullptr; + } if (file.read(tempBitmap, tempGlyph.dataLength) != static_cast(tempGlyph.dataLength)) { LOG_ERR("SDCF", "Overflow: failed to read bitmap for U+%04X", codepoint); delete[] tempBitmap; diff --git a/lib/EpdFont/SdCardFontManager.cpp b/lib/EpdFont/SdCardFontManager.cpp index f6c51bfb..5b8a2da3 100644 --- a/lib/EpdFont/SdCardFontManager.cpp +++ b/lib/EpdFont/SdCardFontManager.cpp @@ -39,15 +39,7 @@ bool SdCardFontManager::loadFamily(const SdCardFontFamilyInfo& family, GfxRender // Pick the single file whose size is closest to targetPtSize. Loading // only one size bounds resident memory (intervals + kern/ligature tables // per style) to one file's worth, vs. N_sizes × per-file overhead. - const SdCardFontFileInfo* selected = nullptr; - int bestDiff = INT32_MAX; - for (const auto& fileInfo : family.files) { - int diff = std::abs(static_cast(fileInfo.pointSize) - static_cast(targetPtSize)); - if (diff < bestDiff) { - bestDiff = diff; - selected = &fileInfo; - } - } + const SdCardFontFileInfo* selected = family.pickClosestSize(targetPtSize); if (!selected) { LOG_ERR("SDMGR", "Family %s has no files to load", family.name.c_str()); return false; diff --git a/lib/EpdFont/SdCardFontRegistry.cpp b/lib/EpdFont/SdCardFontRegistry.cpp index 8a162f48..099732f2 100644 --- a/lib/EpdFont/SdCardFontRegistry.cpp +++ b/lib/EpdFont/SdCardFontRegistry.cpp @@ -38,6 +38,21 @@ std::vector SdCardFontFamilyInfo::availableSizes() const { return sizes; } +const SdCardFontFileInfo* SdCardFontFamilyInfo::pickClosestSize(uint8_t targetPtSize) const { + const SdCardFontFileInfo* selected = nullptr; + int bestDiff = INT32_MAX; + for (const auto& f : files) { + int diff = std::abs(static_cast(f.pointSize) - static_cast(targetPtSize)); + // Strict < ensures the first scan wins on ties; then tie-break by smaller + // pointSize to make the choice independent of filesystem enumeration order. + if (diff < bestDiff || (diff == bestDiff && selected && f.pointSize < selected->pointSize)) { + bestDiff = diff; + selected = &f; + } + } + return selected; +} + // --- SdCardFontRegistry --- bool SdCardFontRegistry::parseFilename(const char* filename, uint8_t& size, uint8_t& style) { diff --git a/lib/EpdFont/SdCardFontRegistry.h b/lib/EpdFont/SdCardFontRegistry.h index 941668cc..ee8c34bb 100644 --- a/lib/EpdFont/SdCardFontRegistry.h +++ b/lib/EpdFont/SdCardFontRegistry.h @@ -19,6 +19,11 @@ struct SdCardFontFamilyInfo { const SdCardFontFileInfo* findFile(uint8_t size, uint8_t style = 0) const; bool hasSize(uint8_t size) const; std::vector availableSizes() const; + + // Pick the file whose pointSize is closest to targetPtSize. On ties (equal + // distance) prefers the smaller pointSize so behaviour is deterministic + // across SD card layouts. Returns nullptr when files is empty. + const SdCardFontFileInfo* pickClosestSize(uint8_t targetPtSize) const; }; class SdCardFontRegistry { diff --git a/lib/EpdFont/scripts/fontconvert_sdcard.py b/lib/EpdFont/scripts/fontconvert_sdcard.py index 3fa36d1e..84015150 100644 --- a/lib/EpdFont/scripts/fontconvert_sdcard.py +++ b/lib/EpdFont/scripts/fontconvert_sdcard.py @@ -451,6 +451,12 @@ def rasterize_font_style(fontfile, size, intervals, style_id=0, force_autohint=F style_label = style_names.get(style_id, str(style_id)) face = freetype.Face(fontfile) + # Set font size at 150 DPI (matching fontconvert.py) BEFORE any glyph load + # — load_glyph() with FT_LOAD_RENDER renders at the active size, so calling + # it before set_char_size() would waste work at the default size and risk + # Invalid_Size_Handle on some fonts. + face.set_char_size(size << 6, size << 6, 150, 150) + load_flags = freetype.FT_LOAD_RENDER if force_autohint: load_flags |= freetype.FT_LOAD_FORCE_AUTOHINT @@ -480,9 +486,6 @@ def rasterize_font_style(fontfile, size, intervals, style_id=0, force_autohint=F total_glyphs = sum(end - start + 1 for start, end in intervals) print(f" [{style_label}] Validated: {len(intervals)} intervals, {total_glyphs} glyphs", file=sys.stderr) - # Set font size at 150 DPI (matching fontconvert.py) - face.set_char_size(size << 6, size << 6, 150, 150) - # Rasterize all glyphs total_bitmap_size = 0 all_glyphs = [] diff --git a/src/SdCardFontSystem.cpp b/src/SdCardFontSystem.cpp index e471c395..35265b09 100644 --- a/src/SdCardFontSystem.cpp +++ b/src/SdCardFontSystem.cpp @@ -120,15 +120,8 @@ void SdCardFontSystem::ensureLoaded(GfxRenderer& renderer) { SETTINGS.sdFontFamilyName[0] = '\0'; return; } - uint8_t bestPt = 0; - int bestDiff = INT32_MAX; - for (const auto& f : family->files) { - int diff = abs(static_cast(f.pointSize) - static_cast(targetPt)); - if (diff < bestDiff) { - bestDiff = diff; - bestPt = f.pointSize; - } - } + const auto* best = family->pickClosestSize(targetPt); + const uint8_t bestPt = best ? best->pointSize : 0; if (bestPt == manager_.currentPointSize()) return; // already loaded with the right size LOG_DBG("SDFS", "Reloading %s: size %u -> %u (target %u)", wantedFamily, manager_.currentPointSize(), bestPt, targetPt); diff --git a/src/activities/reader/EpubReaderMenuActivity.cpp b/src/activities/reader/EpubReaderMenuActivity.cpp index bb621e54..8f42e97e 100644 --- a/src/activities/reader/EpubReaderMenuActivity.cpp +++ b/src/activities/reader/EpubReaderMenuActivity.cpp @@ -5,10 +5,31 @@ #include "KOReaderCredentialStore.h" #include "MappedInputManager.h" +#include "SdCardFontGlobals.h" #include "activities/settings/SettingsSubmenuActivity.h" #include "components/UITheme.h" #include "fontIds.h" +namespace { +// Returns the localized name of the family currently used as the global default +// for the reader. When the user has selected an SD card font globally, the +// override menu's "Default" label should reflect that family by name even +// though the per-book override list itself is built-in only. +std::string defaultFontFamilyLabel(const SettingInfo& item) { + if (SETTINGS.sdFontFamilyName[0] != '\0') { + return std::string(SETTINGS.sdFontFamilyName); + } + // Built-in: enumValues[0] is STR_DEFAULT_VALUE, [1..3] are the three families + // in the same order as CrossPointSettings::FONT_FAMILY (BOOKERLY, NOTOSANS, + // OPENDYSLEXIC). + const auto idx = static_cast(SETTINGS.fontFamily + 1); + if (idx < item.enumValues.size()) { + return I18N.get(item.enumValues[idx]); + } + return {}; +} +} // namespace + EpubReaderMenuActivity::EpubReaderMenuActivity(GfxRenderer& renderer, MappedInputManager& mappedInput, const std::string& title, const int currentPage, const int totalPages, const int bookProgressPercent, const uint8_t currentOrientation, @@ -290,9 +311,9 @@ std::string EpubReaderMenuActivity::getItemValueString(int index) const { } } if (item.nameId == StrId::STR_FONT_FAMILY && pendingFontFamilyOverride < 0) { - const auto defaultIndex = static_cast(SETTINGS.fontFamily + 1); - if (defaultIndex < item.enumValues.size()) { - return std::string(tr(STR_DEFAULT_VALUE)) + " (" + I18N.get(item.enumValues[defaultIndex]) + ")"; + const auto label = defaultFontFamilyLabel(item); + if (!label.empty()) { + return std::string(tr(STR_DEFAULT_VALUE)) + " (" + label + ")"; } } if (item.nameId == StrId::STR_FONT_SIZE && pendingFontSizeOverride < 0) { @@ -324,9 +345,9 @@ void EpubReaderMenuActivity::openSubmenu(const SettingInfo& submenuEntry) { } } if (item.nameId == StrId::STR_FONT_FAMILY && pendingFontFamilyOverride < 0) { - const auto valueIndex = static_cast(SETTINGS.fontFamily + 1); - if (valueIndex < item.enumValues.size()) { - return std::string(tr(STR_DEFAULT_VALUE)) + " (" + I18N.get(item.enumValues[valueIndex]) + ")"; + const auto label = defaultFontFamilyLabel(item); + if (!label.empty()) { + return std::string(tr(STR_DEFAULT_VALUE)) + " (" + label + ")"; } } if (item.nameId == StrId::STR_FONT_SIZE && pendingFontSizeOverride < 0) {