From 705358dcc6208bbf14b84fa2ade8dd10aa2e1a65 Mon Sep 17 00:00:00 2001 From: Torlando <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:26:24 +0000 Subject: [PATCH] fix(nomadnet): partial refresh keeps real alt text for region images Round-3 Greptile P1 (Partial Images Lose Alt Text): the round-2 fix dropped fragment image records, so a refreshed-region image could only render its fixed '[Image: loading]' run instead of its real alt text. Reference 1.4.3 semantics: a partial refresh never fetches images (detect_images runs only at full page load), but the refreshed region is re-parsed and its images show their real alt text. Implementation: - region-owned base records (referenced by the region's image blocks) are discarded each refresh, keeping the record list bounded; - all other base records are copied verbatim (stable compact indices, so decoded images keep rendering); - fragment records are appended with an offset remap, so a refreshed-region image renders its real alt text and is never fetched (no loader entry exists for it). Also fixes a latent kept-offset bug the new two-refresh test exposed: the fragment offset used the base record total instead of the kept count (wrong whenever a region-owned record was discarded). Test: native partial-core now asserts real alt text after a refresh, a bounded count across two refreshes (1 retained + 2 region), and stable retained indices; verified failing against the prior build. --- lib/tdeck_ui/UI/LXMF/NomadNetCompactPage.cpp | 111 +++++++++++++++---- tests/native/test_nomadnet_partial_core.cpp | 53 +++++---- 2 files changed, 125 insertions(+), 39 deletions(-) diff --git a/lib/tdeck_ui/UI/LXMF/NomadNetCompactPage.cpp b/lib/tdeck_ui/UI/LXMF/NomadNetCompactPage.cpp index a791c831..cc9fe3e1 100644 --- a/lib/tdeck_ui/UI/LXMF/NomadNetCompactPage.cpp +++ b/lib/tdeck_ui/UI/LXMF/NomadNetCompactPage.cpp @@ -4,6 +4,7 @@ #include #include #include +#include namespace UI::LXMF::NomadNet { @@ -463,19 +464,46 @@ PartialReplaceResult CompactPage::assign_replacing_partial( // Image records are stable identity: the screen's decode slots and // the loader entries are both keyed by the compact image index, and - // the loader is configured once at full page load. The reference - // detects images only at full page load — a partial refresh never - // scans (or loads) images, neither in the retained body nor in the - // refreshed region — so the record list is copied verbatim from the - // base: same records, same order, same indices, and a constant - // count across repeated refreshes (no accumulation, no remap). - // Fragment image blocks therefore carry no record (image_index -1) - // and render as alt-text placeholders until the next full load. - for (const auto& source : base._images) { + // the loader is configured once at full page load. A partial refresh + // never scans for or fetches images (reference: detect_images runs + // only at full page load; a refreshed region re-parses but is not + // image-detected), but the reference DOES show the region's real alt + // text for such images. So: + // - base records owned by the replaced region (referenced by its + // image blocks) are discarded — each refresh replaces them, so + // the list stays bounded (no accumulation across refreshes); + // - all other base records are copied verbatim (same order and + // indices, so a decoded image keeps rendering); + // - the fragment's records are appended with an offset remap, so + // a refreshed-region image renders its real alt text (and a + // decoded buffer, had one been stored) instead of a fixed + // "loading" string. They are never fetched (no loader entry). + auto image_in_region = [&](const CompactPage& page, + std::size_t source_index) { + if (source_index >= page._blocks.size()) return false; + const BlockRecord& b = page._blocks[source_index]; + return b.type == BlockType::IMAGE && b.image_index >= 0 && + b.partial_region_index == static_cast(partial_index); + }; + std::vector region_owned; + { + bool bad = false; + try { + region_owned.assign(base._images.size(), false); + for (std::size_t i = 0; i < base._blocks.size(); ++i) + if (image_in_region(base, i)) + region_owned[static_cast( + base._blocks[i].image_index)] = true; + } catch (const std::bad_alloc&) { bad = true; } + if (bad) { clear(); return PartialReplaceResult::ALLOCATION_FAILED; } + } + for (std::size_t i = 0; i < base._images.size(); ++i) { + if (region_owned[i]) continue; if (_images.size() >= MAX_IMAGES) { clear(); return PartialReplaceResult::LIMIT_EXCEEDED; } + const auto& source = base._images[i]; ImageRecord image; if (!append_view(base.image_alt(source), image.alt_offset, image.alt_length) || @@ -491,6 +519,43 @@ PartialReplaceResult CompactPage::assign_replacing_partial( image.align = source.align; _images.push_back(image); } + for (const auto& source : fragment_page._images) { + if (_images.size() >= MAX_IMAGES) break; // the rest render via + // their run fallback + ImageRecord image; + if (!append_view(fragment_page.image_alt(source), image.alt_offset, + image.alt_length) || + !append_view(fragment_page.image_url(source), image.url_offset, + image.url_length)) { + clear(); + return PartialReplaceResult::LIMIT_EXCEEDED; + } + image.width_kind = source.width_kind; + image.height_kind = source.height_kind; + image.width_value = source.width_value; + image.height_value = source.height_value; + image.align = source.align; + _images.push_back(image); + } + // Image map: base record i -> its new index, or -1 when the record + // was region-owned and discarded. Fragment records follow the kept + // base records (offset = number kept, NOT the base total). + ExternalVector base_image_map; + base_image_map.assign(base._images.size(), -1); + std::size_t kept_base_images = 0; + { + bool bad = false; + try { + std::size_t next = 0; + for (std::size_t i = 0; i < base._images.size(); ++i) { + if (region_owned[i]) continue; + base_image_map[i] = static_cast(next); + ++next; + } + kept_base_images = next; + } catch (const std::bad_alloc&) { bad = true; } + if (bad) { clear(); return PartialReplaceResult::ALLOCATION_FAILED; } + } struct CopyMaps { ExternalVector links; @@ -660,16 +725,24 @@ PartialReplaceResult CompactPage::assign_replacing_partial( block.partial_region_index = region_override >= 0 ? region_override : old.partial_region_index; if (old.image_index >= 0) { - // Base records were copied verbatim (identity), so a - // retained image keeps its compact index. Fragment images - // have no record in the new page (a partial refresh never - // loads images) and fall back to -1. - const bool from_base = (&source == &base); - const std::size_t mapped = from_base - ? static_cast(old.image_index) - : _images.size(); // out of range -> placeholder - block.image_index = (from_base && mapped < _images.size()) - ? static_cast(mapped) : -1; + // Base records survive verbatim (remapped through the kept + // base map; region-owned records were discarded). Fragment + // records follow the kept base records (offset remap), so a + // refreshed-region image renders its real alt text and is + // still never fetched (no loader entry exists for it). + int16_t mapped_index = -1; + if (&source == &base) { + mapped_index = + base_image_map[static_cast(old.image_index)]; + } else if (static_cast(old.image_index) < + fragment_page._images.size() && + kept_base_images + + static_cast(old.image_index) < + _images.size()) { + mapped_index = static_cast( + kept_base_images + static_cast(old.image_index)); + } + block.image_index = mapped_index; } if (old.table_index >= 0) { block.table_index = copy_table(source, maps, old.table_index); diff --git a/tests/native/test_nomadnet_partial_core.cpp b/tests/native/test_nomadnet_partial_core.cpp index 382a6e34..d2f8623d 100644 --- a/tests/native/test_nomadnet_partial_core.cpp +++ b/tests/native/test_nomadnet_partial_core.cpp @@ -259,10 +259,13 @@ int main() { std::string(preserved_form.fields()[0].value.data(), preserved_form.fields()[0].value_length) == "user value"); - // Image records must survive a partial refresh with stable identity: - // the base list is copied verbatim (same records, same indices), the - // count stays constant across repeated refreshes (no accumulation), and - // fragment images render as placeholders (image_index -1). + // Image records across a partial refresh (reference 1.4.3): + // - a partial refresh never fetches images, but the refreshed region's + // images keep their real alt text (records are replaced, not dropped); + // - region-owned records are discarded each refresh, so the list stays + // bounded (no accumulation across refreshes); + // - retained (non-region) images keep their compact index, so a + // decoded image keeps rendering. const auto img_source = parser.parse( R"(before `(base img`a.webp) @@ -279,12 +282,12 @@ after)"); CompactPage img_candidate; const auto img_replace = img_candidate.assign_replacing_partial( img_base, 0, img_fragment, CompactPage::MAX_ARENA_BYTES); - check("partial refresh preserves base image records and identity", + check("partial refresh replaces region image records (no accumulation)", img_replace == PartialReplaceResult::APPLIED && - img_candidate.images().size() == 1 && + img_candidate.images().size() == 2 && img_candidate.has_image_blocks()); - if (img_replace == PartialReplaceResult::APPLIED) { - // Base image: same compact index, same alt/url. + if (img_replace == PartialReplaceResult::APPLIED && + img_candidate.images().size() == 2) { const auto& base_image = img_candidate.blocks()[1]; check("retained image block keeps its compact image index", base_image.type == BlockType::IMAGE && @@ -292,29 +295,39 @@ after)"); check("retained image record keeps its alt and url", img_candidate.image_alt(img_candidate.images()[0]) == "base img" && img_candidate.image_url(img_candidate.images()[0]) == "a.webp"); - // Fragment image: placeholder (no record, no fetch). + // Fragment image: region records replaced; real alt text present. const auto& frag_image = img_candidate.blocks()[3]; - check("fragment image renders as a placeholder block", + check("fragment image keeps its real alt text (no loading string)", frag_image.type == BlockType::IMAGE && - frag_image.image_index == -1); - // Second refresh: the candidate is the new base; the record list - // must not accumulate (constant count), and the base image keeps - // its index. + frag_image.image_index == 1 && + img_candidate.image_alt(img_candidate.images()[1]) == "frag img" && + img_candidate.image_url(img_candidate.images()[1]) == "b.webp"); + // Second refresh (region now holds two images): the count reflects + // the region's NEW content, not the accumulated history, and the + // retained image still has index 0. const auto img_fragment2 = parser.parse( R"(v2 -`(frag2 img`c.webp))"); +`(frag2a`c.webp) +`(frag2b`d.webp))"); CompactPage img_candidate2; const auto img_replace2 = img_candidate2.assign_replacing_partial( img_candidate, 0, img_fragment2, CompactPage::MAX_ARENA_BYTES); - check("repeated partial refresh does not accumulate image records", + check("repeated partial refresh is bounded (1 retained + 2 region)", img_replace2 == PartialReplaceResult::APPLIED && - img_candidate2.images().size() == 1); + img_candidate2.images().size() == 3); if (img_replace2 == PartialReplaceResult::APPLIED && - img_candidate2.images().size() == 1) { - check("base image keeps its index after the second refresh", - img_candidate2.has_image_blocks() && + img_candidate2.images().size() == 3) { + check("retained image keeps its index after the second refresh", + img_candidate2.blocks()[1].image_index == 0 && img_candidate2.image_alt(img_candidate2.images()[0]) == "base img"); + check("second-refresh region images keep their real alt text", + img_candidate2.blocks()[3].image_index == 1 && + img_candidate2.blocks()[4].image_index == 2 && + img_candidate2.image_alt(img_candidate2.images()[1]) == + "frag2a" && + img_candidate2.image_alt(img_candidate2.images()[2]) == + "frag2b"); } } const auto region_source = parser.parse(