fix(overlay): explainable alert splitting, scrolling long alerts, resizable regions - #103
Conversation
…gold kicker
`split_alert_text` partitioned an alert on the first separator in
`ALERT_SEPARATORS` with no constraint on what it took. Any of ` — `, ` -- `,
` - ` or `: ` occurring anywhere in the text split it, and the whole head
became the small tracked-out gold cap. The design is "Gorenaire — ENRAGED":
a short label naming WHO, then the big word saying WHAT. On a raid-mob info
dump it was nonsense — the reported
<Dozekar The Cursed> [Slowable, baneable] - [CH Unslowed: 2s, ...
lost its opening 41 characters to the kicker because the line happens to
contain " - " before "[CH Unslowed" (#102).
The rule is now narrow, stated once, and repeated in prose in
docs/windows/event-overlay.md so a user can predict it: the FIRST separator
in the TEXT (not the first in the tuple — "FTE: Someone - and more" has to
kick off "FTE"), and only when everything before it passes
`looks_like_a_kicker` — at most 28 characters, at most 4 words, and free of
[]{}()<>|/*. Anything else is one headline at one size, and a failed split
does NOT fall through to a later separator: that would go looking for a
split the author did not write.
`": "` earns its place on evidence rather than taste: two shipped built-ins
are exactly that shape (`Death Touch: {target}`, `Resisted: {spell}`), so
constraining the head is the fix, not dropping the separator.
Also: every label that renders event text is now PlainText. Qt's AutoText
heuristic turns a string opening with a known HTML tag name into rich text,
which would swallow "<b>" out of a trigger's display text (and read "<"
as an escape) — the same hazard settingswindow.py already guards against.
Measured: this was NOT what mangled the reported line (Qt 6 looks the tag
name up, and "dozekar" is not an element), but it is live for any trigger
that names a real one.
…s resizable Two halves of one complaint (#102): a long trigger "Display text" was cut off at the bottom of its region, and the region it was cut off in could be moved in position mode but never resized. *Shrink, then scroll.* `fit_text_size` stepped the headline down to MIN_ALERT_TEXT_SIZE and then the text simply clipped — the search bottomed out and the rest of the sentence was gone. `_AlertViewport` is the fix: the label keeps its whole wrapped height inside a fixed-height parent, and Qt clips a child to its parent's rect, so the crawl is a `move()`. Deliberately not a QGraphicsEffect — the label already carries the shadow/glow and a widget only gets one. `alert_scroll_speed` is the pure decision and honors the two requirements in the order they were asked for. Fast enough to finish: the overflow is walked inside the alert's OWN lifetime (`general.overlay_text_seconds`, re-derived live through `apply_timings`, never a hardcoded rate) less the dwells that park the first and last lines standing still. Slow enough to read: capped at 2.5 lines/second. When those disagree — an alert far longer than its display time — the readable rate wins and the fix is a longer duration or a taller region, not a crawl nobody can follow. Measured end-to-end in the real app: a 3-paragraph alert in a 460x110 Alerts region shrinks to the 12 px floor, then walks its 50 px of overflow at 17.9 px/s, arriving in 2.9 s of a 4.0 s alert. The invariant holds throughout: `current_text()` and the reset match still see the whole string, asserted after driving a crawl to completion. *Resizable regions.* Region editing grows from move-only to move AND resize, reusing `overlaybase.edge_at`/`cursor_for_edges` (the 1.8 margin-band hit test) rather than a second one — `region_resize_margin` only narrows the band so a 30 px lane strip keeps an interior to drag by. The drag math is a rect move plus `region_offsets`, the exact inverse of the placement rule now extracted as `region_origin`, so every anchor comes out right without a case per edge per anchor. `OverlayRegion.height` persists it. A region's height is a FLOOR for the content-driven regions (a sixth timer bar still grows the region rather than being swallowed) and EXACT for Alerts, because that region has a way to honor it: it is the same number `_alert_budget_height` fits the headline into. That is what ties the two halves together — the box you draw is the box the text is fitted to.
prokopto-dev
left a comment
There was a problem hiding this comment.
Requesting changes: alert-region resizing can leave the displayed text laid out for the old box, and the configured minimum region height is not honored by the headline budget.
…the one it left Two defects from review of #102, both in the seam between the region a user drags and the budget the headline is fitted into. *The drag measured the old geometry.* `_apply_region_resize` ended by calling `_restyle_alert()`, but its caller ran `_layout_regions()` afterwards — so every measurement during an edge drag read the host width the region was about to leave. Shrinking clipped wrapped text and growing kept a crawl for overflow that no longer existed. The refit now happens after the hosts have their new geometry. The regression test is a fixed-point property: a second `_restyle_alert()` after the drag must change nothing, which is exactly what "stale" would break. *The budget could exceed its own region.* `MIN_ALERT_BUDGET` (40) floored the configured-region path while a region could be dragged to `MIN_REGION_HEIGHT` (32), so a short Alerts region got a headline budget taller than the region itself — the clipping this path exists to stop, in a region documented as exact. The configured budget is now exact and unfloored, and the floor moved to where it belongs: `_min_region_height` gives Alerts its own resize minimum, one line of the smallest headline plus its chrome. The underlying cause of both was one number with two expressions, so both are now single definitions with the callers that must agree named in the docstring: `_alert_region_height` (what `_region_size` lays the host out at AND what the budget subtracts from) and `_alert_chrome_height` (what the budget subtracts AND what the resize floor adds back). `MIN_ALERT_BUDGET` keeps only its original job, the default window-fraction path. A hand-edited settings.json can still name a height below the drag floor, so the budget is asserted to stay inside the host for that too — the drag floor is UX, not the invariant.
prokopto-dev
left a comment
There was a problem hiding this comment.
Requesting changes: the requested head commit still has two alert-region layout defects.
|
Both confirmed and fixed in b278000. Thank you — the second one was a real hole in the "exact" claim I documented.
Regression test is a fixed-point property, since "stale" is exactly what a second pass would reveal: settled = _alert_layout_state(overlay)
overlay._restyle_alert()
assert _alert_layout_state(overlay) == settledParametrized over shrink, grow and a left-edge grab. Verified it fails on the pre-fix ordering (3/3) and passes after. 40 px budget vs a 32 px region — also correct, and it made the region not exact in the one case the docs promise it is. The configured path is now exact and unfloored: return max(1, self._alert_region_height(region) - self._alert_chrome_height())and the floor moved to where it can actually be honored: The cause under both was one number with two expressions, so both are now single definitions naming the callers that must agree — One thing your finding surfaced that the fix alone doesn't cover: a hand-edited Full suite green (2474 passed), ruff clean, and re-verified end-to-end in the real app: the 3-paragraph alert in a 460×110 region still shrinks to the 12 px floor and walks its 50 px of overflow at 17.9 px/s, arriving in 2.9 s of a 4.0 s alert. Docs updated to state the Alerts minimum. |
prokopto-dev
left a comment
There was a problem hiding this comment.
Approved: the latest commit addresses the previously reported stale live-alert measurement during region resizing and keeps the configured alert budget within the Alerts host, including hand-edited small heights.
CI caught the worst-case chrome still not being worst case, in the one place a Mac cannot see it. A QLabel's height depends on whether it holds any text at all — not on which glyphs — and by how much is platform-specific: the populated kicker measures one pixel TALLER than the empty one on Linux and two pixels SHORTER on Windows, while macOS reports the same number either way. The floor is computed in position mode, where the kicker is empty, so on Linux it under-charged and the "guaranteed" budget came out at 39 against a MIN_ALERT_BUDGET of 40 — this commit's own defect one pixel wide instead of eleven. _measure_kicker_height asks the widget both ways and keeps the larger, through the real label rather than a QFontMetrics of our own (the size comes from a stylesheet, which a QLabel's font() does not carry — the trap _measure_headline documents). Called from apply_skin and cached: the skin and the base font are what move it, and the alternative is mutating a widget from inside _min_region_height, which a region drag calls on every mouse move. Also drops an over-assertion in the new scroll test. It required a paragraph to finish crawling inside its four-second lifetime, which #103 explicitly does not promise — alert_scroll_speed is capped at a readable 2.5 lines/second and says so, and the remedy for a longer alert is a longer duration or a taller region. Whether THIS paragraph fits is a question about a platform's font metrics; it failed on Windows for exactly the documented reason. The widget test now asserts the crawl walks its whole travel and stops; the deadline stays pinned on the pure function, where font metrics are not involved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(overlay): stop laying the alert viewport out zero pixels wide Closes #107. A trigger fired, the user heard the TTS, and nothing appeared on screen — on every alert, on every path, legacy stacked and region mode alike. Regression from #103. Cause 1: the viewport had no width. #103 swapped the headline's label out of the alert layout for the new clipping viewport, and apply_skin's loop went on setting AlignHCenter on that layout item. Qt clamps an aligned item to its size hint width instead of stretching it to the cell, and a bare QWidget with no layout answers QSize(-1, -1) — width 0. Pre-#103 the loop named a QLabel, which has a real hint, so the swap broke it silently. Measured at 1200x800: viewport QRect(580, 15, 0, 98), center_text QRect(0, 30, 1, 38). The kicker is still a real QLabel, so "Name — WORD" showed the tiny gold cap alone and unsplit text — most triggers — showed nothing at all. The flag comes off that one item; the kicker and the rule keep it, and the headline stays centered by the label's own alignment inside a viewport that now spans the host. The alert panel is still centered in its region under every skin. Two things that made it silent also go: _place_label's max(1, self.width()) — a one-pixel floor is what let every height-and-text assertion in the suite pass at width 0 — and the missing hint, since _AlertViewport now reports a real width and an Expanding horizontal policy, so re-aligning that item could never collapse it again. Cause 2: the minimum alert budget was not guaranteed. Layout keeps honoring the drawn box exactly — clamping the budget up made a short region's headline taller than its region, which is the clipping that path exists to stop — so the floor lands on the region instead: - _min_region_height now charges WORST-CASE chrome. It is evaluated in position mode, where no alert is live and the kicker is hidden, so it floored a region at 47 (chrome 7) that the next "Gorenaire — ENRAGED" overran: chrome 18, budget 29, against MIN_ALERT_BUDGET 40. - _Hairline gained a sizeHint. Its QSize(-1, -1) was being subtracted from the budget, crediting the headline two pixels the region does not have — the same invariant, two pixels' worth. - A loaded Alerts region below that floor is clamped up, in apply_skin because the floor is font-relative. That also covers raising the base font live, which could put a legally drawn region back under it. - OverlayRegion.width/height get ge= bounds plus a before-validator that clamps a too-small size up and drops a non-numeric one to None. It never raises: load_settings reads a ValueError as a corrupt document and would discard every other setting over one bad pixel count (the PluginsSettings precedent). Those two floors live in config.settings and ui/eventoverlay.py imports them, so the drag floor and the load-time bound cannot drift; config stays Qt-free. Tests: the 597 lines #103 added asserted height and text, never width or on-screen extent, so all of them held at width 0. The new ones assert visible extent — including a grab()-based check that firing an alert produces non-background pixels, in both layout modes and at the smallest region the UI now permits. test_alert_content_is_centered_in_every_skin required the alignment flag that caused this; it now asserts the geometry that flag was supposed to produce. 19 of the new assertions fail against the code this replaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(overlay): charge the alert floor for a kicker that has text CI caught the worst-case chrome still not being worst case, in the one place a Mac cannot see it. A QLabel's height depends on whether it holds any text at all — not on which glyphs — and by how much is platform-specific: the populated kicker measures one pixel TALLER than the empty one on Linux and two pixels SHORTER on Windows, while macOS reports the same number either way. The floor is computed in position mode, where the kicker is empty, so on Linux it under-charged and the "guaranteed" budget came out at 39 against a MIN_ALERT_BUDGET of 40 — this commit's own defect one pixel wide instead of eleven. _measure_kicker_height asks the widget both ways and keeps the larger, through the real label rather than a QFontMetrics of our own (the size comes from a stylesheet, which a QLabel's font() does not carry — the trap _measure_headline documents). Called from apply_skin and cached: the skin and the base font are what move it, and the alternative is mutating a widget from inside _min_region_height, which a region drag calls on every mouse move. Also drops an over-assertion in the new scroll test. It required a paragraph to finish crawling inside its four-second lifetime, which #103 explicitly does not promise — alert_scroll_speed is capped at a readable 2.5 lines/second and says so, and the remedy for a longer alert is a longer duration or a taller region. Whether THIS paragraph fits is a question about a platform's font metrics; it failed on Windows for exactly the documented reason. The widget test now asserts the crawl walks its whole travel and stops; the deadline stays pinned on the pure function, where font metrics are not involved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes #102.
Three complaints from one report, all in
ui/eventoverlay.py.1. The "smaller gold text" — confirmed and fixed
split_alert_textpartitioned an alert on the first separator inALERT_SEPARATORS = (" — ", " -- ", " - ", ": ")with no constraint on whatit took. The reporter's line contains
" - "before"[CH Unslowed", soits opening 41 characters —
<Dozekar The Cursed> [Slowable, baneable]—became the small tracked-out gold kicker. That is the accidental formatting,
and the second screenshot shows it exactly:
<DERAKOR THE VINDICATOR> [UNSLOWABLE, RAMPAGES, BANEABLE]in gold caps above[CH: 4s] // Melee fight.The rule is now narrow and stated once:
Anything failing any part of that is one headline at one size. A failed split
does not fall through to a later separator — that would go looking for a
split the author never wrote.
": "earns its place on evidence rather than taste: two shipped built-insare exactly that shape (
Death Touch: {target},Resisted: {spell}), so thefix constrains the head rather than dropping the separator. Both design cases
(
Gorenaire — ENRAGED,FTE: Someone) still split; the reporter's line doesnot. Both directions are tested.
The rule is documented for users in
docs/windows/event-overlay.md→ "The small gold label", linked from theDisplay text bullet in
docs/features/triggers.md.2. Clipping → shrink, then scroll
fit_text_sizestepped down toMIN_ALERT_TEXT_SIZEand then the text simplyclipped.
_AlertViewportholds the label at its full wrapped height inside afixed-height parent — Qt clips a child to its parent's rect, so the crawl is a
move(). No second QGraphicsEffect: the label keeps the shadow/glow italready carries.
alert_scroll_speedis the pure decision, bounded on both sides:lifetime (
general.overlay_text_seconds, re-derived live throughapply_timings, never hardcoded), less the dwells that park the first andlast lines standing still;
When those disagree (an alert far longer than its display time) the readable
rate wins and the remedy is a longer duration or a taller region, not a crawl
nobody can follow. Documented as such.
The invariant holds:
current_text()and the reset match still see thewhole string, asserted after driving a crawl to completion.
3. Resizable Alerts region
Region editing goes from move-only to move and resize, reusing
overlaybase.edge_at/cursor_for_edges(the 1.8 margin-band hit test)rather than a second one;
region_resize_marginonly narrows the band so a30 px lane strip keeps an interior to drag by. The drag math is a rect move
plus
region_offsets— the exact inverse of the placement rule, now extractedas
region_origin— so every anchor is right without a case per edge peranchor.
OverlayRegion.heightpersists it.A region's height is a floor for the content-driven regions (a sixth timer
bar still grows the region) and exact for Alerts, because that region can
honor it: it is the same number
_alert_budget_heightfits the headline into.That is what ties parts 1 and 2 together — the box you draw is the box the
text is fitted to.
Also: rich-text auto-detection
Investigated as suspected. Measured with
Qt.mightBeRichText: it was notmangling the reported line — Qt 6 looks the tag name up against known HTML
elements and
dozekaris not one, so<Dozekar The Cursed>survived (thescreenshot confirms it rendered). But it is live for any trigger naming a
real tag (
<b>,<p>,<i>) or using an entity. Every label that rendersevent text is now
PlainText: alert kicker and headline, timer-bar name andvalue, utility lines, CH lane names. Trigger text is user data, never markup.
Verification
Full suite green (2466 passed) plus
ruff check/ruff format --check.Driven end-to-end through the real app offscreen (project
verifyskill):real triggers, real log lines, real overlay.
Gorenaire — ENRAGEDGorenaireENRAGEDcurrent_text()returned the whole string in every case, including afterdriving the crawl to completion.
Notes for review
ui/settingswindow.pyuntouched (no new setting needed — the region isedited in position mode and the split rule is documented).
assertion now reads the viewport, which is the headline's layout item.
v2.7.0.🤖 Generated with Claude Code