fix(magazine): deleting a picture removes both of its records; say clearly while a turn is being read

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
mARTin-B78 2026-09-30 18:35:20 +02:00
parent 916d19d22f
commit 80ec22b752
9 changed files with 245 additions and 11 deletions

View File

@ -6,6 +6,10 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi
---
## [Unreleased]
### Fixed
- **Deleting a picture left an empty paragraph in its place (seen on c't p. 157 and 159).** A picture region is stored as two records, a visual block and an image entry linked by `source_block`, and the review list shows either one as a row. Delete removed only the row's own record, so the twin stayed at the same spot as an empty, unticked "Paragraph" that could not be told from a real one. Deleting either half now removes both (single delete, bulk delete and the type changes), the articles forget both ids, and Undo brings both back; a hand-drawn picture (no twin) and a text block that was retyped since are untouched. Copies of a magazine that already carry leftovers: *Clean up boxes* re-types an empty picture block that has no picture (not narrated, still listed, reversible; your production issue has 1 such, on page 158). Verified on your real page 159 through the real route on a scratch copy: delete removes both, Undo restores both.
- **A turn in progress now says so.** While a turn is being read the picture and text on screen are still the old ones for 30 to 60 seconds, which looked like a failed turn (a screenshot showed the stale upside-down picture and its gibberish text beside "Re-reading page 156"). The review now shows a notice with the elapsed time saying the picture and text below are from before the turn, dims them and blocks clicks into them, and removes both when the read finishes.
---

View File

@ -407,6 +407,9 @@ of the two pages the buttons act on, as with the other page tools.
are re-measured. On a test page, reading it sideways scored word-F1 0.02 and reading it after turning 0.88 (an ordinary page scores
about 0.88). The turn is remembered: re-running detection or re-importing does not flip the page back.
**While a turn is being read** the picture and text you see are the old ones: the notice above the page says so, shows the time spent, and
dims them until the read finishes (usually 30 to 60 seconds). The text is never turned itself; it is read again from the turned picture.
**Keeping track of turns.** Turns add up: a right turn plus a half turn is three quarters. The toolbar says so in words (for example
"Turned 270° clockwise from the scan") and offers **Undo turns**, which returns the page to the orientation the PDF itself gives it. Only one
turn or re-read of a page can run at a time: pressing a button again while one is still working is refused with a message, instead of stacking

View File

@ -477,6 +477,15 @@ Verified end-to-end against a synthetic 8-page issue (contents page, two-column
feature with a byline and a page-8 jump, full-page advert, second feature,
reviews) and against an image-only rendering of the same file to exercise OCR.
## A picture is two records
A picture region is stored as a visual block (`figure`/`chart`/`table`/`code`, empty or with OCR text) and an image entry whose `source_block` names
it (`crop_visual_regions`); hand-drawn pictures have only the image entry. `region_ids_with` returns the ids that belong together, `delete_region` removes
them all, and the delete and bulk routes drop every one of them from the articles. Only a *visual* block is taken with its image, so a text block that was
retyped after the image was made is never deleted by mistake. Before this, deleting the image row left the block as an empty paragraph-looking box in the
same place (real c't pages 157 and 159). `clean_up_regions` re-types an empty visual block that no image entry owns as `ocr_noise`, to tidy leftovers from
before the fix.
## Endless-token regions and unbreakable text
A recogniser stuck in a loop can emit one token with no spaces at all (c't p. 176: `Giro + 2.1.1.1.1…`, 3,408 characters). `_looks_like_repetition_loop`

View File

@ -4768,7 +4768,13 @@ def clean_up_regions(layout: dict[str, Any], pages: Iterable[int] | None = None)
b["type"], b["region_label"], b["confidence"] = BLOCK_FURNITURE, label, 0.9
report[label].append({"page": b["page"], "id": b["id"], "text": " ".join((b.get("text") or "").split())[:80]})
owned = {im.get("source_block") for im in layout.get("images", []) if im.get("source_block")}
for page_no, page_blocks in by_page.items():
# What deleting a picture used to leave behind: an empty figure block whose image entry is gone. It is nothing
# (no text, no picture to play), so it is re-typed as noise -- still listed, reversible, not narrated.
for b in page_blocks:
if b["type"] in _VISUAL_TYPES and not (b.get("text") or "").strip() and b["id"] not in owned:
mark(b, "ocr_noise")
text_boxes = [b for b in page_blocks if b["type"] not in _VISUAL_TYPES and b["type"] != BLOCK_FURNITURE]
for b in text_boxes:
text = " ".join((b.get("text") or "").split())
@ -5095,15 +5101,32 @@ def reorder_page(layout: dict[str, Any], page_no: int) -> dict[str, Any]:
return layout
def region_ids_with(layout: dict[str, Any], region_id: str) -> set[str]:
"""The ids that go when `region_id` is deleted: the region itself and, for a picture, its other half.
A picture region is stored twice: a visual block (figure/chart/table/code, empty or carrying OCR text) and an
image entry whose `source_block` names it. The review list shows either one as a row, so deleting "the image"
used to leave the block behind as a box with an empty paragraph in the same place (and the other way round).
"""
ids = {region_id}
for im in layout.get("images", []):
if im["id"] == region_id and im.get("source_block"):
twin = next((b for b in layout.get("blocks", []) if b["id"] == im["source_block"]), None)
if twin is not None and twin.get("type") in VISUAL_TYPES: # never take a text block that was retyped since
ids.add(twin["id"])
ids |= {im["id"] for im in layout.get("images", []) if im.get("source_block") == region_id}
return ids
def delete_region(layout: dict[str, Any], block_id: str) -> dict[str, Any]:
"""Remove a region the layout engine got wrong entirely (not just
misplaced). An article referencing this id in its own blocks/images
list is left as-is -- article_text() and the review screen already
treat an id no longer present in the layout as simply absent, the same
way a page re-OCR can already drop ids an article still lists."""
"""Remove a region the layout engine got wrong entirely (not just misplaced) -- both halves of a picture region
(see region_ids_with). An article referencing a removed id in its own blocks/images list is left as-is here:
article_text() and the review screen treat an id no longer in the layout as absent, and the routes drop the
ids from the articles themselves."""
gone = region_ids_with(layout, block_id)
layout = dict(layout)
layout["blocks"] = [b for b in layout.get("blocks", []) if b["id"] != block_id]
layout["images"] = [im for im in layout.get("images", []) if im["id"] != block_id]
layout["blocks"] = [b for b in layout.get("blocks", []) if b["id"] not in gone]
layout["images"] = [im for im in layout.get("images", []) if im["id"] not in gone]
return layout

View File

@ -1760,8 +1760,9 @@ async def magazine_bulk_regions(doc_id: str, page: int, request: Request):
remap: dict[str, str | None] = {}
if action == "delete":
for rid in ids:
for gone_id in mag.region_ids_with(layout, rid):
remap[gone_id] = None # the twin half of a picture is forgotten by the articles too
layout = mag.delete_region(layout, rid)
remap[rid] = None
else:
settings = _load_settings()
ocr = [str(settings.get(k) or "").strip() or None for k in
@ -2148,6 +2149,7 @@ async def magazine_delete_region(doc_id: str, page: int, block_id: str):
not any(im["id"] == block_id for im in layout.get("images", [])):
raise HTTPException(404, "Region not found")
token = _snapshot_for_undo(d, "Deleted a region") if d is not None else None
gone = mag.region_ids_with(layout, block_id) # a picture goes with its twin record
layout = mag.delete_region(layout, block_id)
mag.save_layout(d, layout)
# Also drop it from articles: an article still listing a deleted id made Approve fail with
@ -2156,8 +2158,8 @@ async def magazine_delete_region(doc_id: str, page: int, block_id: str):
changed = False
for article in articles:
for key in ("blocks", "images"):
if block_id in (article.get(key) or []):
article[key] = [x for x in article[key] if x != block_id]
if any(x in gone for x in (article.get(key) or [])):
article[key] = [x for x in article[key] if x not in gone]
changed = True
if changed:
mag.save_articles(d, articles)

View File

@ -3628,6 +3628,29 @@ async function magReocrPage() {
renderReview();
}
// While a turn is being read, the picture and the text on screen are still the OLD ones (the page is replaced as a whole
// when the read finishes, 30 s to a few minutes). Say so plainly, with the time spent, and dim what is about to be replaced,
// so nobody mistakes the stale, sideways picture and its gibberish text for the result.
function magTurnBusyNote(on, page, detail, startedAt) {
const review = $('mag-review');
const split = review?.querySelector('.mag-review-split');
let note = $('mag-turn-busy');
if (!on) { note?.remove(); split?.classList.remove('is-stale'); return; }
if (!split) return;
if (!note) {
note = document.createElement('div');
note.id = 'mag-turn-busy';
note.className = 'mag-turn-busy';
note.setAttribute('role', 'status');
split.before(note);
}
const secs = Math.round((Date.now() - startedAt) / 1000);
note.innerHTML = '<span class="mdi mdi-loading mdi-spin" aria-hidden="true"></span> <strong>Turning page ' + Number(page) +
' and reading it again</strong> — ' + esc(detail || 'working') + ' (' + secs + ' s). The picture and the text below are from <em>before</em> ' +
'the turn and are replaced when this finishes; it usually takes 30 to 60 seconds.';
split.classList.add('is-stale');
}
// How far this page has been turned from the PDF's own orientation, in words, with a way back. Turns add up (a right
// turn and a half turn is three quarters), so pressing the buttons more than once is easy to lose track of.
function magTurnNow(page) {
@ -3662,7 +3685,12 @@ async function magRotatePage(degrees) {
magOfferUndo(null); // an older Undo would put old-orientation boxes on the turned page
const buttons = ['mag-turn-left', 'mag-turn-right', 'mag-turn-flip', 'mag-turn-undo', 'mag-reocr-btn'].map($).filter(Boolean);
const statusEl = $('mag-reocr-status');
const busy = (on, text) => { buttons.forEach((b) => { b.disabled = on; }); if (statusEl) statusEl.textContent = text || ''; };
const startedAt = Date.now();
const busy = (on, text) => {
buttons.forEach((b) => { b.disabled = on; });
if (statusEl) statusEl.textContent = text || '';
magTurnBusyNote(on, page, text, startedAt);
};
busy(true, 'Turning page ' + page + '…');
let jobId;
try {

View File

@ -5655,3 +5655,7 @@ details.mag-report-group summary { cursor: pointer; margin-bottom: 6px; }
.mag-review-split > *, .mag-blocks, .mag-block { min-width: 0; }
.mag-block-text, .mag-diff, .mag-article, .mag-driving-lyrics, .mag-page-article-title, .mag-toc-title { overflow-wrap: anywhere; }
.mag-turn-state { align-self: center; padding: 0 6px; font-size: 12px; font-weight: 600; color: var(--subtext); }
.mag-turn-busy { margin: 8px 0; padding: 10px 14px; border-radius: var(--radius); border: 2px solid var(--accent);
background: color-mix(in srgb, var(--accent) 12%, var(--surface)); font-size: 14px; line-height: 1.4; }
.mag-review-split.is-stale { opacity: .4; pointer-events: none; filter: grayscale(.6); transition: opacity .15s; }
@media (prefers-reduced-motion: reduce) { .mag-review-split.is-stale { transition: none; } }

View File

@ -2036,6 +2036,25 @@ test.describe('Turning a page scanned sideways', () => {
await expect(page.getByRole('button', { name: /Turn right/ })).toBeEnabled();
});
test('while a turn is read the old picture and text are dimmed and labelled as old, then the notice goes', async ({ page }) => {
const calls = await open(page);
let polls = 0;
await page.route(`**/api/magazine/${DOC_ID}/pages/3/rotate`, route => { calls.rotate.push(route.request().postDataJSON()); return route.fulfill({ json: { job_id: 'rot1', degrees: 90 } }); });
await page.route('**/api/jobs/rot1', route => { polls++; return route.fulfill({ json: polls < 3 ? { done: false, detail: 'Re-reading page 3' } : { done: true, error: null } }); });
await page.getByRole('button', { name: /Turn right/ }).click();
await page.getByRole('alertdialog').getByRole('button', { name: 'Turn and read again' }).click();
const note = page.locator('#mag-turn-busy');
await expect(note).toBeVisible();
await expect(note).toContainText('Turning page 3 and reading it again');
await expect(note).toContainText('from before');
await expect(note).toHaveAttribute('role', 'status');
await expect(page.locator('.mag-review-split')).toHaveClass(/is-stale/);
// what is stale cannot be clicked into by accident
expect(await page.locator('.mag-review-split').evaluate(el => getComputedStyle(el).pointerEvents)).toBe('none');
await expect(page.locator('#mag-turn-busy')).toHaveCount(0, { timeout: 15_000 });
await expect(page.locator('.mag-review-split')).not.toHaveClass(/is-stale/);
});
test('cancelling the question turns nothing', async ({ page }) => {
const calls = await open(page);
await routeRotate(page, calls);

View File

@ -2176,3 +2176,145 @@ class FastLlmCallTests(unittest.TestCase):
with patch('routes.conversation._post_llm_chat_completion', return_value=MagicMock(status_code=200)) as post:
_llm_chat_fast({"llm_model": "m"}, [])
self.assertEqual(post.call_count, 1)
class DeletePictureTests(unittest.TestCase):
"""A picture is a visual block plus an image entry; deleting it must take both, or an empty paragraph is left."""
def layout(self):
return {"pages": [{"page": 1, "width": 595.0, "height": 842.0}], "blocks": [
{"id": "fig", "page": 1, "bbox": [50, 50, 200, 150], "type": "figure", "text": "", "reading_order": 0},
{"id": "tbl", "page": 1, "bbox": [50, 250, 200, 80], "type": "table", "text": "a | b", "reading_order": 1},
{"id": "txt", "page": 1, "bbox": [50, 350, 200, 40], "type": "body", "text": "Real text.", "reading_order": 2},
{"id": "retyped", "page": 1, "bbox": [50, 400, 200, 40], "type": "body", "text": "", "reading_order": 3},
], "images": [
{"id": "p1-region-fig", "page": 1, "bbox": [50, 50, 200, 150], "source_block": "fig", "type": "figure", "filename": "f.png"},
{"id": "p1-region-tbl", "page": 1, "bbox": [50, 250, 200, 80], "source_block": "tbl", "type": "table", "filename": "t.png"},
{"id": "p1-manual-000", "page": 1, "bbox": [300, 50, 100, 100], "filename": "m.png"},
{"id": "p1-stale", "page": 1, "bbox": [50, 400, 200, 40], "source_block": "retyped", "type": "figure", "filename": "s.png"},
]}
def ids(self, layout):
return sorted(b["id"] for b in layout["blocks"]), sorted(i["id"] for i in layout["images"])
def test_deleting_the_image_takes_its_block_too(self):
from core.magazine import delete_region
blocks, images = self.ids(delete_region(self.layout(), "p1-region-fig"))
self.assertNotIn("fig", blocks)
self.assertNotIn("p1-region-fig", images)
def test_deleting_the_block_takes_its_image_too(self):
from core.magazine import delete_region
blocks, images = self.ids(delete_region(self.layout(), "tbl"))
self.assertNotIn("tbl", blocks)
self.assertNotIn("p1-region-tbl", images)
def test_nothing_else_is_touched(self):
from core.magazine import delete_region
blocks, images = self.ids(delete_region(self.layout(), "p1-region-fig"))
self.assertEqual(blocks, ["retyped", "tbl", "txt"])
self.assertEqual(images, ["p1-manual-000", "p1-region-tbl", "p1-stale"])
def test_a_hand_drawn_picture_has_no_twin_and_a_text_block_has_none(self):
from core.magazine import delete_region, region_ids_with
self.assertEqual(region_ids_with(self.layout(), "p1-manual-000"), {"p1-manual-000"})
self.assertEqual(region_ids_with(self.layout(), "txt"), {"txt"})
self.assertEqual(self.ids(delete_region(self.layout(), "txt"))[0], ["fig", "retyped", "tbl"])
def test_an_image_whose_block_was_retyped_to_text_does_not_take_that_text_block(self):
from core.magazine import delete_region
blocks, images = self.ids(delete_region(self.layout(), "p1-stale"))
self.assertIn("retyped", blocks)
self.assertNotIn("p1-stale", images)
def test_the_input_layout_is_not_modified_and_an_unknown_id_changes_nothing(self):
from core.magazine import delete_region
layout = self.layout()
snapshot = json.dumps(layout, sort_keys=True)
delete_region(layout, "p1-region-fig")
self.assertEqual(json.dumps(layout, sort_keys=True), snapshot)
self.assertEqual(self.ids(delete_region(layout, "nope")), self.ids(layout))
def test_the_real_page_159_pictures_leave_nothing_behind(self):
"""Measured on the production c't issue: deleting either picture of page 159 used to leave its empty figure block."""
import copy
from core.magazine import delete_region
path = Path(__file__).resolve().parents[1] / "config" / "reader_library" / "bd871c7c22f0449984d3b0e3d603d6d6" / "layout.json"
if not path.is_file():
self.skipTest("the production layout is not on this machine")
real = json.loads(path.read_text("utf-8"))
for image_id, block_id in (("p0159-region-b0159-v008", "b0159-v008"), ("p0159-region-b0159-v011", "b0159-v011")):
after = delete_region(copy.deepcopy(real), image_id)
self.assertNotIn(block_id, {b["id"] for b in after["blocks"]})
self.assertNotIn(image_id, {i["id"] for i in after["images"]})
self.assertEqual(len(after["blocks"]), len(real["blocks"]) - 1)
self.assertEqual(len(after["images"]), len(real["images"]) - 1)
def test_clean_up_retypes_what_earlier_deletes_left_behind_but_not_real_pictures(self):
from core.magazine import clean_up_regions
layout = self.layout()
layout["images"] = [i for i in layout["images"] if i["id"] != "p1-region-fig"] # the old bug's leftover: empty figure, no image
out, report = clean_up_regions(layout)
types = {b["id"]: (b["type"], b.get("region_label", "")) for b in out["blocks"]}
self.assertEqual(types["fig"], ("furniture", "ocr_noise"))
self.assertEqual(types["tbl"][0], "table") # has an image entry
self.assertEqual(types["txt"][0], "body")
layout = self.layout() # an empty figure that still has its picture is a real picture
self.assertEqual({b["id"]: b["type"] for b in clean_up_regions(layout)[0]["blocks"]}["fig"], "figure")
class DeletePictureRouteTests(unittest.TestCase):
def setUp(self):
from fastapi import FastAPI
from fastapi.testclient import TestClient
import routes.magazine as routes
self.lib = Path(tempfile.mkdtemp(prefix="ttvc-lib-"))
from unittest import mock
p = mock.patch.object(routes, "_LIB", self.lib)
p.start(); self.addCleanup(p.stop)
app = FastAPI(); app.include_router(routes.router)
self.client = TestClient(app)
self.doc = self.client.post("/api/magazine/docs", json={"title": "T"}).json()["id"]
self.d = self.lib / self.doc
base = DeletePictureTests().layout()
(self.d / "layout.json").write_text(json.dumps(base))
(self.d / "articles.json").write_text(json.dumps({"articles": [
{"id": "a1", "title": "T", "pages": [1], "blocks": ["fig", "txt"], "images": ["p1-region-fig", "p1-manual-000"], "status": "detected"},
{"id": "a2", "title": "U", "pages": [1], "blocks": ["tbl"], "images": ["p1-region-tbl"], "status": "detected"}]}))
def state(self):
layout = json.loads((self.d / "layout.json").read_text())
arts = {a["id"]: a for a in json.loads((self.d / "articles.json").read_text())["articles"]}
return layout, arts
def test_deleting_an_image_row_leaves_no_twin_in_the_layout_or_the_articles(self):
r = self.client.delete(f"/api/magazine/{self.doc}/pages/1/regions/p1-region-fig")
self.assertEqual(r.status_code, 200, r.text)
layout, arts = self.state()
self.assertNotIn("fig", {b["id"] for b in layout["blocks"]})
self.assertEqual(arts["a1"]["blocks"], ["txt"]) # the block half is forgotten as well
self.assertEqual(arts["a1"]["images"], ["p1-manual-000"])
self.assertEqual(arts["a2"]["blocks"], ["tbl"]) # another article is untouched
def test_deleting_the_block_row_takes_the_image_too(self):
self.assertEqual(self.client.delete(f"/api/magazine/{self.doc}/pages/1/regions/tbl").status_code, 200)
layout, arts = self.state()
self.assertNotIn("p1-region-tbl", {i["id"] for i in layout["images"]})
self.assertEqual((arts["a2"]["blocks"], arts["a2"]["images"]), ([], []))
def test_bulk_delete_removes_both_halves_and_forgets_both_in_the_articles(self):
r = self.client.post(f"/api/magazine/{self.doc}/pages/1/regions/bulk", json={"ids": ["p1-region-fig", "tbl"], "action": "delete"})
self.assertEqual(r.status_code, 200, r.text)
layout, arts = self.state()
self.assertEqual(sorted(b["id"] for b in layout["blocks"]), ["retyped", "txt"])
self.assertEqual(sorted(i["id"] for i in layout["images"]), ["p1-manual-000", "p1-stale"])
self.assertEqual((arts["a1"]["blocks"], arts["a1"]["images"]), (["txt"], ["p1-manual-000"]))
self.assertEqual((arts["a2"]["blocks"], arts["a2"]["images"]), ([], []))
def test_undo_after_deleting_a_picture_brings_both_halves_back(self):
token = self.client.delete(f"/api/magazine/{self.doc}/pages/1/regions/p1-region-fig").json()["undo"]
self.assertEqual(self.client.post(f"/api/magazine/{self.doc}/undo", json={"token": token}).status_code, 200)
layout, arts = self.state()
self.assertIn("fig", {b["id"] for b in layout["blocks"]})
self.assertIn("p1-region-fig", {i["id"] for i in layout["images"]})
self.assertEqual(arts["a1"]["blocks"], ["fig", "txt"])