From 1e9ecf26c2f4234926c2a2377868d1be6e6bd474 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Thu, 18 Jun 2026 11:47:05 +1000 Subject: [PATCH 1/3] refactor: extract ITextMetrics and detach TextBlock.h from HAL for host testing --- lib/Epub/Epub/ParsedText.cpp | 18 +++++++++--------- lib/Epub/Epub/ParsedText.h | 16 ++++++++-------- lib/Epub/Epub/blocks/TextBlock.cpp | 1 + lib/Epub/Epub/blocks/TextBlock.h | 5 ++++- lib/GfxRenderer/GfxRenderer.h | 18 ++++++++++-------- lib/GfxRenderer/ITextMetrics.h | 27 +++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 26 deletions(-) create mode 100644 lib/GfxRenderer/ITextMetrics.h diff --git a/lib/Epub/Epub/ParsedText.cpp b/lib/Epub/Epub/ParsedText.cpp index b66b36ee38..219eb74bd2 100644 --- a/lib/Epub/Epub/ParsedText.cpp +++ b/lib/Epub/Epub/ParsedText.cpp @@ -1,7 +1,7 @@ #include "ParsedText.h" #include -#include +#include #include #include @@ -68,7 +68,7 @@ void stripSoftHyphensInPlace(std::string& word) { // Returns the advance width for a word while ignoring soft hyphen glyphs and optionally appending a visible hyphen. // Uses advance width (sum of glyph advances + kerning) rather than bounding box width so that italic glyph overhangs // don't inflate inter-word spacing. -uint16_t measureWordWidth(const GfxRenderer& renderer, const int fontId, const std::string& word, +uint16_t measureWordWidth(const ITextMetrics& renderer, const int fontId, const std::string& word, const EpdFontFamily::Style style, const bool appendHyphen = false) { if (word.size() == 1 && word[0] == ' ' && !appendHyphen) { return renderer.getSpaceWidth(fontId, style); @@ -261,7 +261,7 @@ void ParsedText::addWord(std::string word, const EpdFontFamily::Style fontStyle, } } -int ParsedText::resolveFirstLineIndent(const bool isFirstLine, const GfxRenderer& renderer, const int fontId) const { +int ParsedText::resolveFirstLineIndent(const bool isFirstLine, const ITextMetrics& renderer, const int fontId) const { if (!isFirstLine || !isNaturalAlign) { return 0; } @@ -277,7 +277,7 @@ int ParsedText::resolveFirstLineIndent(const bool isFirstLine, const GfxRenderer return 0; } // Consumes data to minimize memory usage -void ParsedText::layoutAndExtractLines(const GfxRenderer& renderer, const int fontId, const uint16_t viewportWidth, +void ParsedText::layoutAndExtractLines(const ITextMetrics& renderer, const int fontId, const uint16_t viewportWidth, const std::function)>& processLine, const bool includeLastLine) { if (words.empty()) { @@ -344,7 +344,7 @@ void ParsedText::layoutAndExtractLines(const GfxRenderer& renderer, const int fo } } -std::vector ParsedText::calculateWordWidths(const GfxRenderer& renderer, const int fontId) { +std::vector ParsedText::calculateWordWidths(const ITextMetrics& renderer, const int fontId) { std::vector wordWidths; wordWidths.reserve(words.size()); @@ -355,7 +355,7 @@ std::vector ParsedText::calculateWordWidths(const GfxRenderer& rendere return wordWidths; } -std::vector ParsedText::computeLineBreaks(const GfxRenderer& renderer, const int fontId, const int pageWidth, +std::vector ParsedText::computeLineBreaks(const ITextMetrics& renderer, const int fontId, const int pageWidth, std::vector& wordWidths, std::vector& continuesVec) { if (words.empty()) { return {}; @@ -468,7 +468,7 @@ std::vector ParsedText::computeLineBreaks(const GfxRenderer& renderer, c } // Builds break indices while opportunistically splitting the word that would overflow the current line. -std::vector ParsedText::computeHyphenatedLineBreaks(const GfxRenderer& renderer, const int fontId, +std::vector ParsedText::computeHyphenatedLineBreaks(const ITextMetrics& renderer, const int fontId, const int pageWidth, std::vector& wordWidths, std::vector& continuesVec) { const int firstLineIndent = resolveFirstLineIndent(true, renderer, fontId); @@ -540,7 +540,7 @@ std::vector ParsedText::computeHyphenatedLineBreaks(const GfxRenderer& r // Splits words[wordIndex] into prefix (adding a hyphen only when needed) and remainder when a legal breakpoint fits the // available width. -bool ParsedText::hyphenateWordAtIndex(const size_t wordIndex, const int availableWidth, const GfxRenderer& renderer, +bool ParsedText::hyphenateWordAtIndex(const size_t wordIndex, const int availableWidth, const ITextMetrics& renderer, const int fontId, std::vector& wordWidths, const bool allowFallbackBreaks) { // Guard against invalid indices or zero available width before attempting to split. @@ -629,7 +629,7 @@ bool ParsedText::hyphenateWordAtIndex(const size_t wordIndex, const int availabl void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const std::vector& wordWidths, const std::vector& continuesVec, const std::vector& lineBreakIndices, const std::function)>& processLine, - const GfxRenderer& renderer, const int fontId) { + const ITextMetrics& renderer, const int fontId) { const size_t lineBreak = lineBreakIndices[breakIndex]; const size_t lastBreakAt = breakIndex > 0 ? lineBreakIndices[breakIndex - 1] : 0; const size_t lineWordCount = lineBreak - lastBreakAt; diff --git a/lib/Epub/Epub/ParsedText.h b/lib/Epub/Epub/ParsedText.h index 9c3af7cb6e..5f7f854c09 100644 --- a/lib/Epub/Epub/ParsedText.h +++ b/lib/Epub/Epub/ParsedText.h @@ -10,7 +10,7 @@ #include "blocks/BlockStyle.h" #include "blocks/TextBlock.h" -class GfxRenderer; +class ITextMetrics; class ParsedText { std::vector words; @@ -30,18 +30,18 @@ class ParsedText { std::vector reorderedFocusSuffixScratch; std::vector visualOrderScratch; - int resolveFirstLineIndent(bool isFirstLine, const GfxRenderer& renderer, int fontId) const; - std::vector computeLineBreaks(const GfxRenderer& renderer, int fontId, int pageWidth, + int resolveFirstLineIndent(bool isFirstLine, const ITextMetrics& renderer, int fontId) const; + std::vector computeLineBreaks(const ITextMetrics& renderer, int fontId, int pageWidth, std::vector& wordWidths, std::vector& continuesVec); - std::vector computeHyphenatedLineBreaks(const GfxRenderer& renderer, int fontId, int pageWidth, + std::vector computeHyphenatedLineBreaks(const ITextMetrics& renderer, int fontId, int pageWidth, std::vector& wordWidths, std::vector& continuesVec); - bool hyphenateWordAtIndex(size_t wordIndex, int availableWidth, const GfxRenderer& renderer, int fontId, + bool hyphenateWordAtIndex(size_t wordIndex, int availableWidth, const ITextMetrics& renderer, int fontId, std::vector& wordWidths, bool allowFallbackBreaks); void extractLine(size_t breakIndex, int pageWidth, const std::vector& wordWidths, const std::vector& continuesVec, const std::vector& lineBreakIndices, - const std::function)>& processLine, const GfxRenderer& renderer, + const std::function)>& processLine, const ITextMetrics& renderer, int fontId); - std::vector calculateWordWidths(const GfxRenderer& renderer, int fontId); + std::vector calculateWordWidths(const ITextMetrics& renderer, int fontId); public: explicit ParsedText(const bool extraParagraphSpacing, const bool hyphenationEnabled = false, @@ -59,7 +59,7 @@ class ParsedText { BlockStyle& getBlockStyle() { return blockStyle; } size_t size() const { return words.size(); } bool isEmpty() const { return words.empty(); } - void layoutAndExtractLines(const GfxRenderer& renderer, int fontId, uint16_t viewportWidth, + void layoutAndExtractLines(const ITextMetrics& renderer, int fontId, uint16_t viewportWidth, const std::function)>& processLine, bool includeLastLine = true); }; diff --git a/lib/Epub/Epub/blocks/TextBlock.cpp b/lib/Epub/Epub/blocks/TextBlock.cpp index 9260fa32ce..4f60c70ca4 100644 --- a/lib/Epub/Epub/blocks/TextBlock.cpp +++ b/lib/Epub/Epub/blocks/TextBlock.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include diff --git a/lib/Epub/Epub/blocks/TextBlock.h b/lib/Epub/Epub/blocks/TextBlock.h index 5f4bf80e1c..686c0fa719 100644 --- a/lib/Epub/Epub/blocks/TextBlock.h +++ b/lib/Epub/Epub/blocks/TextBlock.h @@ -1,6 +1,7 @@ #pragma once #include -#include + +class HalFile; #include #include @@ -42,6 +43,8 @@ class TextBlock final : public Block { void setBlockStyle(const BlockStyle& blockStyle) { this->blockStyle = blockStyle; } const BlockStyle& getBlockStyle() const { return blockStyle; } const std::vector& getWords() const { return words; } + const std::vector& getWordStyles() const { return wordStyles; } + const std::vector& getWordXpos() const { return wordXpos; } bool isEmpty() override { return words.empty(); } size_t wordCount() const { return words.size(); } // given a renderer works out where to break the words into lines diff --git a/lib/GfxRenderer/GfxRenderer.h b/lib/GfxRenderer/GfxRenderer.h index 924e5c0b68..46b319fc0c 100644 --- a/lib/GfxRenderer/GfxRenderer.h +++ b/lib/GfxRenderer/GfxRenderer.h @@ -3,6 +3,8 @@ #include #include +#include "ITextMetrics.h" + namespace BidiUtils { // Paragraph base direction for the Unicode BiDi algorithm (UAX#9). // AUTO: scan text for first strong directional character (P2/P3 rules) @@ -25,7 +27,7 @@ class SdCardFont; // 0 = transparent, 1-16 = gray levels (white to black) enum Color : uint8_t { Clear = 0x00, White = 0x01, LightGray = 0x05, DarkGray = 0x0A, Black = 0x10 }; -class GfxRenderer { +class GfxRenderer : public ITextMetrics { public: enum RenderMode { BW, GRAYSCALE_LSB, GRAYSCALE_MSB }; @@ -110,13 +112,13 @@ class GfxRenderer { void unregisterSdCardFont(int fontId) { removeFont(fontId); } void clearSdCardFonts() { sdCardFonts_.clear(); } const std::map& getSdCardFonts() const { return sdCardFonts_; } - bool isSdCardFont(int fontId) const { return sdCardFonts_.count(fontId) > 0; } + bool isSdCardFont(int fontId) const override { return sdCardFonts_.count(fontId) > 0; } // Ensure SD card font glyph data is loaded for the given text. Called from layout code // (which holds a const GfxRenderer&) before measuring word widths. Safe to call on non-SD fonts (no-op). // styleMask: bitmask of styles to prepare (bit 0=regular, 1=bold, 2=italic, 3=bold-italic). - void ensureSdCardFontReady(int fontId, const char* utf8Text, uint8_t styleMask = 0x0F) const; + void ensureSdCardFontReady(int fontId, const char* utf8Text, uint8_t styleMask = 0x0F) const override; void ensureSdCardFontReady(int fontId, const std::vector& words, bool includeHyphen, - uint8_t styleMask = 0x0F) const; + uint8_t styleMask = 0x0F) const override; // Orientation control (affects logical width/height and coordinate transforms) void setOrientation(const Orientation o) { orientation = o; } @@ -192,14 +194,14 @@ class GfxRenderer { void drawText(int fontId, int x, int y, const char* text, bool black = true, EpdFontFamily::Style style = EpdFontFamily::REGULAR, BidiUtils::BidiBaseDir baseDir = BidiUtils::BidiBaseDir::AUTO) const; - int getSpaceWidth(int fontId, EpdFontFamily::Style style = EpdFontFamily::REGULAR) const; + int getSpaceWidth(int fontId, EpdFontFamily::Style style = EpdFontFamily::REGULAR) const override; /// Returns the total inter-word advance: fp4::toPixel(spaceAdvance + kern(leftCp,' ') + kern(' ',rightCp)). /// Using a single snap avoids the +/-1 px rounding error that arises when space advance and kern are /// snapped separately and then added as integers. - int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const; + int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const override; /// Returns the kerning adjustment between two adjacent codepoints. - int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const; - int getTextAdvanceX(int fontId, const char* text, EpdFontFamily::Style style) const; + int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const override; + int getTextAdvanceX(int fontId, const char* text, EpdFontFamily::Style style) const override; int getFontAscenderSize(int fontId) const; int getLineHeight(int fontId) const; std::string truncatedText(int fontId, const char* text, int maxWidth, diff --git a/lib/GfxRenderer/ITextMetrics.h b/lib/GfxRenderer/ITextMetrics.h new file mode 100644 index 0000000000..d012701ea4 --- /dev/null +++ b/lib/GfxRenderer/ITextMetrics.h @@ -0,0 +1,27 @@ +#pragma once + +#include +#include +#include + +#include + +// Text-measurement surface used by layout code (ParsedText). Extracted from +// GfxRenderer so layout has no dependency on the E-ink HAL / Arduino, which +// keeps it host-testable. GfxRenderer implements this interface; production +// code keeps passing a GfxRenderer&, which up-casts implicitly. +class ITextMetrics { + public: + virtual ~ITextMetrics() = default; + + virtual int getSpaceWidth(int fontId, EpdFontFamily::Style style) const = 0; + virtual int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, + EpdFontFamily::Style style) const = 0; + virtual int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, + EpdFontFamily::Style style) const = 0; + virtual int getTextAdvanceX(int fontId, const char* text, EpdFontFamily::Style style) const = 0; + virtual bool isSdCardFont(int fontId) const = 0; + virtual void ensureSdCardFontReady(int fontId, const char* utf8Text, uint8_t styleMask) const = 0; + virtual void ensureSdCardFontReady(int fontId, const std::vector& words, bool includeHyphen, + uint8_t styleMask) const = 0; +}; From 7bda669e95e47745f4a713b77332bfde7f8e6558 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Thu, 18 Jun 2026 11:59:30 +1000 Subject: [PATCH 2/3] test: host harness pinning current ParsedText layout (English, nbsp, hyphenation, BiDi) --- .githooks/pre-commit | 20 ++- .github/workflows/ci.yml | 16 ++ test/CMakeLists.txt | 3 +- test/parsed_text/CMakeLists.txt | 28 ++++ test/parsed_text/ParsedTextLayoutTest.cpp | 170 ++++++++++++++++++++++ test/support/FakeTextMetrics.h | 31 ++++ test/support/Logging.h | 9 ++ 7 files changed, 275 insertions(+), 2 deletions(-) create mode 100644 test/parsed_text/CMakeLists.txt create mode 100644 test/parsed_text/ParsedTextLayoutTest.cpp create mode 100644 test/support/FakeTextMetrics.h create mode 100644 test/support/Logging.h diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 6943ec3616..26baa596fd 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -23,4 +23,22 @@ echo "Running clang-format fix before commit..." # working tree. if ((${#staged_files[@]})); then git add -- "${staged_files[@]}" -fi \ No newline at end of file +fi +# --- Round-10: Phase 1 placeholder-sentinel gate --- +# Block commits that still carry first-run sentinels in the host test sources. +# The Phase 1 commit MUST NOT land with these in place — they leave the tests +# vacuous. CI's ci.yml step is the mandatory gate; this hook is the fast local +# feedback path. +if git grep -l 'WILL_BE_REPLACED_AFTER_FIRST_RUN' -- test/ ; then + echo "ERROR: placeholder string WILL_BE_REPLACED_AFTER_FIRST_RUN still in test source." >&2 + echo " Run the test, copy the actual values, and replace the sentinels." >&2 + exit 1 +fi +if git grep -l 'VISUAL_ORDER_WORD_' -- test/ ; then + echo "ERROR: placeholder string VISUAL_ORDER_WORD_ still in test source." >&2 + exit 1 +fi +if git grep -l 'static_cast(-32768)' -- test/parsed_text/ ; then + echo "ERROR: placeholder int16 sentinel -32768 still in BiDi test source." >&2 + exit 1 +fi diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cfe848fbad..8aa46cdeda 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -123,6 +123,22 @@ jobs: with: submodules: recursive + - name: Verify no placeholder sentinels in test sources + run: | + if git grep -l 'WILL_BE_REPLACED_AFTER_FIRST_RUN' -- test/ ; then + echo "::error::placeholder string WILL_BE_REPLACED_AFTER_FIRST_RUN still in test source." >&2 + echo "::error::Run the test, copy the actual values, and replace the sentinels." >&2 + exit 1 + fi + if git grep -l 'VISUAL_ORDER_WORD_' -- test/ ; then + echo "::error::placeholder string VISUAL_ORDER_WORD_ still in test source." >&2 + exit 1 + fi + if git grep -l 'static_cast(-32768)' -- test/parsed_text/ ; then + echo "::error::placeholder int16 sentinel -32768 still in BiDi test source." >&2 + exit 1 + fi + - name: Install build tools run: | sudo apt-get update diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 684a1203b6..fb7c535f79 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,5 +1,5 @@ cmake_minimum_required(VERSION 3.16) -project(crosspoint_reader_tests CXX) +project(crosspoint_reader_tests CXX C) set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -46,3 +46,4 @@ add_subdirectory(cjk_ui_fallback) add_subdirectory(cjk_font_parity) add_subdirectory(font_resolver) add_subdirectory(font_boundary) +add_subdirectory(parsed_text) diff --git a/test/parsed_text/CMakeLists.txt b/test/parsed_text/CMakeLists.txt new file mode 100644 index 0000000000..e56d1c8ffc --- /dev/null +++ b/test/parsed_text/CMakeLists.txt @@ -0,0 +1,28 @@ +# IMPORTANT: TextBlock.cpp is intentionally NOT linked here. It pulls , +# which chains into HalDisplay (Arduino/HAL). ParsedText only uses TextBlock through +# its header-inline ctor and getWords(), so the cpp body is not required to link. +# If a later refactor moves any of those out-of-line, add a HOST_TEST guard inside +# TextBlock.cpp around the render/serialize bodies and link it here with -DHOST_TEST. +add_executable(ParsedTextLayoutTest + ParsedTextLayoutTest.cpp + ${REPO_ROOT}/lib/Epub/Epub/ParsedText.cpp + ${REPO_ROOT}/lib/Epub/Epub/hyphenation/Hyphenator.cpp + ${REPO_ROOT}/lib/Epub/Epub/hyphenation/LanguageRegistry.cpp + ${REPO_ROOT}/lib/Epub/Epub/hyphenation/LiangHyphenation.cpp + ${REPO_ROOT}/lib/Epub/Epub/hyphenation/HyphenationCommon.cpp + ${REPO_ROOT}/lib/MiniBidi/BidiUtils.cpp + ${REPO_ROOT}/lib/MiniBidi/minibidi.c + ${REPO_ROOT}/lib/Utf8/Utf8.cpp +) +target_include_directories(ParsedTextLayoutTest PRIVATE + ${REPO_ROOT} + ${REPO_ROOT}/lib/Epub + ${REPO_ROOT}/lib/GfxRenderer + ${REPO_ROOT}/lib/EpdFont + ${REPO_ROOT}/lib/Utf8 + ${REPO_ROOT}/lib/MiniBidi + ${REPO_ROOT}/test + ${REPO_ROOT}/test/support +) +target_link_libraries(ParsedTextLayoutTest PRIVATE crosspoint_test_common GTest::gtest_main) +gtest_discover_tests(ParsedTextLayoutTest) diff --git a/test/parsed_text/ParsedTextLayoutTest.cpp b/test/parsed_text/ParsedTextLayoutTest.cpp new file mode 100644 index 0000000000..31a0f23cfc --- /dev/null +++ b/test/parsed_text/ParsedTextLayoutTest.cpp @@ -0,0 +1,170 @@ +#include + +#include +#include +#include + +#include "lib/Epub/Epub/ParsedText.h" +#include "lib/Epub/Epub/blocks/TextBlock.h" +#include "lib/Epub/Epub/blocks/BlockStyle.h" +#include "support/FakeTextMetrics.h" + +namespace { + +constexpr int kFontId = 0; +constexpr int kCell = 10; // px per character cell + +// Each produced line's visible characters (whitespace is NOT stored as words, +// so this is glued — we assert break positions, not spacing). +std::vector linesOf(ParsedText& text, int viewportWidthPx) { + FakeTextMetrics metrics(kCell); + std::vector lines; + text.layoutAndExtractLines(metrics, kFontId, static_cast(viewportWidthPx), + [&](std::shared_ptr block) { + std::string joined; + for (const auto& w : block->getWords()) joined += w; + lines.push_back(joined); + }); + return lines; +} + +// Round-8 M3: layout helper that returns the TextBlock per line, not just the +// joined string. Needed by BiDi xpos assertions and any test that wants to +// inspect getWordXpos() / getWordStyles() / getWords() together. linesOf() is +// still preferred for break-position-only tests because its string return type +// keeps the assertions concise; layoutBlocksOf() is the escape hatch when a +// test needs the full TextBlock surface. +std::vector> layoutBlocksOf(ParsedText& text, int viewportWidthPx) { + FakeTextMetrics metrics(kCell); + std::vector> blocks; + text.layoutAndExtractLines(metrics, kFontId, static_cast(viewportWidthPx), + [&](std::shared_ptr block) { blocks.push_back(block); }); + return blocks; +} + +BlockStyle leftAligned() { + // BlockStyle default is Justify with no textIndent override, so resolveFirstLineIndent + // can fall back to "3 × space width" when extraParagraphSpacing=false. Pin both axes + // explicitly so layout tests measure only the break logic, not the first-line indent. + BlockStyle s; + s.alignment = CssTextAlign::Left; + s.textAlignDefined = true; + s.textIndent = 0; + s.textIndentDefined = true; + return s; +} + +} // namespace + +// English wraps at word boundaries. Whitespace is dropped, so the joined text +// of each line is space-less; we pin WHERE the breaks fall. +TEST(ParsedTextLayout, EnglishWrapsAtWordBoundaries) { + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); + for (const char* w : {"the", "quick", "brown", "fox"}) text.addWord(w, EpdFontFamily::REGULAR); + // viewport 9 cells: "the"(3)+space+"quick"(5) = 9 → fits (computeLineBreaks at + // ParsedText.cpp:407 uses strict `>` against viewport, so content==viewport fits). + // Next line: "brown"(5)+space+"fox"(3) = 9 → also fits. Total: 2 lines, not 3. + auto lines = linesOf(text, 9 * kCell); + ASSERT_EQ(lines.size(), 2u); + EXPECT_EQ(lines[0], "thequick"); // space-less by design (gap stored as xpos) + EXPECT_EQ(lines[1], "brownfox"); +} + +// No-break space IS an explicit " " word, so it appears in getWords(). +// +// Round-9 C2: viewport sizing rationale. The earlier "viewport=5 cells / content=6 cells" +// form was wrong — at the DP layer (ParsedText.cpp:407, strict `>`), 6 cells of content +// against a 5-cell viewport OVERFLOWS, and Glue forbids any break inside the run. With +// no valid break point, the fallback in `computeLineBreaks` (~L439-447) sets `ans[i] = i` +// (single-word line), and the result is `"200"` on line 0 alone — NOT `"200 km"` together. +// The test would have asserted lines.size()==1 against a fallback that produces a different +// single-line shape than intended ("200" only), confirming nothing about Glue policy. +// +// Correct setup: viewport=6 cells exactly accommodates "200" (3) + " " (1) + "km" (2) = +// 6 cells; under strict `>`, content == viewport fits on one line. To prove Glue does +// real work, juxtapose against a token that could otherwise produce a break: +// tokens "x", "200", NBSP, "km" at viewport=6 cells +// "x"(1) fits; can break after "x". "200"(3) + " "(1, Glue) + "km"(2) = 6 cells fits. +// Expected output: 2 lines — "x" and "200 km". Without Glue between " " and "km", +// the layout would prefer "x 200" on line 0 (1+1+3 = 5 ≤ 6) and " km" on line 1, +// which is a wrong shape for a no-break-space group. +TEST(ParsedTextLayout, NoBreakSpaceGlueKeepsGroupAtomic) { + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); + text.addWord("x", EpdFontFamily::REGULAR); // join=Space (default) + text.addWord("200", EpdFontFamily::REGULAR); // join=Space — breakable before + text.addWord(" ", EpdFontFamily::REGULAR, /*underline=*/false, /*attachToPrevious=*/true); // NBSP — Glue + text.addWord("km", EpdFontFamily::REGULAR, /*underline=*/false, /*attachToPrevious=*/true); // Glue + auto lines = linesOf(text, 6 * kCell); + // Glue forbids breaking between "200", " ", and "km" — the trio must move as one + // unit. The optimal layout puts "x" on line 0 and "200 km" on line 1. + ASSERT_EQ(lines.size(), 2u); + EXPECT_EQ(lines[0], "x"); + EXPECT_EQ(lines[1], "200 km"); // the explicit " " word is present in the join +} + +// Hyphenation: a word longer than the line splits via fallback breaks. +TEST(ParsedTextLayout, LongWordHyphenatesAcrossLines) { + // Note: hyphenation is the 2nd arg (hyphenationEnabled=true). allowFallback is not a ParsedText + // constructor argument — it is a layout-time parameter. This test enables hyphenation. + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/true, /*focusReadingEnabled=*/false, leftAligned()); + text.addWord("supercalifragilistic", EpdFontFamily::REGULAR); // 20 chars + auto lines = linesOf(text, 8 * kCell); + + // Round-9 M8: pin the WHOLE vector, not just lines[0] / lines[1]. A partial + // assertion lets a regression that drops, duplicates, or reorders any line + // from index 2 onwards pass silently. The hyphenation algorithm is + // deterministic for a given font width + viewport, so the full `lines` + // vector IS the contract. + // + // These are characterized (pinned) current-behaviour outputs of the Liang + // hyphenation algorithm for this input + 8-cell viewport. + const std::vector expectedLines = { + "superca-", + "lifragi-", + "listic", + }; + EXPECT_EQ(lines, expectedLines); + + // Sanity: concatenating all lines must recover the input word (modulo + // optional hyphen characters added by Liang hyphenation). Strip '-' before + // comparing so the integrity check doesn't depend on the exact break shape. + std::string joined; + for (const auto& l : lines) joined += l; + joined.erase(std::remove(joined.begin(), joined.end(), '-'), joined.end()); + EXPECT_EQ(joined, "supercalifragilistic") + << "hyphenation must not drop or duplicate characters across lines"; +} + +// BiDi: an RTL run reorders visually. Pin reorder itself by asserting the visual +// word order, not only xpos. +// +// Round-9 M1: an xpos-only assertion does NOT directly characterize whether reorder +// happened — three different orderings (`abc אב xyz`, `אב abc xyz`, `xyz אב abc`) +// can land on the same xpos column under degenerate widths. The non-vacuous pin is +// `blocks[0]->getWords()` returning the visual order. We use asymmetric widths +// (`abc`=3 cells, `אב`=2 cells, `xyz`=3 cells, total=8 ≤ 20 → 1 line) so that any +// reorder regression also moves the xpos boundaries (auxiliary assert below). +TEST(ParsedTextLayout, BidiRtlRunLaysOut) { + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); + text.addWord("abc", EpdFontFamily::REGULAR); // 3 cells, LTR + text.addWord("\xD7\x90\xD7\x91", EpdFontFamily::REGULAR); // אב — 2 cells, RTL (Hebrew aleph-bet) + text.addWord("xyz", EpdFontFamily::REGULAR); // 3 cells, LTR + auto blocks = layoutBlocksOf(text, 20 * kCell); + ASSERT_EQ(blocks.size(), 1u); + ASSERT_EQ(blocks[0]->getWords().size(), 3u); + + // PRIMARY assertion: visual order is the contract. These are characterized + // (pinned) current-behaviour outputs of the BiDi reorder for this input. + const auto& words = blocks[0]->getWords(); + EXPECT_EQ(words[0], "xyz"); + EXPECT_EQ(words[1], "\xD7\x90\xD7\x91"); // אב — Hebrew aleph-bet, visually RTL + EXPECT_EQ(words[2], "abc"); + + // AUXILIARY assertion: xpos sequence (helpful for diagnosing reorder regressions + // because reorder shifts the LTR runs' x-positions). + const auto& xpos = blocks[0]->getWordXpos(); + ASSERT_EQ(xpos.size(), 3u); + EXPECT_EQ(xpos[0], static_cast(0)); + EXPECT_EQ(xpos[1], static_cast(40)); + EXPECT_EQ(xpos[2], static_cast(70)); +} diff --git a/test/support/FakeTextMetrics.h b/test/support/FakeTextMetrics.h new file mode 100644 index 0000000000..407696c724 --- /dev/null +++ b/test/support/FakeTextMetrics.h @@ -0,0 +1,31 @@ +#pragma once + +#include +#include + +#include +#include +#include + +// Deterministic: every codepoint advances cellPx, a space advances cellPx, +// kerning is zero. Makes line-break math exact and font-independent. +class FakeTextMetrics : public ITextMetrics { + public: + explicit FakeTextMetrics(int cellPx = 10) : cellPx_(cellPx) {} + + int getSpaceWidth(int, EpdFontFamily::Style) const override { return cellPx_; } + int getSpaceAdvance(int, uint32_t, uint32_t, EpdFontFamily::Style) const override { return cellPx_; } + int getKerning(int, uint32_t, uint32_t, EpdFontFamily::Style) const override { return 0; } + int getTextAdvanceX(int, const char* text, EpdFontFamily::Style) const override { + int width = 0; + const auto* p = reinterpret_cast(text); + while (utf8NextCodepoint(&p) != 0) width += cellPx_; + return width; + } + bool isSdCardFont(int) const override { return false; } + void ensureSdCardFontReady(int, const char*, uint8_t) const override {} + void ensureSdCardFontReady(int, const std::vector&, bool, uint8_t) const override {} + + private: + int cellPx_; +}; diff --git a/test/support/Logging.h b/test/support/Logging.h new file mode 100644 index 0000000000..df68c3bf31 --- /dev/null +++ b/test/support/Logging.h @@ -0,0 +1,9 @@ +#pragma once +// Host (non-Arduino) stub so BidiUtils.cpp and any other lib files that +// include compile under gtest without pulling in HardwareSerial.h. +// All log macros are silenced on host — they emit nothing. This matches the +// device-side ENABLE_SERIAL_LOG=off behaviour (release builds). + +#define LOG_DBG(origin, format, ...) +#define LOG_ERR(origin, format, ...) +#define LOG_INF(origin, format, ...) From 0cbe62c9539fb40ed7d723e640b662f817edc2d4 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Thu, 18 Jun 2026 12:13:51 +1000 Subject: [PATCH 3/3] style: apply clang-format-21 to ITextMetrics.h and ParsedTextLayoutTest.cpp --- lib/GfxRenderer/ITextMetrics.h | 10 ++++----- test/parsed_text/ParsedTextLayoutTest.cpp | 27 +++++++++++++---------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/GfxRenderer/ITextMetrics.h b/lib/GfxRenderer/ITextMetrics.h index d012701ea4..d6b232b917 100644 --- a/lib/GfxRenderer/ITextMetrics.h +++ b/lib/GfxRenderer/ITextMetrics.h @@ -1,11 +1,11 @@ #pragma once +#include + #include #include #include -#include - // Text-measurement surface used by layout code (ParsedText). Extracted from // GfxRenderer so layout has no dependency on the E-ink HAL / Arduino, which // keeps it host-testable. GfxRenderer implements this interface; production @@ -15,10 +15,8 @@ class ITextMetrics { virtual ~ITextMetrics() = default; virtual int getSpaceWidth(int fontId, EpdFontFamily::Style style) const = 0; - virtual int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, - EpdFontFamily::Style style) const = 0; - virtual int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, - EpdFontFamily::Style style) const = 0; + virtual int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const = 0; + virtual int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const = 0; virtual int getTextAdvanceX(int fontId, const char* text, EpdFontFamily::Style style) const = 0; virtual bool isSdCardFont(int fontId) const = 0; virtual void ensureSdCardFontReady(int fontId, const char* utf8Text, uint8_t styleMask) const = 0; diff --git a/test/parsed_text/ParsedTextLayoutTest.cpp b/test/parsed_text/ParsedTextLayoutTest.cpp index 31a0f23cfc..61285c8c90 100644 --- a/test/parsed_text/ParsedTextLayoutTest.cpp +++ b/test/parsed_text/ParsedTextLayoutTest.cpp @@ -5,8 +5,8 @@ #include #include "lib/Epub/Epub/ParsedText.h" -#include "lib/Epub/Epub/blocks/TextBlock.h" #include "lib/Epub/Epub/blocks/BlockStyle.h" +#include "lib/Epub/Epub/blocks/TextBlock.h" #include "support/FakeTextMetrics.h" namespace { @@ -59,7 +59,8 @@ BlockStyle leftAligned() { // English wraps at word boundaries. Whitespace is dropped, so the joined text // of each line is space-less; we pin WHERE the breaks fall. TEST(ParsedTextLayout, EnglishWrapsAtWordBoundaries) { - ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, + leftAligned()); for (const char* w : {"the", "quick", "brown", "fox"}) text.addWord(w, EpdFontFamily::REGULAR); // viewport 9 cells: "the"(3)+space+"quick"(5) = 9 → fits (computeLineBreaks at // ParsedText.cpp:407 uses strict `>` against viewport, so content==viewport fits). @@ -89,10 +90,11 @@ TEST(ParsedTextLayout, EnglishWrapsAtWordBoundaries) { // the layout would prefer "x 200" on line 0 (1+1+3 = 5 ≤ 6) and " km" on line 1, // which is a wrong shape for a no-break-space group. TEST(ParsedTextLayout, NoBreakSpaceGlueKeepsGroupAtomic) { - ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); - text.addWord("x", EpdFontFamily::REGULAR); // join=Space (default) + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, + leftAligned()); + text.addWord("x", EpdFontFamily::REGULAR); // join=Space (default) text.addWord("200", EpdFontFamily::REGULAR); // join=Space — breakable before - text.addWord(" ", EpdFontFamily::REGULAR, /*underline=*/false, /*attachToPrevious=*/true); // NBSP — Glue + text.addWord(" ", EpdFontFamily::REGULAR, /*underline=*/false, /*attachToPrevious=*/true); // NBSP — Glue text.addWord("km", EpdFontFamily::REGULAR, /*underline=*/false, /*attachToPrevious=*/true); // Glue auto lines = linesOf(text, 6 * kCell); // Glue forbids breaking between "200", " ", and "km" — the trio must move as one @@ -106,7 +108,8 @@ TEST(ParsedTextLayout, NoBreakSpaceGlueKeepsGroupAtomic) { TEST(ParsedTextLayout, LongWordHyphenatesAcrossLines) { // Note: hyphenation is the 2nd arg (hyphenationEnabled=true). allowFallback is not a ParsedText // constructor argument — it is a layout-time parameter. This test enables hyphenation. - ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/true, /*focusReadingEnabled=*/false, leftAligned()); + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/true, /*focusReadingEnabled=*/false, + leftAligned()); text.addWord("supercalifragilistic", EpdFontFamily::REGULAR); // 20 chars auto lines = linesOf(text, 8 * kCell); @@ -131,8 +134,7 @@ TEST(ParsedTextLayout, LongWordHyphenatesAcrossLines) { std::string joined; for (const auto& l : lines) joined += l; joined.erase(std::remove(joined.begin(), joined.end(), '-'), joined.end()); - EXPECT_EQ(joined, "supercalifragilistic") - << "hyphenation must not drop or duplicate characters across lines"; + EXPECT_EQ(joined, "supercalifragilistic") << "hyphenation must not drop or duplicate characters across lines"; } // BiDi: an RTL run reorders visually. Pin reorder itself by asserting the visual @@ -145,10 +147,11 @@ TEST(ParsedTextLayout, LongWordHyphenatesAcrossLines) { // (`abc`=3 cells, `אב`=2 cells, `xyz`=3 cells, total=8 ≤ 20 → 1 line) so that any // reorder regression also moves the xpos boundaries (auxiliary assert below). TEST(ParsedTextLayout, BidiRtlRunLaysOut) { - ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, leftAligned()); - text.addWord("abc", EpdFontFamily::REGULAR); // 3 cells, LTR - text.addWord("\xD7\x90\xD7\x91", EpdFontFamily::REGULAR); // אב — 2 cells, RTL (Hebrew aleph-bet) - text.addWord("xyz", EpdFontFamily::REGULAR); // 3 cells, LTR + ParsedText text(/*extraParagraphSpacing=*/false, /*hyphenationEnabled=*/false, /*focusReadingEnabled=*/false, + leftAligned()); + text.addWord("abc", EpdFontFamily::REGULAR); // 3 cells, LTR + text.addWord("\xD7\x90\xD7\x91", EpdFontFamily::REGULAR); // אב — 2 cells, RTL (Hebrew aleph-bet) + text.addWord("xyz", EpdFontFamily::REGULAR); // 3 cells, LTR auto blocks = layoutBlocksOf(text, 20 * kCell); ASSERT_EQ(blocks.size(), 1u); ASSERT_EQ(blocks[0]->getWords().size(), 3u);