diff --git a/CHANGELOG.md b/CHANGELOG.md index c853979..f5111ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi ## [Unreleased] ### Fixed +- **A turned page could keep showing its old picture, which made turning look broken (and invited pressing the buttons again).** Page pictures had no cache header until recently, and a browser may treat an unchanged URL as fresh for days, so after a successful turn the review could keep showing the old image. The layout now stamps every page with its picture's version (the file's modification time) and every page picture URL (review, page grid, reading pane, cover, lock-screen artwork) carries it, so a changed picture is a new URL in any browser. Turns also add up (right + half turn = three quarters), so the toolbar now says how far the page is turned from the scan ("Turned 270° clockwise from the scan") with an **Undo turns** button back to the PDF's own orientation, and a second turn or re-read of a page that is still being processed is refused (HTTP 409) instead of stacking. `extract_pages` records a stored turn in the page's layout entry, so it survives a re-import visibly. Verified end to end with the real OCR engines on a scratch copy of c't p. 156: turn 90 gives an upright 886x1039 page read at word-F1 0.878, Undo turns restores the 1039x886 original and clears the stored turn. - **A page's picture could vanish from the review (seen on c't p. 176).** One region held a 3,408-character token with no spaces (`Giro + 2.1.1.1.1…`, the OCR engine stuck in a loop). Its width was taken as the minimum width of the list column, which widened to 20,305 px and squeezed the page-picture column to 2 px. The review grid's tracks can now shrink (`minmax(0, …)`), and text in the review list, reading view, figure diffs and page cards wraps anywhere, so no content can collapse the layout again; a long URL or a loop now simply wraps. The cause is also handled: *Clean up boxes* (and every import and re-read, which run it) re-type a region that is mostly one endless repeated letter/digit token as OCR noise (kept, visible, reversible in the type picker, not read aloud). Leader dots, rules, section numbers, phone numbers, prices and URLs are not affected; run over all three of your magazines (about 6,000 regions) the rule flags exactly that one region on page 176 in each and nothing else. To clear it on an existing magazine press *Clean up boxes* or *Fix this page automatically*. diff --git a/Manual.md b/Manual.md index 5c8c3b7..cbdd30b 100644 --- a/Manual.md +++ b/Manual.md @@ -407,6 +407,13 @@ 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. +**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 +a second turn. The turned picture appears as soon as the page is re-read; page pictures are requested under a version number, so a browser +that remembered the old picture fetches the new one. Note what a PDF's own orientation can be: some pages are stored sideways on purpose +(c't page 156 is a landscape page that reads sideways), and one **Turn right** is then the whole fix. + **What is replaced.** The page's boxes and text are replaced by the new reading, so corrections you made to that page by hand are lost, and articles that used the page go back to "not approved" where the new boxes do not line up with the old ones. **Undo does not cover a turn** (it changes the page image); if you turned the wrong way, press the opposite button. diff --git a/core/magazine.py b/core/magazine.py index 204d3c9..7bf0622 100644 --- a/core/magazine.py +++ b/core/magazine.py @@ -2670,6 +2670,7 @@ def extract_pages(pdf_path: Path, out_dir: Path, render_dpi: int = 110, "page": page_no, "width": rect.width, "height": rect.height, + "rotation": extra_turn, "blocks": blocks, "image_boxes": image_boxes, "images": page_images, @@ -2734,7 +2735,8 @@ def extract_pages(pdf_path: Path, out_dir: Path, render_dpi: int = 110, return { "page_count": doc.page_count, - "pages": [{"page": p["page"], "width": p["width"], "height": p["height"]} + "pages": [{"page": p["page"], "width": p["width"], "height": p["height"], + **({"rotation": p["rotation"]} if p.get("rotation") else {})} for p in raw_pages], "blocks": [b.to_json() for b in all_blocks], "images": all_images, diff --git a/routes/magazine.py b/routes/magazine.py index 2c73df7..b72f810 100644 --- a/routes/magazine.py +++ b/routes/magazine.py @@ -40,6 +40,7 @@ from core.constants import CONFIG_DIR from core.database import ( background_job_create, background_job_get, + background_job_list, background_job_update, ) from core import spelling @@ -551,9 +552,20 @@ async def magazine_layout(doc_id: str, page: int | None = None): layout = dict(layout) layout["blocks"] = [b for b in layout.get("blocks", []) if b.get("page") == page] layout["pages"] = [p for p in layout.get("pages", []) if p.get("page") == page] + # The page picture's URL carries this number, so a turned or re-rendered page is fetched again by every browser + # whatever it cached earlier (a browser may treat an unchanged URL as fresh for days). + layout = dict(layout) + layout["pages"] = [{**p, "image_v": _page_image_version(d, p.get("page"))} for p in layout.get("pages", [])] return layout +def _page_image_version(d: Path, page: Any) -> int: + try: + return int((d / "pages" / f"{int(page):04d}.png").stat().st_mtime) + except (OSError, TypeError, ValueError): + return 0 + + def _next_article_id(articles: list[dict]) -> str: nums = [int(m.group(1)) for a in articles if (m := re.fullmatch(r"article-(\d+)", a.get("id") or ""))] @@ -1054,6 +1066,22 @@ def _reocr_page_sync(doc_id: str, job_id: str, page: int, background_job_update(job_id, stage="failed", done=True, error=str(exc)[:1000]) +_PAGE_JOB_KINDS = ("magazine-rotate-page", "magazine-reocr-page") +_PAGE_JOB_STALE_SECONDS = 15 * 60 # a job that has not reported for this long died with the server + + +def _page_job_running(doc_id: str, page: int) -> bool: + """True while a turn or re-read of this page is in flight. Two at once would read and rewrite the same image and + layout; pressing the button again because the first one seemed to do nothing must not stack a second turn.""" + now = time.time() + for kind in _PAGE_JOB_KINDS: + for job in background_job_list(kind, 50): + if (job and not job.get("done") and job.get("fingerprint") == f"{doc_id}:{page}" + and now - float(job.get("updated") or 0) < _PAGE_JOB_STALE_SECONDS): + return True + return False + + @router.post("/api/magazine/{doc_id}/pages/{page}/reocr") async def magazine_reocr_page(doc_id: str, page: int, request: Request): """Force one page through the configured vision OCR engine again, @@ -1083,6 +1111,8 @@ async def magazine_reocr_page(doc_id: str, page: int, request: Request): vision_ocr_fallback_model = str( body.get("vision_ocr_fallback_model") or settings.get("vision_ocr_fallback_model") or "").strip() or None + if _page_job_running(doc_id, page): + raise HTTPException(409, f"Page {page} is still being read; wait for it to finish") job_id = uuid.uuid4().hex background_job_create(job_id, "magazine-reocr-page", fingerprint=f"{doc_id}:{page}") asyncio.create_task( @@ -1119,6 +1149,8 @@ async def magazine_rotate_page(doc_id: str, page: int, request: Request): url, model, fb_url, fb_model = _vision_args(settings) if not (url and model): raise HTTPException(400, "No vision OCR engine configured -- set one in Settings (turning re-reads the page)") + if _page_job_running(doc_id, page): + raise HTTPException(409, f"Page {page} is still being turned or read; wait for it to finish before turning it again") job_id = uuid.uuid4().hex background_job_create(job_id, "magazine-rotate-page", fingerprint=f"{doc_id}:{page}") asyncio.create_task(asyncio.to_thread(_reocr_page_sync, doc_id, job_id, page, url, model, fb_url, fb_model, degrees)) diff --git a/static/js/magazine.js b/static/js/magazine.js index e37e6a2..2804dd9 100644 --- a/static/js/magazine.js +++ b/static/js/magazine.js @@ -175,6 +175,15 @@ function setupMagIngestPreviewResize() { }); } +// The URL of a page's picture. It carries the picture's version (its modification time, stamped on each page by the +// layout endpoint), so when a page is turned or re-rendered the URL changes and every browser fetches it again instead +// of trusting a copy it cached under the old one. +function magPageSrc(page, docId) { + const id = docId || state.doc?.id; + const meta = (state.layout?.pages || []).find((p) => p.page === Number(page)); + return '/api/magazine/' + id + '/pages/' + page + (meta && meta.image_v ? '?v=' + meta.image_v : ''); +} + // ── Library ────────────────────────────────────────────────────────────────── async function magLoadLibrary() { @@ -773,7 +782,7 @@ async function pollIngest() { if (data.preview.page !== lastPreviewPage) { lastPreviewPage = data.preview.page; const img = $('mag-ingest-preview-img'); - if (img) img.src = '/api/magazine/' + state.doc.id + '/pages/' + data.preview.page; + if (img) img.src = '/api/magazine/' + state.doc.id + '/pages/' + data.preview.page + '?t=' + Date.now(); } magRenderPreviewBoxes(data.preview.page, data.preview.boxes || [], data.preview.stage); const textEl = $('mag-ingest-preview-text'); @@ -1242,7 +1251,7 @@ function renderPagesView(host) { }; html += '
' + '' + + 'Page ' + p + '' + 'Page ' + p + '' + '' + (arts.length ? arts.length + (arts.length === 1 ? ' article' : ' articles') : 'no article') + '' + magQualityBadge(quality.get(p), p) + @@ -1793,7 +1802,7 @@ function magReviewPageColumn(page, wrapId, interactive) { let pageHtml = ''; if (page != null) { pageHtml = '
' + - 'Page ' + page + '' + + 'Page ' + page + '' + (interactive ? '' : ''); regions.forEach((r, i) => { if (!pageMeta) return; @@ -2003,6 +2012,7 @@ function renderReview() { '' + + magTurnStateHtml() + '' + '' + @@ -3618,6 +3628,22 @@ async function magReocrPage() { renderReview(); } +// 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) { + const meta = (state.layout?.pages || []).find((p) => p.page === Number(page)); + return ((Number(meta?.rotation) || 0) % 360 + 360) % 360; +} + +function magTurnStateHtml() { + const now = magTurnNow(state.reviewPage); + if (!now) return ''; + return 'Turned ' + now + '° clockwise from the scan' + + ''; +} + // A page scanned sideways or upside down: turn the page itself, then read it again. The boxes and text // produced from the wrong orientation are replaced, so this asks first; approvals carry over where the new // regions line up with the old ones. Turning is not covered by Undo (it rewrites the page image), so the @@ -3634,7 +3660,7 @@ async function magRotatePage(degrees) { { title: what + '?', okLabel: 'Turn and read again' }); if (!ok) return; 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-reocr-btn'].map($).filter(Boolean); + 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 || ''; }; busy(true, 'Turning page ' + page + '…'); @@ -4732,7 +4758,7 @@ function showPage(page, bbox) { const img = $('mag-page-img'); const box = $('mag-page-box'); if (!img) return; - const src = '/api/magazine/' + state.doc.id + '/pages/' + page; + const src = magPageSrc(page); if (img.getAttribute('src') !== src) img.setAttribute('src', src); img.setAttribute('alt', 'Page ' + page + ' of ' + (state.doc.title || 'the magazine')); const label = $('mag-page-label'); @@ -5165,7 +5191,7 @@ function magPlaylistPlay(articleId, options) { const cover = $('mag-playlist-cover'); if (cover) { const firstPage = (a.pages || [])[0]; - if (firstPage) { cover.src = '/api/magazine/' + state.doc.id + '/pages/' + firstPage; cover.hidden = false; } + if (firstPage) { cover.src = magPageSrc(firstPage); cover.hidden = false; } else cover.hidden = true; } renderPlaylistTracks(); @@ -5636,7 +5662,7 @@ function magMediaSessionSync() { title: a.title || 'Untitled', artist: a.author || state.doc?.title || '', album: [state.doc?.title, state.doc?.issue].filter(Boolean).join(' — '), - artwork: first ? [{ src: '/api/magazine/' + state.doc.id + '/pages/' + first, type: 'image/png' }] : [], + artwork: first ? [{ src: magPageSrc(first), type: 'image/png' }] : [], }); navigator.mediaSession.playbackState = audio.paused ? 'paused' : 'playing'; } catch (_) { /* metadata is presentation only */ } diff --git a/static/style.css b/static/style.css index 28db6f9..b445678 100644 --- a/static/style.css +++ b/static/style.css @@ -5654,3 +5654,4 @@ details.mag-report-group summary { cursor: pointer; margin-bottom: 6px; } grid tracks are min-content by default, so every track that holds text is allowed to shrink and text wraps anywhere. */ .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); } diff --git a/tests/magazine.spec.js b/tests/magazine.spec.js index 605b260..a69712c 100644 --- a/tests/magazine.spec.js +++ b/tests/magazine.spec.js @@ -2242,3 +2242,70 @@ test.describe('Unbreakable text cannot collapse the review', () => { await expect(page.locator('[data-block-row="b9"]').getByRole('button', { name: /Delete region/ })).toBeVisible(); }); }); + +test.describe('A turned page shows at once, and its net turn is visible', () => { + async function open(page, { rotation = 0 } = {}) { + await mockBackend(page); + const state = { v: 100, rotation, rotates: [] }; + await page.route(`**/api/magazine/${DOC_ID}/layout*`, route => route.fulfill({ + json: { ...LAYOUT, pages: LAYOUT.pages.map(p => ({ ...p, image_v: state.v, ...(state.rotation ? { rotation: state.rotation } : {}) })) } })); + await page.route(`**/api/magazine/${DOC_ID}/pages/3/rotate`, route => { + state.rotates.push(route.request().postDataJSON()); + state.v += 50; state.rotation = (state.rotation + ((state.rotates.at(-1).degrees % 360) + 360)) % 360; // what the server would now report + return route.fulfill({ json: { job_id: 'rot1', degrees: state.rotates.at(-1).degrees } }); + }); + await page.route('**/api/jobs/rot1', route => route.fulfill({ json: { done: true, error: null, detail: 'done' } })); + await page.goto('/', { waitUntil: 'domcontentloaded' }); + await page.waitForSelector('#app-skeleton', { state: 'detached', timeout: 15_000 }); + await navTo(page, 's-magazine'); + await page.locator('#mag-library-list .mag-card').first().click(); + await page.locator('#mag-contents-view-toggle').click(); + await page.locator('.mag-card-wrap').filter({ hasText: 'Page 3' }).getByRole('button', { name: /^Review/ }).click(); + await page.waitForSelector('#mag-undo-btn'); + return state; + } + + test('every picture of a page carries its version, so a changed picture is a new URL', async ({ page }) => { + await open(page); + await expect(page.locator('#mag-review-page-wrap img')).toHaveAttribute('src', new RegExp(`/pages/3\\?v=100$`)); + }); + + test('after a turn the page picture is requested again under its new version', async ({ page }) => { + const state = await open(page); + await page.getByRole('button', { name: /Turn right/ }).click(); + await page.getByRole('alertdialog').getByRole('button', { name: 'Turn and read again' }).click(); + await expect.poll(() => state.rotates.length).toBe(1); + await expect(page.locator('#mag-review-page-wrap img')).toHaveAttribute('src', /\/pages\/3\?v=150$/); + }); + + test('the page grid thumbnails use the version too', async ({ page }) => { + await open(page); + await page.getByRole('button', { name: /^Contents$/ }).click(); + await expect(page.locator('.mag-card-wrap[data-page="3"] img.mag-card-thumb')).toHaveAttribute('src', /\/pages\/3\?v=100$/); + }); + + test('a page that has not been turned shows no turn state', async ({ page }) => { + await open(page); + await expect(page.locator('#mag-turn-state')).toHaveCount(0); + await expect(page.getByRole('button', { name: /Undo turns/ })).toHaveCount(0); + }); + + test('a turned page says by how much, and Undo turns sends what returns it to the PDF orientation', async ({ page }) => { + const state = await open(page, { rotation: 270 }); + await expect(page.locator('#mag-turn-state')).toHaveText('Turned 270° clockwise from the scan'); + await page.getByRole('button', { name: /Undo turns/ }).click(); + await page.getByRole('alertdialog').getByRole('button', { name: 'Turn and read again' }).click(); + await expect.poll(() => state.rotates).toEqual([{ degrees: 90 }]); // 270 + 90 = a full circle + await expect(page.locator('#mag-turn-state')).toHaveCount(0); // and the state line is gone once it is back to 0 + }); + + test('the server refusing a second turn while one runs is explained and the buttons come back', async ({ page }) => { + await open(page); + await page.route(`**/api/magazine/${DOC_ID}/pages/3/rotate`, route => route.fulfill({ + status: 409, json: { detail: 'Page 3 is still being turned or read; wait for it to finish before turning it again' } })); + await page.getByRole('button', { name: /Turn right/ }).click(); + await page.getByRole('alertdialog').getByRole('button', { name: 'Turn and read again' }).click(); + await expect(page.locator('.toast, #toast, [role="status"]').filter({ hasText: 'still being turned or read' }).first()).toBeVisible(); + await expect(page.getByRole('button', { name: /Turn right/ })).toBeEnabled(); + }); +}); diff --git a/tests/test_magazine_rotation.py b/tests/test_magazine_rotation.py index 2cd1a95..cfa3b5e 100644 --- a/tests/test_magazine_rotation.py +++ b/tests/test_magazine_rotation.py @@ -298,6 +298,8 @@ class RenderTests(unittest.TestCase): self.assertTrue(any(b["text"].startswith("A sentence") for b in plain["blocks"])) turned = mag.extract_pages(d / "source.pdf", d / "turned", render_dpi=50, page_rotations={1: 90}) self.assertEqual((turned["pages"][0]["width"], turned["pages"][0]["height"]), (200.0, 300.0)) + self.assertEqual(turned["pages"][0].get("rotation"), 90) # the stored turn is visible in the layout again + self.assertNotIn("rotation", plain["pages"][0]) with Image.open(d / "turned" / "pages" / "0001.png") as im, Image.open(d / "plain" / "pages" / "0001.png") as ref: self.assertEqual(im.size, (ref.size[1], ref.size[0])) # The PDF's own text boxes belong to the unturned page; on a turned page only OCR of the render counts. @@ -311,6 +313,75 @@ class RenderTests(unittest.TestCase): self.assertEqual([x["text"] for x in a["blocks"]], [x["text"] for x in b["blocks"]]) +class ImageVersionAndJobGuardTests(unittest.TestCase): + def setUp(self): + from fastapi import FastAPI + from fastapi.testclient import TestClient + import routes.magazine as routes + self.routes = routes + self.lib = Path(tempfile.mkdtemp(prefix="ttvc-lib-")) + 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 + (self.d / "pages").mkdir() + self.png = self.d / "pages" / "0002.png" + Image.new("RGB", (50, 80), "white").save(self.png) + (self.d / "layout.json").write_text(json.dumps({"pages": [{"page": 2, "width": 50.0, "height": 80.0}, {"page": 3, "width": 50.0, "height": 80.0}], + "blocks": [], "images": []})) + + def test_each_page_is_stamped_with_its_picture_version_and_it_changes_with_the_picture(self): + first = self.client.get(f"/api/magazine/{self.doc}/layout").json()["pages"] + self.assertEqual(first[0]["image_v"], int(self.png.stat().st_mtime)) + self.assertEqual(first[1]["image_v"], 0) # page 3 has no picture: 0, not an error + os.utime(self.png, (self.png.stat().st_atime, self.png.stat().st_mtime + 100)) + again = self.client.get(f"/api/magazine/{self.doc}/layout").json()["pages"] + self.assertEqual(again[0]["image_v"], first[0]["image_v"] + 100) + + def test_the_stamp_is_also_there_when_one_page_is_asked_for(self): + pages = self.client.get(f"/api/magazine/{self.doc}/layout?page=2").json()["pages"] + self.assertEqual([p["page"] for p in pages], [2]) + self.assertGreater(pages[0]["image_v"], 0) + + def test_the_stamp_is_not_written_into_the_saved_layout(self): + self.client.get(f"/api/magazine/{self.doc}/layout") + self.assertNotIn("image_v", json.loads((self.d / "layout.json").read_text())["pages"][0]) + + def job(self, **kw): + base = {"done": False, "fingerprint": f"{self.doc}:2", "updated": __import__("time").time()} + base.update(kw) + return base + + def running(self, jobs, page=2): + with mock.patch.object(self.routes, "background_job_list", side_effect=lambda kind, limit=50: jobs if kind == "magazine-rotate-page" else []): + return self.routes._page_job_running(self.doc, page) + + def test_a_job_in_flight_for_the_same_page_counts(self): + self.assertTrue(self.running([self.job()])) + + def test_finished_stale_and_other_page_jobs_do_not(self): + self.assertFalse(self.running([self.job(done=True)])) + self.assertFalse(self.running([self.job(updated=__import__("time").time() - 3600)])) # died with the server + self.assertFalse(self.running([self.job(fingerprint=f"{self.doc}:3")])) + self.assertFalse(self.running([self.job(fingerprint="other:2")])) + self.assertFalse(self.running([])) + + def test_a_second_turn_or_re_read_while_one_runs_is_refused_and_changes_nothing(self): + before = self.png.read_bytes() + settings = {"vision_ocr_url": "http://ocr", "vision_ocr_model": "m"} + with mock.patch.object(self.routes, "_load_settings", return_value=settings), \ + mock.patch.object(self.routes, "_page_job_running", return_value=True), \ + mock.patch.object(self.routes.asyncio, "create_task") as task: + turn = self.client.post(f"/api/magazine/{self.doc}/pages/2/rotate", json={"degrees": 90}) + reread = self.client.post(f"/api/magazine/{self.doc}/pages/2/reocr", json={}) + self.assertEqual((turn.status_code, reread.status_code), (409, 409)) + self.assertIn("wait for it to finish", turn.json()["detail"]) + task.assert_not_called() + self.assertEqual(self.png.read_bytes(), before) + + class EndpointTests(unittest.TestCase): def setUp(self): from fastapi import FastAPI