From 6529d9ea31bad1390d879370a805e81fd320b66d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 03:09:46 +0200 Subject: [PATCH 1/4] =?UTF-8?q?docs(guide):=20spec=20obs=C5=82ugi=20popup?= =?UTF-8?q?=C3=B3w=20w=20przewodniku=20PDF?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opisuje zamianę `_Capture.page` z „okna głównego" na „okno aktywne", cykl życia sterowany zamrożoną flagą `opens_popup` zamiast heurystyki, oraz letterboxing popupu w CSS zamiast przetwarzania obrazu. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PEZwp8YivARFrZSjrSUnD5 --- .../2026-07-25-guide-popup-support-design.md | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-25-guide-popup-support-design.md diff --git a/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md b/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md new file mode 100644 index 0000000..95a465d --- /dev/null +++ b/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md @@ -0,0 +1,107 @@ +# Obsługa popupów w `guidebot guide` (PDF) + +Data: 2026-07-25 + +## Problem + +`guide` odrzuca każdy scenariusz, w którym jakiś krok otwiera nowe okno: + +``` +BŁĄD: scenariusze z popupem nie są obsługiwane w `guide` v1 (krok otwiera nowe okno) +``` + +Blokada siedzi w `guide/prolog.py` (`scan_for_blockers`) i wyzwala ją zamrożona +flaga `opens_popup: true`. Dotyczy to całej klasy scenariuszy, których sednem +jest logowanie — a więc dokładnie tych, dla których przewodnik krok-po-kroku ma +największy sens. + +Blokada nie jest kaprysem: `guide` nie ma **żadnego** cyklu życia okien. +`_Capture.page` jest ustawiane raz z okna głównego i każde z pięciu wywołań +`_screenshot(cap.page, …)` (`replay.py:216, 268, 328, 396, 403`) fotografuje +właśnie je. Krok działający na popupie szukałby swojego celu w oknie głównym +i padał z niezgodnością tożsamości — kilka kroków za późno, w niezrozumiałym +miejscu. Odrzucenie z góry było uczciwsze niż taka awaria. + +## Rozwiązanie + +Zamiast dokładać wykrywanie okien, zmieniamy jedno pojęcie: `_Capture.page` +przestaje znaczyć „okno główne", a zaczyna znaczyć **okno aktywne**. Pętla +przechwytywania nie dowiaduje się o tym niczego — te same pięć wywołań +`_screenshot(cap.page, …)` fotografuje inną wartość. + +### Cykl życia sterowany sidecarem + +Popupu nie wykrywamy heurystycznie. `compile` już zamraża `opens_popup: true` +(`recorder/compile/run.py:365`), więc: + +* krok `click` z tą flagą owija kliknięcie w `context.expect_page()` + i po powrocie przełącza aktywną parę `(page, recorder)` na nowe okno; +* krok `closeWindow` zamyka popup i przywraca parę okna głównego. + +Świadomie **nie** sięgamy po `render/popup_detect.py` ani `render/popup_crop.py` +(789 linii). To maszyneria wideo: wykrywanie momentu z kwantem ciszy, przejścia +float/slide, kadrowanie klatek. PDF nie ma osi czasu, więc nie ma czego kadrować. + +Popup dostaje własny `Recorder` — bez `frame=`, bo nie żyje w iframie powłoki +chrome. Pasek chrome na popupie zostaje sterowany istniejącym `cfg.popup.is_bare`, +które `run_guide` już przekazuje do `Chrome(...)`. + +`closeWindow` przestaje być komendą wyłącznie narracyjną. Dziś siedzi +w `NARRATION_ONLY_KINDS` z komentarzem „popup bookkeeping the guide already +rejects" — dostaje własny rodzaj strony i własną fazę, bo od teraz **wykonuje** +pracę w przeglądarce. + +### Płótno — w CSS, nie w obrazie + +Zrzut popupu zostaje w naturalnym rozmiarze okna. Wyśrodkowanie na płótnie +o proporcjach okna głównego robi `layout.py`: bez przetwarzania obrazu i bez +nowej zależności. + +Jeden haczyk, i to on decyduje o kształcie zmiany. Dziś SVG z adnotacjami leży +`inset: 0` nad `.shot`, a `img` wypełnia ten box — więc oba układy współrzędnych +się pokrywają. Gdy obrazek przestanie wypełniać `.shot`, adnotacje rozjadą się +o wielkość marginesu. Dlatego `img` i `svg` trafiają do wewnętrznego `.plate` +o rozmiarze obrazka, wyśrodkowanego w `.shot`. Dla stron nieletterboxowanych +`.plate` ma `width: 100%` i renderuje się identycznie jak dotąd. + +`GuidePage` dostaje `canvas_size` obok istniejącego `screenshot_size`. +Letterboxing włącza się wtedy i tylko wtedy, gdy oba są znane i różne — czyli +sam fakt „to jest popup" nie jest nigdzie zapisywany jako flaga. Rozmiar +wystarcza. + +## Obsługa błędów + +Popup, który nie pojawi się mimo `opens_popup: true`, to twardy `GuideError` +z `plik:linia` i fragmentem YAML — tak jak każdy inny niespełniony namiar +w `guide`. Bez cichego pomijania: zamrożona flaga mówi, że okno *ma* się otworzyć, +a PDF bez tej strony byłby przewodnikiem, który gubi połowę instrukcji. + +Wywołanie `enter_popup` bez wstrzykniętej fabryki `Recorder`-ów jest błędem +programisty, nie scenariusza, więc leci `RuntimeError`. + +## Testy + +Pakiet ma własny strażnik szwów (`tests/unit/guide/test_capture_seams.py`), +który wymaga, by nazwa patchowana na `capture` była wołana z `capture`. Nowy +przełącznik okien mieszka na `_Capture`, więc go nie dotyczy — ale testy nie +mogą go obchodzić name-importem w siostrzanym module. + +Nowe przypadki: + +* przejście na popup po kroku z `opens_popup` i powrót na `closeWindow`; +* zrzuty po przełączeniu pochodzą z popupu, nie z okna głównego; +* ślad kursora zeruje się na każdej zmianie okna (strzałka między oknami nie ma sensu); +* brak popupu mimo flagi → `GuideError` z `plik:linia`; +* `render_html` letterboksuje wtedy i tylko wtedy, gdy `canvas_size ≠ screenshot_size`, + a adnotacje pozostają w układzie obrazka; +* `scan_for_blockers` przestaje odrzucać popupy, nadal odrzuca nieznane komendy + i nierozwiązane kroki obowiązkowe. + +## Zakres + +Zmiana dotyczy wyłącznie `guidebot_recorder/guide/`. Nie ruszamy `render`, +przejść float/slide ani kadrowania wideo. + +Pliki: `prolog.py`, `replay.py`, `capture.py`, `model.py`, `layout.py`, `guide.py`. +Wszystkie zostają grubo poniżej limitu 600 linii; żadna funkcja nie zbliża się +do CC 10. From 32de4bde280ab91dcf49ab00b92e1febd1d35eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 03:26:57 +0200 Subject: [PATCH 2/4] =?UTF-8?q?feat(guide):=20obs=C5=82u=C5=BC=20popupy=20?= =?UTF-8?q?w=20przewodniku=20PDF=20zamiast=20je=20odrzuca=C4=87?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `guide` odrzucał każdy scenariusz z `opens_popup: true` — a więc całą klasę scenariuszy logowania, dla których przewodnik krok-po-kroku ma największy sens. Odmowa była uczciwa: pakiet nie miał żadnego cyklu życia okien, `_Capture.page` było ustawiane raz z okna głównego, a krok działający na popupie szukałby celu w złym oknie i padał kilka kroków później na niezgodności tożsamości. Zmienia się jedno pojęcie: `_Capture.page` znaczy teraz „okno aktywne", nie „okno główne". Pętla przechwytywania nie dowiaduje się o niczym — te same pięć wywołań `_screenshot(cap.page, ...)` fotografuje inną wartość. Cykl życia sterowany jest zamrożoną flagą, nie heurystyką: klik z `opens_popup` owija się w `context.expect_page()` i przełącza parę `(page, recorder)`, a `closeWindow` — dotąd komenda wyłącznie narracyjna, bo nie miała czego zamykać — dostaje własny rodzaj strony i przełącza z powrotem. Świadomie bez `render/popup_detect.py` i `popup_crop.py`: te 789 linii to maszyneria wideo, a PDF nie ma osi czasu. Popup jest mniejszy od okna głównego, więc `layout.py` centruje go na płótnie o proporcjach okna głównego — w CSS, bez przetwarzania obrazu. Adnotacje przenoszą się z `.shot` do wewnętrznego `.plate` o rozmiarze zrzutu: pinowane do `.shot` rozjechałyby się o szerokość marginesu. Sam fakt „to popup" nie jest nigdzie flagą — różnica `screenshot_size` i `canvas_size` nią jest. Zweryfikowane end-to-end na scenariuszu logowania do Onetu: 8 stron, zrzut popupu 500x670 obok stron 1376x800 okna głównego. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PEZwp8YivARFrZSjrSUnD5 --- guidebot_recorder/guide/capture.py | 15 ++ guidebot_recorder/guide/guide.py | 14 +- guidebot_recorder/guide/layout.py | 43 +++- guidebot_recorder/guide/model.py | 13 +- guidebot_recorder/guide/prolog.py | 25 ++- guidebot_recorder/guide/replay.py | 121 +++++++++- tests/unit/guide/test_capture_popup.py | 291 +++++++++++++++++++++++++ tests/unit/guide/test_layout.py | 68 ++++++ tests/unit/guide/test_prolog.py | 32 ++- 9 files changed, 599 insertions(+), 23 deletions(-) create mode 100644 tests/unit/guide/test_capture_popup.py diff --git a/guidebot_recorder/guide/capture.py b/guidebot_recorder/guide/capture.py index e5fc8fc..f470c6b 100644 --- a/guidebot_recorder/guide/capture.py +++ b/guidebot_recorder/guide/capture.py @@ -37,6 +37,7 @@ _approach_target, _Capture, _click_or_hover_frame, + _close_window_page, _gate_page, _navigate_page, _scroll_page, @@ -95,6 +96,7 @@ "text": _text_page, "scroll": _scroll_page, "wait": _wait_page, + "closeWindow": _close_window_page, } @@ -161,6 +163,7 @@ def _append_step_page(run: _StepRun) -> None: bounds=bounds, ), screenshot_size=run.size, + canvas_size=cap.canvas, ) ) @@ -181,6 +184,11 @@ async def _action_page(run: _StepRun) -> None: return await cap.recorder.apply_readiness(run.action.expect) _append_step_page(run) + # Last, and the order is the point: this step's page shows the window the + # reader clicked *in*, photographed before the click. Only once that page is + # built does the popup become the window the following steps are read against. + if run.popup is not None: + cap.enter_popup(run.popup) async def capture_pages( @@ -195,6 +203,7 @@ async def capture_pages( pause_on_error: bool = False, sensitive_values: Iterable[str] = (), selects: Selects | None = None, + make_recorder: Callable[[Page], Recorder] | None = None, ) -> list[GuidePage]: """Replay the compiled scenario, keeping one annotated frame per step. @@ -203,6 +212,11 @@ async def capture_pages( — the guide's half of the readiness barrier compile and render also take. See :meth:`~guidebot_recorder.guide.replay._Capture.await_selects_ready` for where it is taken and why. + + ``make_recorder`` builds the ``Recorder`` for a popup window; omit it for a + scenario that never opens one. It is injected rather than built here because a + ``Recorder`` needs the caller's ``Overlay`` — see + :meth:`~guidebot_recorder.guide.replay._Capture.enter_popup`. """ cap = _Capture( @@ -216,6 +230,7 @@ async def capture_pages( pause_on_error=pause_on_error, sensitive_values=sensitive_values, selects=selects, + make_recorder=make_recorder, ) # Wrapping the iterator (rather than `bar.update(1)` as render does) is what # keeps the count honest here: this loop leaves through a dozen early returns diff --git a/guidebot_recorder/guide/guide.py b/guidebot_recorder/guide/guide.py index 2b90e45..a5b1e91 100644 --- a/guidebot_recorder/guide/guide.py +++ b/guidebot_recorder/guide/guide.py @@ -4,7 +4,7 @@ from pathlib import Path -from playwright.async_api import Browser, Frame +from playwright.async_api import Browser, Frame, Page from guidebot_recorder.chrome import SHELL_URL, Chrome from guidebot_recorder.chrome.framing import install_framing @@ -92,6 +92,17 @@ async def run_guide( # otherwise the recorder drives the page directly (frame=None -> page). recorder = Recorder(page, overlay, frame=site_frame, type_delay_ms=None) + def make_popup_recorder(popup: Page) -> Recorder: + """The popup's recorder — deliberately without a ``frame``. + + The shell iframe belongs to the main window only; a popup is its own + top-level document, so the recorder drives the page directly. Its + chrome bar, if any, is the legacy in-DOM one that ``chrome.js`` mounts + on popup-site documents under ``bare_popups=False``. + """ + + return Recorder(popup, overlay, frame=None, type_delay_ms=None) + shots_dir = out_pdf.parent / (out_pdf.stem + "_shots") pages = await capture_pages( scenario, @@ -104,6 +115,7 @@ async def run_guide( pause_on_error=pause_on_error, sensitive_values=sensitive_values, selects=selects, + make_recorder=make_popup_recorder, ) finally: await context.close() diff --git a/guidebot_recorder/guide/layout.py b/guidebot_recorder/guide/layout.py index 7729d20..463e8ad 100644 --- a/guidebot_recorder/guide/layout.py +++ b/guidebot_recorder/guide/layout.py @@ -16,9 +16,16 @@ page-break-after: always; align-items: start; } .page:last-child { page-break-after: auto; } .shot { position: relative; width: 100%; border: 1px solid #ddd; border-radius: 6px; - overflow: hidden; } -.shot img { width: 100%; display: block; } -.shot svg { position: absolute; inset: 0; width: 100%; height: 100%; } + overflow: hidden; display: flex; align-items: center; justify-content: center; + background: #eceef2; } +/* The still and its annotation layer share one box, so the SVG's viewBox + coordinates and the image's pixels line up. That box is `.plate`, not `.shot`: + a popup still does not fill `.shot`, and an overlay pinned to `.shot` would sit + off the picture by the width of the letterbox margin. At `width: 100%` (every + main-window page) the two boxes coincide and this renders as it always did. */ +.plate { position: relative; width: 100%; } +.plate img { width: 100%; display: block; } +.plate svg { position: absolute; inset: 0; width: 100%; height: 100%; } .side { padding-top: 2mm; } .side .heading { font-size: 20px; font-weight: 700; margin: 0 0 4mm; } .side .body { font-size: 16px; line-height: 1.5; white-space: pre-wrap; } @@ -91,14 +98,42 @@ def _svg(anns: list[Annotation], size: tuple[int, int]) -> str: return "".join(parts) +def _letterbox(page: GuidePage) -> tuple[str, str]: + """Inline styles that centre a smaller still on the page's canvas. + + Returns ``(shot_style, plate_style)``, both empty when the still already fills + the canvas — which is every main-window page, so the common case adds no + markup at all. A popup opens at its own size, and the two sizes differing is + the only signal that it is one (see :class:`GuidePage`). + + ``.shot`` gets the canvas aspect ratio rather than a pixel height so it keeps + scaling with the print column, and ``.plate`` is sized as a percentage of it — + the same ratio the still has to the canvas, so the picture lands unscaled + relative to its neighbours and a 600px-wide popup reads as narrower than the + 1376px window it came from, which is the honest depiction. + """ + + shot, canvas = page.screenshot_size, page.canvas_size + if not shot or not canvas or shot == canvas or not all(canvas): + return "", "" + width = round(100 * shot[0] / canvas[0], 3) + height = round(100 * shot[1] / canvas[1], 3) + return ( + f' style="aspect-ratio:{canvas[0]}/{canvas[1]}"', + f' style="width:{width}%;height:{height}%"', + ) + + def _shot_page(page: GuidePage) -> str: uri = Path(page.screenshot).absolute().as_uri() svg = _svg(page.annotations, page.screenshot_size or (1, 1)) heading = f'
{html.escape(page.heading)}
' if page.heading else "" body = f'
{html.escape(page.text)}
' if page.text else "" + shot_style, plate_style = _letterbox(page) return ( '
' - f'
{svg}
' + f'
' + f'
{svg}
' f'
{heading}{body}
' "
" ) diff --git a/guidebot_recorder/guide/model.py b/guidebot_recorder/guide/model.py index 5804332..c7270cc 100644 --- a/guidebot_recorder/guide/model.py +++ b/guidebot_recorder/guide/model.py @@ -38,7 +38,17 @@ class Annotation: @dataclass class GuidePage: - """One PDF page: a screenshot (or none) plus its description and annotations.""" + """One PDF page: a screenshot (or none) plus its description and annotations. + + ``screenshot_size`` is the still's own pixel size — the coordinate system every + :class:`Annotation` on this page is expressed in. ``canvas_size`` is the size + the page is *presented* at, which for the main window is the same thing and for + a popup window is not: a popup opens at whatever size the site asked for. + + A popup page is not flagged as one anywhere; the two sizes differing IS the + flag. That is deliberate — a separate boolean could disagree with the sizes, + and then the layout would have two sources of truth for one question. + """ kind: Literal["step", "navigate", "slide", "text"] screenshot: Path | None @@ -46,6 +56,7 @@ class GuidePage: heading: str | None annotations: list[Annotation] = field(default_factory=list) screenshot_size: tuple[int, int] | None = None + canvas_size: tuple[int, int] | None = None def page_text(step: Step) -> str: diff --git a/guidebot_recorder/guide/prolog.py b/guidebot_recorder/guide/prolog.py index 7d4dff1..537fce3 100644 --- a/guidebot_recorder/guide/prolog.py +++ b/guidebot_recorder/guide/prolog.py @@ -9,22 +9,27 @@ class GuideError(Exception): - """A scenario the guide cannot render (popup, unresolved mandatory step).""" + """A scenario the guide cannot render (unresolved mandatory step, broken sidecar).""" -PageKind = Literal["gate", "navigate", "slide", "action", "scroll", "text", "wait"] +PageKind = Literal["gate", "navigate", "slide", "action", "scroll", "text", "wait", "closeWindow"] #: Commands the guide replays in the browser off a frozen target. `highlight` is #: one of them even though it performs nothing: the guide still has to resolve the #: target to know where to draw its ellipse. ACTION_KINDS = frozenset({"click", "hover", "enterText", "teach", "select", "highlight"}) #: Commands with no browser work in a still-image pass: a film-only flourish -#: (`desktop`), popup bookkeeping the guide already rejects (`closeWindow`), -#: and a bare `say`. All that survives into the PDF is their narration. -NARRATION_ONLY_KINDS = frozenset({"desktop", "closeWindow", "say"}) +#: (`desktop`) and a bare `say`. All that survives into the PDF is their narration. +#: +#: `closeWindow` used to sit here, back when a popup was rejected outright and the +#: command therefore had nothing to close. It now drives the window switch back to +#: the main page, so it is browser work and has a page kind of its own. +NARRATION_ONLY_KINDS = frozenset({"desktop", "say"}) #: Every command `classify` knows how to place. A command outside this set is a #: gap in the guide, not a no-op — see :func:`scan_for_blockers`. -SUPPORTED_KINDS = ACTION_KINDS | NARRATION_ONLY_KINDS | {"navigate", "slide", "scroll", "wait"} +SUPPORTED_KINDS = ( + ACTION_KINDS | NARRATION_ONLY_KINDS | {"navigate", "slide", "scroll", "wait", "closeWindow"} +) def classify(flat_step: FlatStep) -> PageKind: @@ -38,6 +43,8 @@ def classify(flat_step: FlatStep) -> PageKind: return "slide" if kind == "scroll": return "scroll" + if kind == "closeWindow": + return "closeWindow" if kind in ACTION_KINDS: return "action" if kind == "wait": @@ -47,7 +54,7 @@ def classify(flat_step: FlatStep) -> PageKind: def scan_for_blockers(flat: list[FlatStep], actions: list) -> None: - """Raise GuideError for popups, unsupported commands, or a mandatory unresolved step.""" + """Raise GuideError for unsupported commands or a mandatory unresolved step.""" for flat_step, action in zip(flat, actions, strict=True): kind = flat_step.step.command_kind() @@ -58,10 +65,6 @@ def scan_for_blockers(flat: list[FlatStep], actions: list) -> None: # browser never saw the action, and the failure surfaced steps later # as an identity mismatch against a page the compiler never saw. raise GuideError(f"komenda `{kind}` nie jest obsługiwana w `guide`") - if isinstance(action, CachedAction) and action.opens_popup: - raise GuideError( - "scenariusze z popupem nie są obsługiwane w `guide` v1 (krok otwiera nowe okno)" - ) pending = action is not None and not isinstance(action, CachedAction) mandatory = ( flat_step.branch is None diff --git a/guidebot_recorder/guide/replay.py b/guidebot_recorder/guide/replay.py index fbfe922..c139b3a 100644 --- a/guidebot_recorder/guide/replay.py +++ b/guidebot_recorder/guide/replay.py @@ -23,6 +23,7 @@ from playwright.async_api import Error as PlaywrightError from playwright.async_api import Page +from playwright.async_api import TimeoutError as PlaywrightTimeoutError from tqdm import tqdm from guidebot_recorder.diagnostics import step_banner @@ -65,8 +66,14 @@ class _Capture: """What outlives one step of the capture pass. :attr:`pages` is the document being built, :attr:`skipped_branch` the gate - that turned out to be absent, and :attr:`trail` the cursor memory. The rest - is settled before the loop starts and only read. + that turned out to be absent, and :attr:`trail` the cursor memory. + + :attr:`page` and :attr:`recorder` are the **active window**, not the main one. + A step that opens a popup swaps both (:meth:`enter_popup`) and ``closeWindow`` + swaps them back (:meth:`leave_popup`); every ``_screenshot(cap.page, ...)`` in + this module photographs whichever window is active at the time, which is the + whole of the popup support. The main pair is kept aside in ``__post_init__`` + so leaving never has to reconstruct it. """ scenario: object @@ -79,9 +86,62 @@ class _Capture: pause_on_error: bool sensitive_values: Iterable[str] selects: Selects | None + #: Builds the popup's own ``Recorder``. Injected rather than constructed here + #: because a ``Recorder`` needs the ``Overlay``, which belongs to the caller's + #: browser context — and because a test can hand over a fake without a browser. + #: ``None`` in a capture that never meets a popup. + make_recorder: Callable[[Page], Recorder] | None = None pages: list[GuidePage] = field(default_factory=list) trail: _CursorTrail = field(default_factory=_CursorTrail) skipped_branch: int | None = None + popup: Page | None = None + + def __post_init__(self) -> None: + self._main_page = self.page + self._main_recorder = self.recorder + + @property + def canvas(self) -> tuple[int, int]: + """The size every page is presented at — the main window's viewport. + + A popup still is smaller than this and gets centred on it by the layout; + see :func:`~guidebot_recorder.guide.layout._letterbox`. + """ + + viewport = self.scenario.config.viewport + return (viewport.width, viewport.height) + + def enter_popup(self, popup: Page) -> None: + """Make ``popup`` the active window for the steps that follow.""" + + if self.make_recorder is None: + raise RuntimeError( + "guide nie ma fabryki Recordera, a krok otworzył popup — " + "to błąd wywołania `capture_pages`, nie scenariusza" + ) + self.popup = popup + self.page = popup + self.recorder = self.make_recorder(popup) + # A new window is a new coordinate system: an arrow drawn from the last + # target in the other window would point at nothing on this picture. + self.trail.reset() + + async def leave_popup(self) -> None: + """Close the popup, if one is open, and go back to the main window. + + A no-op when no popup is open: `closeWindow` in a scenario that never + opened one is bookkeeping the author wrote for the film, and the guide has + nothing to undo. + """ + + if self.popup is None: + return + if not self.popup.is_closed(): + await self.popup.close() + self.popup = None + self.page = self._main_page + self.recorder = self._main_recorder + self.trail.reset() def banner(self, entry: FlatStep, entry_index: int, message: str) -> str: """Komunikat kroku z `plik:linia` i fragmentem YAML; sekrety zredagowane.""" @@ -170,6 +230,11 @@ class _StepRun: row_box: dict | None = None row_center: _Point | None = None mark: object = None + #: The window this step's click opened, filled by :func:`_click_or_hover_frame` + #: for a frozen ``opens_popup`` click. The switch itself happens one phase + #: later, in ``capture._action_page``: this step's own page still belongs to + #: the window the reader was looking at when they clicked. + popup: Page | None = None @property def step(self) -> Step: @@ -222,6 +287,7 @@ async def _navigate_page(run: _StepRun) -> None: heading=f"Otwórz adres: {url}", annotations=[], screenshot_size=size, + canvas_size=cap.canvas, ) ) cap.trail.reset() @@ -274,6 +340,7 @@ async def _scroll_page(run: _StepRun) -> None: heading=None, annotations=[], screenshot_size=size, + canvas_size=cap.canvas, ) ) @@ -397,17 +464,67 @@ async def _highlight_frame(run: _StepRun) -> bool: return True +def _opens_popup(action: CompiledAction | None) -> bool: + """Whether ``compile`` froze this action as the one that opens a new window.""" + + return isinstance(action, CachedAction) and action.opens_popup + + +async def _click_into_popup(run: _StepRun) -> Page: + """Click, and hand back the window that click opened. + + The wait wraps the click rather than following it: a popup that opens fast + would have fired its event before a bare ``wait_for_event`` started listening, + and the guide would then blame the site for its own race. + """ + + cap = run.capture + try: + async with cap.page.context.expect_page(timeout=cap.timeout * 1000) as opened: + await run.locator.click() + popup = await opened.value + except PlaywrightTimeoutError as exc: + # The frozen flag says a window opens here, so its absence is a broken + # guide, not a branch to skip: a PDF missing the popup's steps would read + # as a complete set of instructions while omitting half of them. + raise GuideError( + run.banner("krok miał otworzyć nowe okno, a żadne się nie pojawiło") + ) from exc + await popup.wait_for_load_state("domcontentloaded") + return popup + + async def _click_or_hover_frame(run: _StepRun) -> bool: cap = run.capture # frame BEFORE click/hover run.shot, run.size = await _screenshot(cap.page, cap.shots_dir, run.index) if run.act == "hover": await run.locator.hover() + elif _opens_popup(run.action): + run.popup = await _click_into_popup(run) else: await run.locator.click() return True +async def _close_window_page(run: _StepRun) -> None: + """Close the popup, go back to the main window, and narrate if the step does. + + No screenshot of its own: the closing is not a thing the reader does, it is + the film's way of getting back — and the next step photographs the main window + anyway. + """ + + cap = run.capture + await cap.leave_popup() + text = page_text(run.step) + if not text: + return + cap.pages.append( + GuidePage(kind="text", screenshot=None, text=text, heading=None, annotations=[]) + ) + + #: How each frozen action gets photographed. `click` and `hover` share the #: default because they differ only in the call that follows the frame. _FRAMES: dict[str, Callable[[_StepRun], Awaitable[bool]]] = { diff --git a/tests/unit/guide/test_capture_popup.py b/tests/unit/guide/test_capture_popup.py new file mode 100644 index 0000000..ff21e8e --- /dev/null +++ b/tests/unit/guide/test_capture_popup.py @@ -0,0 +1,291 @@ +"""Popup windows in the PDF guide: the active window switches, and switches back. + +Driven with fakes, no browser. The fakes here stand in for the one Playwright API +the feature leans on — ``BrowserContext.expect_page`` — and stay in this file +rather than travelling to ``_capture_helpers.py`` because nothing else uses them. + +What is actually being pinned is an *ordering*: the step that opens a popup is +photographed in the window the reader clicked in, and only the steps after it are +read against the popup. Get that backwards and every guide with a login flow +shows the reader a picture of the wrong window at the moment they act. +""" + +from __future__ import annotations + +import pytest +from playwright.async_api import TimeoutError as PlaywrightTimeoutError + +import guidebot_recorder.guide.capture as capture +from guidebot_recorder.guide.capture import capture_pages +from guidebot_recorder.guide.prolog import GuideError +from guidebot_recorder.models.action import CachedAction +from guidebot_recorder.models.scenario import Scenario, Step + +from ._capture_helpers import ( + FakeRecorder, + _async_none, + _cfg, + _compiled, + _fp, + _target, +) + + +class _Opened: + """Stand-in for the object ``expect_page()`` yields. + + ``None`` for the popup means the event never fired, and the refusal is raised + from ``__aexit__`` — which is where Playwright raises it too, so the + production code's ``try`` has to span the whole ``async with``, not just the + ``await opened.value`` after it. + """ + + def __init__(self, popup): + self._popup = popup + + async def __aenter__(self): + return self + + async def __aexit__(self, *_exc): + if self._popup is None: + raise PlaywrightTimeoutError('Timeout exceeded while waiting for event "page"') + return False + + @property + def value(self): + return self._resolve() + + async def _resolve(self): + return self._popup + + +class FakeContext: + def __init__(self, popup): + self.popup = popup + self.expect_timeouts: list[float | None] = [] + + def expect_page(self, timeout=None): + self.expect_timeouts.append(timeout) + return _Opened(self.popup) + + +class NamedPage: + """A page that says which window a screenshot came from.""" + + def __init__(self, name: str, events: list[str], size=(1280, 720), context=None): + self.name = name + self.events = events + self.viewport_size = {"width": size[0], "height": size[1]} + self.context = context + self.closed = False + + async def screenshot(self, path): + from pathlib import Path + + Path(path).write_bytes(b"fake") + self.events.append(f"shot:{self.name}") + + def is_closed(self): + return self.closed + + async def close(self): + self.closed = True + self.events.append(f"close:{self.name}") + + async def wait_for_load_state(self, _state): + return None + + +def _click(opens_popup=False): + return CachedAction( + action="click", + target=_target(), + expect="none", + opens_popup=opens_popup, + fingerprint=_fp(command_kind="click"), + ) + + +def _windows(events, popup_size=(600, 700)): + """Main window plus the popup its context will hand over.""" + + popup = NamedPage("popup", events, size=popup_size) + main = NamedPage("main", events, context=FakeContext(popup)) + popup.context = main.context + return main, popup + + +async def test_popup_click_switches_the_active_window_and_close_switches_back( + tmp_path, monkeypatch +): + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[ + Step(click="ikona profilu"), + Step(click="pole w wyskakującym oknie"), + Step(close_window=True, say="wracamy do serwisu"), + Step(click="menu w oknie głównym"), + ], + ) + popup_recorders: list[FakeRecorder] = [] + + def make_recorder(page): + assert page is popup + recorder = FakeRecorder(events) + popup_recorders.append(recorder) + return recorder + + await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click(), None, _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=make_recorder, + ) + + # The click that opens the popup is photographed in the MAIN window: that is + # what the reader is looking at when they perform it. + assert [e for e in events if e.startswith(("shot:", "close:"))] == [ + "shot:main", + "shot:popup", + "close:popup", + "shot:main", + ] + assert len(popup_recorders) == 1 + assert popup.closed is True + + +async def test_popup_page_keeps_its_own_size_and_the_main_windows_canvas(tmp_path, monkeypatch): + """Zrzut popupu zostaje w swoim rozmiarze; płótno pozostaje okna głównego. + + Te dwie liczby są jedynym sygnałem „to jest popup" — `layout._letterbox` + włącza się dokładnie na ich różnicy, więc gdyby capture zapisał rozmiar + płótna jako rozmiar zrzutu, letterboxing zniknąłby bez żadnego czerwonego + testu w warstwie układu. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + pages = await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + assert pages[0].screenshot_size == (1280, 720) + assert pages[1].screenshot_size == (600, 700) + # `_cfg()` viewport — the canvas never follows the active window. + assert [p.canvas_size for p in pages] == [(1280, 720), (1280, 720)] + + +async def test_first_popup_step_draws_no_arrow_from_the_other_window(tmp_path, monkeypatch): + """Ślad kursora zeruje się przy zmianie okna. + + Strzałka prowadzi wzrok od poprzedniego celu do bieżącego. Poprzedni cel + został w innym oknie i na innym obrazku, więc narysowana tu strzałka + wskazywałaby współrzędne, które na tej stronie nie znaczą nic. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + pages = await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + assert not [a for a in pages[1].annotations if a.kind == "arrow"] + + +async def test_a_popup_that_never_opens_fails_loudly_with_the_yaml_location(tmp_path, monkeypatch): + """Brak okna mimo zamrożonej flagi to twardy błąd, nie krok do pominięcia. + + Cicha tolerancja dałaby PDF, który wygląda na komplet instrukcji, a gubi + wszystko, co dzieje się w wyskakującym oknie — czyli zwykle sedno scenariusza. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, _popup = _windows(events) + main.context.popup = None # click opens nothing + scenario = Scenario(config=_cfg(), steps=[Step(click="ikona profilu")]) + + with pytest.raises(GuideError, match="nowe okno"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True)]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + +async def test_entering_a_popup_without_a_recorder_factory_blames_the_caller(tmp_path, monkeypatch): + """Brak fabryki to błąd wywołania `capture_pages`, nie wada scenariusza. + + Dlatego `RuntimeError`, a nie `GuideError`: `GuideError` niesie `plik:linia` + i mówi autorowi, co poprawić w YAML-u — a tu nie ma czego poprawiać. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, _popup = _windows(events) + scenario = Scenario(config=_cfg(), steps=[Step(click="ikona profilu")]) + + with pytest.raises(RuntimeError, match="fabryki Recordera"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True)]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + ) + + +async def test_close_window_without_a_popup_is_a_no_op(tmp_path): + """`closeWindow` w scenariuszu bez popupu nie ma czego zamykać. + + Autor pisze go dla filmu; przewodnik nie ma powodu ani padać, ani zamykać + okna głównego — zostaje sama narracja. + """ + + events: list[str] = [] + main, _popup = _windows(events) + scenario = Scenario(config=_cfg(), steps=[Step(close_window=True, say="wracamy")]) + pages = await capture_pages( + scenario, + _compiled([None]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + ) + + assert [p.text for p in pages] == ["wracamy"] + assert "close:main" not in events diff --git a/tests/unit/guide/test_layout.py b/tests/unit/guide/test_layout.py index dbffa5f..96d39b7 100644 --- a/tests/unit/guide/test_layout.py +++ b/tests/unit/guide/test_layout.py @@ -94,6 +94,74 @@ def test_a_select_steps_marks_render_through_the_one_shared_arrowhead(): assert '' in html +def test_main_window_page_is_not_letterboxed(): + """Zrzut wypełniający płótno renderuje się dokładnie jak przed zmianą. + + Letterboxing włącza się wyłącznie przy różnicy rozmiarów, więc każda strona + okna głównego musi zostać bez stylów inline — inaczej zmiana „dla popupów" + po cichu przesunęłaby wszystkie dotychczasowe strony. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/shot.png"), + text="t", + heading=None, + annotations=[], + screenshot_size=(1376, 800), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'class="shot"' in html + assert "aspect-ratio" not in html + assert 'class="plate"' in html + + +def test_popup_page_is_centred_on_the_main_window_canvas(): + """Mniejszy zrzut dostaje płótno o proporcjach okna głównego i własną skalę. + + 600/1376 ≈ 43.605%, 700/800 = 87.5% — obie liczby muszą wyjść z jednego + stosunku, bo inaczej obrazek zostałby rozciągnięty względem sąsiednich stron. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/popup.png"), + text="t", + heading=None, + annotations=[], + screenshot_size=(600, 700), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'style="aspect-ratio:1376/800"' in html + assert 'style="width:43.605%;height:87.5%"' in html + + +def test_annotations_stay_in_the_stills_own_coordinate_system(): + """Adnotacje popupu opisują piksele zrzutu, nie płótna. + + To jest powód, dla którego `img` i `svg` siedzą we wspólnym `.plate`, a nie + w `.shot`: viewBox musi zostać rozmiarem zrzutu (600×700), a warstwa SVG musi + leżeć na obrazku, nie na całym płótnie. Gdyby SVG został przypięty do `.shot`, + każda strzałka i gwiazdka przesunęłaby się o szerokość marginesu. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/popup.png"), + text="t", + heading=None, + annotations=[Annotation(kind="frame", x=10.0, y=20.0, w=300.0, h=40.0)], + screenshot_size=(600, 700), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'viewBox="0 0 600 700"' in html + assert ".plate svg {" in html + assert ".shot svg {" not in html + + def test_text_page_has_no_svg(): pages = [ GuidePage(kind="text", screenshot=None, text="tylko tekst", heading=None, annotations=[]) diff --git a/tests/unit/guide/test_prolog.py b/tests/unit/guide/test_prolog.py index 472a1fc..18022b8 100644 --- a/tests/unit/guide/test_prolog.py +++ b/tests/unit/guide/test_prolog.py @@ -1,6 +1,11 @@ import pytest -from guidebot_recorder.guide.prolog import GuideError, classify, scan_for_blockers +from guidebot_recorder.guide.prolog import ( + NARRATION_ONLY_KINDS, + GuideError, + classify, + scan_for_blockers, +) from guidebot_recorder.models.action import CachedAction, Fingerprint, PendingAction from guidebot_recorder.models.config import Config, TtsConfig, Viewport from guidebot_recorder.models.scenario import FlatStep, Scenario, Step, WhenBlock @@ -79,10 +84,29 @@ def test_scan_allows_a_visual_only_command(): scan_for_blockers(scen.flat_steps(), [None]) # no raise -def test_scan_raises_on_popup(): +def test_scan_allows_a_click_that_opens_a_popup(): + """Popup nie jest już blokadą — `guide` przełącza się na nowe okno. + + Ten przypadek odwraca dawne `test_scan_raises_on_popup`. Zamrożona flaga + `opens_popup` przestała być powodem odmowy, a stała się instrukcją: krok + otwiera okno, a kolejne kroki czyta się względem niego. + """ + scen = Scenario(config=_cfg(), steps=[Step(click="opens something")]) - with pytest.raises(GuideError, match="popup"): - scan_for_blockers(scen.flat_steps(), [_cached(opens_popup=True)]) + scan_for_blockers(scen.flat_steps(), [_cached(opens_popup=True)]) # no raise + + +def test_close_window_is_its_own_kind_not_narration(): + """`closeWindow` wykonuje pracę w przeglądarce, więc ma własny rodzaj strony. + + Dopóki popup był odrzucany, `closeWindow` nie miał czego zamykać i siedział + w `NARRATION_ONLY_KINDS`. Gdyby tam został, przełączenie z powrotem na okno + główne nigdy by nie nastąpiło, a dalsze kroki fotografowałyby zamknięty popup. + """ + + assert classify_step_of(Step(close_window=True)) == "closeWindow" + assert classify_step_of(Step(close_window=True, say="wracamy")) == "closeWindow" + assert "closeWindow" not in NARRATION_ONLY_KINDS def test_scan_raises_on_mandatory_pending(): From 8068fb8d74ca8cbac075fad83dbd77170ee3b2a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 03:28:24 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(guide):=20pauzuj=20na=20oknie,=20w=20kt?= =?UTF-8?q?=C3=B3rym=20krok=20pad=C5=82?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--pause-on-error` dostawał parametr `page` funkcji `capture_pages`, czyli zawsze okno główne. Dopóki było jedno okno, była to jedyna możliwa odpowiedź. Odkąd `_Capture.page` znaczy „okno aktywne", błąd w popupie zostawiałby dewelopera przed oknem głównym, na którym nic złego nie widać. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PEZwp8YivARFrZSjrSUnD5 --- guidebot_recorder/guide/capture.py | 5 +++- tests/unit/guide/test_capture_popup.py | 41 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/guidebot_recorder/guide/capture.py b/guidebot_recorder/guide/capture.py index f470c6b..e267256 100644 --- a/guidebot_recorder/guide/capture.py +++ b/guidebot_recorder/guide/capture.py @@ -254,7 +254,10 @@ async def capture_pages( except Exception as exc: if pause_on_error: await pause_for_inspection( - page, + # The window that failed, not the one the run started in. + # A step inside a popup that leaves the main window paused + # shows the developer a page with nothing wrong on it. + cap.page, "guide", index, run.kind, diff --git a/tests/unit/guide/test_capture_popup.py b/tests/unit/guide/test_capture_popup.py index ff21e8e..0f9fdc3 100644 --- a/tests/unit/guide/test_capture_popup.py +++ b/tests/unit/guide/test_capture_popup.py @@ -268,6 +268,47 @@ async def test_entering_a_popup_without_a_recorder_factory_blames_the_caller(tmp ) +async def test_pause_on_error_stops_on_the_window_that_failed(tmp_path, monkeypatch): + """`--pause-on-error` zostawia otwarte okno, w którym krok padł. + + Zanim `_Capture.page` stało się „oknem aktywnym", była tylko jedna możliwa + odpowiedź i pauza dostawała parametr `page` funkcji. Teraz błąd w popupie + zostawiłby dewelopera przed oknem głównym, na którym nic złego nie widać. + """ + + paused: list[object] = [] + + async def _pause(page, *_args, **_kwargs): + paused.append(page) + + monkeypatch.setattr(capture, "pause_for_inspection", _pause) + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + + class ExplodingRecorder(FakeRecorder): + async def point(self, target, ripple=False): + raise RuntimeError("krok padł w popupie") + + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + with pytest.raises(RuntimeError, match="padł w popupie"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + pause_on_error=True, + make_recorder=lambda page: ExplodingRecorder(events), + ) + + assert paused == [popup] + + async def test_close_window_without_a_popup_is_a_no_op(tmp_path): """`closeWindow` w scenariuszu bez popupu nie ma czego zamykać. From 3f5d52cd60942cd97198ee3d8eeb1213d06b9f9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 07:46:10 +0200 Subject: [PATCH 4/4] =?UTF-8?q?feat(guide):=20zwi=C5=84=20narracj=C4=99=20?= =?UTF-8?q?bez=20w=C5=82asnego=20kadru=20na=20poprzedni=C4=85=20stron?= =?UTF-8?q?=C4=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Krok `say:` między akcjami opisuje to, na co czytelnik patrzy — a dostawał osobną kartkę z jednym zdaniem na środku. Teraz ląduje w panelu bocznym poprzedniej strony ze zrzutem, pod poziomą kreską. Zwijanie nie dotyczy slajdów (celowa plansza pełnoekranowa nie absorbuje ani nie jest absorbowana) ani narracji, przed którą nie ma żadnego obrazka. Pusta narracja zwija się w nic zamiast w samotną kreskę — przy okazji znikają puste kartki po krokach narracyjnych bez tekstu. `fold_narration` jest publiczne i wołane przez `run_guide`, nie przez `render_html`. Zwijanie ukryte w rendererze zostawiało wywołującemu listę sprzed zwinięcia jako jedyną miarę dokumentu — a `guide` zwraca jej długość jako liczbę stron, więc CLI ogłaszało osiem stron pięciostronicowego PDF-a. Jedna lista, liczona i drukowana, nie może się rozjechać. Na scenariuszu logowania do Onetu: 8 stron -> 5, bez utraty jednego słowa narracji. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PEZwp8YivARFrZSjrSUnD5 --- guidebot_recorder/guide/guide.py | 10 +++- guidebot_recorder/guide/layout.py | 82 +++++++++++++++++++++++++-- guidebot_recorder/guide/model.py | 7 +++ tests/unit/guide/test_layout.py | 92 ++++++++++++++++++++++++++++++- 4 files changed, 182 insertions(+), 9 deletions(-) diff --git a/guidebot_recorder/guide/guide.py b/guidebot_recorder/guide/guide.py index a5b1e91..79af455 100644 --- a/guidebot_recorder/guide/guide.py +++ b/guidebot_recorder/guide/guide.py @@ -9,7 +9,7 @@ from guidebot_recorder.chrome import SHELL_URL, Chrome from guidebot_recorder.chrome.framing import install_framing from guidebot_recorder.guide.capture import capture_pages -from guidebot_recorder.guide.layout import render_html +from guidebot_recorder.guide.layout import fold_narration, render_html from guidebot_recorder.guide.pdf import html_to_pdf from guidebot_recorder.guide.prolog import GuideError, scan_for_blockers from guidebot_recorder.overlay.overlay import Overlay @@ -120,6 +120,10 @@ def make_popup_recorder(popup: Page) -> Recorder: finally: await context.close() - html = render_html(pages, title=cfg.title) + # Fold before rendering *and* before counting: narration with no still of its + # own rides on the previous picture, so it stops being a page. Counting the + # unfolded list here is what made `guide` report eight pages of a five-page PDF. + sheets = fold_narration(pages) + html = render_html(sheets, title=cfg.title) await html_to_pdf(browser, html, out_pdf) - return len(pages) + return len(sheets) diff --git a/guidebot_recorder/guide/layout.py b/guidebot_recorder/guide/layout.py index 463e8ad..f7fd575 100644 --- a/guidebot_recorder/guide/layout.py +++ b/guidebot_recorder/guide/layout.py @@ -4,6 +4,7 @@ import html import math +from dataclasses import replace from pathlib import Path from guidebot_recorder.guide.model import Annotation, GuidePage @@ -29,6 +30,9 @@ .side { padding-top: 2mm; } .side .heading { font-size: 20px; font-weight: 700; margin: 0 0 4mm; } .side .body { font-size: 16px; line-height: 1.5; white-space: pre-wrap; } +/* Separates this step's own description from narration folded in from steps + that had nothing to photograph — see `_fold_narration`. */ +.side hr.fold { border: 0; border-top: 1px solid #d6d6d6; margin: 4mm 0; } .slide { grid-column: 1 / -1; display: flex; flex-direction: column; justify-content: center; height: 100vh; text-align: center; } .slide .title { font-size: 40px; font-weight: 800; } @@ -124,17 +128,27 @@ def _letterbox(page: GuidePage) -> tuple[str, str]: ) +def _side(page: GuidePage) -> str: + """The right-hand column: this step's description, then anything folded in.""" + + heading = f'
{html.escape(page.heading)}
' if page.heading else "" + body = f'
{html.escape(page.text)}
' if page.text else "" + folded = "".join( + f'
{html.escape(text)}
' + for text in page.folded_text + ) + return f"{heading}{body}{folded}" + + def _shot_page(page: GuidePage) -> str: uri = Path(page.screenshot).absolute().as_uri() svg = _svg(page.annotations, page.screenshot_size or (1, 1)) - heading = f'
{html.escape(page.heading)}
' if page.heading else "" - body = f'
{html.escape(page.text)}
' if page.text else "" shot_style, plate_style = _letterbox(page) return ( '
' f'
' f'
{svg}
' - f'
{heading}{body}
' + f'
{_side(page)}
' "
" ) @@ -150,14 +164,72 @@ def _slide_page(page: GuidePage) -> str: def _text_page(page: GuidePage) -> str: - heading = f'
{html.escape(page.heading)}
' if page.heading else "" + """Narration that found no picture to join — full width, still its own sheet. + + Reached only when :func:`_fold_narration` declined to fold it: nothing came + before it, or what came before was a slide. + """ + return ( '
' - f'{heading}
{html.escape(page.text)}
' + f"{_side(page)}" + ) + + +def _folds_into_previous(page: GuidePage, previous: GuidePage | None) -> bool: + """Whether ``page`` is narration that belongs on the picture before it. + + Three conditions, and each excludes a page that must keep its own sheet: a + narration page with nothing before it has no picture to join; a page with a + still of its own is the thing being described, not a description; and a slide + is a deliberate full-page interstitial, so it neither absorbs nor is absorbed. + """ + + return ( + previous is not None + and previous.screenshot is not None + and page.screenshot is None + and page.kind != "slide" ) +def fold_narration(pages: list[GuidePage]) -> list[GuidePage]: + """Move picture-less narration onto the page of the still it was spoken over. + + A `say:` between two actions describes what the reader is looking at — it did + not earn a sheet of paper with a sentence marooned on it. Folding happens here + and not in the capture pass on purpose: one step still produces one + :class:`GuidePage`, and pagination is the document's question alone. + + Empty narration folds to nothing rather than to a bare rule, which is also how + a text page that never had any text stops printing a blank sheet. + + **Public, and called by ``run_guide`` rather than by :func:`render_html`.** + Folding inside the renderer would leave the caller holding the *unfolded* list + as its only measure of the document — and `guide` reports that length as the + page count, so the CLI would cheerfully announce eight pages of a five-page + PDF. One list, counted and rendered, is the only shape that cannot drift. + """ + + folded: list[GuidePage] = [] + for page in pages: + previous = folded[-1] if folded else None + if not _folds_into_previous(page, previous): + folded.append(page) + continue + carried = [*previous.folded_text, page.text] if page.text else previous.folded_text + folded[-1] = replace(previous, folded_text=carried) + return folded + + def render_html(pages: list[GuidePage], *, title: str) -> str: + """Render exactly the pages given — one ``
`` each. + + Does **not** fold; run :func:`fold_narration` first if you want that. Keeping + the renderer free of pagination is what lets the caller count the same list it + prints. + """ + body_parts: list[str] = [] for page in pages: if page.kind == "slide": diff --git a/guidebot_recorder/guide/model.py b/guidebot_recorder/guide/model.py index c7270cc..4e59ce3 100644 --- a/guidebot_recorder/guide/model.py +++ b/guidebot_recorder/guide/model.py @@ -48,6 +48,12 @@ class GuidePage: A popup page is not flagged as one anywhere; the two sizes differing IS the flag. That is deliberate — a separate boolean could disagree with the sizes, and then the layout would have two sources of truth for one question. + + ``folded_text`` is narration from later steps that had nothing of their own to + photograph, and so rides along in this page's side panel under a rule. It is + filled by the layout, never by the capture pass: one step still produces one + :class:`GuidePage`, and how those pages are composed onto sheets of paper is + a question only the document asks. """ kind: Literal["step", "navigate", "slide", "text"] @@ -57,6 +63,7 @@ class GuidePage: annotations: list[Annotation] = field(default_factory=list) screenshot_size: tuple[int, int] | None = None canvas_size: tuple[int, int] | None = None + folded_text: list[str] = field(default_factory=list) def page_text(step: Step) -> str: diff --git a/tests/unit/guide/test_layout.py b/tests/unit/guide/test_layout.py index 96d39b7..25ba3c3 100644 --- a/tests/unit/guide/test_layout.py +++ b/tests/unit/guide/test_layout.py @@ -1,6 +1,6 @@ from pathlib import Path -from guidebot_recorder.guide.layout import render_html +from guidebot_recorder.guide.layout import fold_narration, render_html from guidebot_recorder.guide.model import Annotation, GuidePage @@ -162,6 +162,96 @@ def test_annotations_stay_in_the_stills_own_coordinate_system(): assert ".shot svg {" not in html +def _text(text): + return GuidePage(kind="text", screenshot=None, text=text, heading=None, annotations=[]) + + +def _slide(heading): + return GuidePage(kind="slide", screenshot=None, text="", heading=heading, annotations=[]) + + +def test_narration_after_a_picture_folds_onto_it_under_a_rule(): + """`say:` między akcjami opisuje to, na co czytelnik patrzy. + + Nie zasłużyło na osobną kartkę z jednym zdaniem na środku — ląduje w panelu + bocznym poprzedniego kadru, oddzielone poziomą kreską. + """ + + html = render_html( + fold_narration([_shot_page([]), _text("Okno otwiera się osobno.")]), title="x" + ) + assert html.count('class="page"') == 1 + assert '
' in html + assert "Okno otwiera się osobno." in html + + +def test_narration_with_no_picture_before_it_keeps_its_own_page(): + """Nie ma czego dosiąść — zostaje pełnowymiarowa strona tekstowa.""" + + html = render_html(fold_narration([_text("Zanim zaczniemy.")]), title="x") + assert html.count('class="page"') == 1 + assert '
' not in html + assert "Zanim zaczniemy." in html + + +def test_a_slide_neither_absorbs_narration_nor_is_absorbed(): + """Slajd to celowa plansza pełnoekranowa, więc nie zwija się z niczym. + + Slajd tytułowy z narracją zaraz za nim dawałby inaczej stronę tytułową + z doklejonym zdaniem — a to jest przerywnik, nie kadr do opisania. + """ + + html = render_html(fold_narration([_slide("Logowanie"), _text("Witaj.")]), title="x") + assert html.count('class="page"') == 2 + assert '
' not in html + + +def test_several_narrations_fold_onto_one_picture_each_under_its_own_rule(): + pages = [_shot_page([]), _text("Pierwsze zdanie."), _text("Drugie zdanie.")] + html = render_html(fold_narration(pages), title="x") + assert html.count('class="page"') == 1 + assert html.count('
') == 2 + + +def test_empty_narration_folds_to_nothing_rather_than_a_bare_rule(): + """Pusta narracja nie ma czego wnieść, a kreska bez tekstu to śmieć na stronie. + + To zarazem koniec pustych kartek: krok narracyjny bez tekstu drukował dotąd + stronę z niczym. + """ + + html = render_html(fold_narration([_shot_page([]), _text("")]), title="x") + assert html.count('class="page"') == 1 + assert '
' not in html + + +def test_fold_narration_is_what_the_document_is_counted_by(): + """Zwinięta lista to jedna i ta sama lista, którą się drukuje i liczy. + + `run_guide` zwraca długość listy jako liczbę stron, więc gdy zwijanie siedziało + wewnątrz `render_html`, CLI ogłaszało osiem stron pięciostronicowego PDF-a. + Ten test pilnuje, że zwijanie jest krokiem wywołującego, a nie efektem ubocznym + renderowania. + """ + + pages = [_slide("Tytuł"), _shot_page([]), _text("dopisek"), _text("i jeszcze")] + sheets = fold_narration(pages) + html = render_html(sheets, title="x") + + assert len(sheets) == 2 + assert html.count('class="page"') == len(sheets) + # A gdyby renderer nadal zwijał sam, podwójne zwinięcie byłoby niewykrywalne. + assert render_html(pages, title="x").count('class="page"') == 4 + + +def test_folding_does_not_mutate_the_caller_s_pages(): + """`render_html` nie może przepisywać wejścia — to lista wywołującego.""" + + shot, text = _shot_page([]), _text("dopisek") + fold_narration([shot, text]) + assert shot.folded_text == [] + + def test_text_page_has_no_svg(): pages = [ GuidePage(kind="text", screenshot=None, text="tylko tekst", heading=None, annotations=[])