fix(ui): PR #217 review fixes — derived legend offset, 960x540 goldens, non-overlap tests
The legend's reposition() now derives its Y from the header panel's MEASURED bottom plus a named 12px gap (new accessors on the viewer; deferred recompute so it reads settled layout, refreshed on header content change) — the old hardcoded 60.0 sat 17px inside the wrapped header's real 77px bottom, fusing the panels. Pixel-proven at the evidence center: bottom=77.0, legend top=89.0, gap=12.0 exact. All 13 goldens regenerated at the DOCUMENTED 960x540 (the 1280x720 rider was my own CLI flag, not a config; PIL-verified) with two-run byte stability. Two structural tests pin the header/legend relationship (T-1192 precedent), proven failing-first against the old constant (-17.0px reported), and the legacy test that pinned the literal 60.0 now asserts the derived relationship so it cannot re-enforce drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Before Width: | Height: | Size: 52 KiB After Width: | Height: | Size: 41 KiB |
|
Before Width: | Height: | Size: 153 KiB After Width: | Height: | Size: 80 KiB |
|
Before Width: | Height: | Size: 40 KiB After Width: | Height: | Size: 28 KiB |
|
Before Width: | Height: | Size: 46 KiB After Width: | Height: | Size: 32 KiB |
|
Before Width: | Height: | Size: 55 KiB After Width: | Height: | Size: 43 KiB |
|
Before Width: | Height: | Size: 218 KiB After Width: | Height: | Size: 110 KiB |
|
Before Width: | Height: | Size: 90 KiB After Width: | Height: | Size: 75 KiB |
|
Before Width: | Height: | Size: 487 KiB After Width: | Height: | Size: 243 KiB |
|
Before Width: | Height: | Size: 60 KiB After Width: | Height: | Size: 43 KiB |
|
Before Width: | Height: | Size: 78 KiB After Width: | Height: | Size: 63 KiB |
|
Before Width: | Height: | Size: 121 KiB After Width: | Height: | Size: 70 KiB |
|
Before Width: | Height: | Size: 1.3 MiB After Width: | Height: | Size: 656 KiB |
|
Before Width: | Height: | Size: 56 KiB After Width: | Height: | Size: 41 KiB |
@@ -73,13 +73,30 @@ func test_refresh_reflects_the_active_overlay_toggle() -> void:
|
||||
).is_greater(rows_before)
|
||||
|
||||
|
||||
func test_reposition_sets_a_fixed_panel_margin_position() -> void:
|
||||
## T-1197 PR #217 review (both reviewers, pixel-proven): this used to assert
|
||||
## a hardcoded Vector2(PANEL_MARGIN, 60.0) — the same "60.0" constant whose
|
||||
## drift out of sync with the header panel's REAL grown footprint (once
|
||||
## step_canvas_viewer.gd wrapped the header in its own ImplantPanel) caused
|
||||
## the two panels to fuse into one double-height box. The fix removed that
|
||||
## hardcoded Y from production code (step_canvas_legend.gd's reposition() now
|
||||
## derives it from StepCanvasViewer.get_header_panel_bottom_y() +
|
||||
## get_header_legend_gap_px()) — this test must assert the SAME derived
|
||||
## relationship, not a second copy of the old magic number, or it would go on
|
||||
## silently enforcing the exact drift-prone shape production code just
|
||||
## stopped doing (test_step_canvas_viewer.gd's
|
||||
## test_legend_panel_never_overlaps_the_header_panel_vertically/
|
||||
## test_legend_panel_leaves_a_real_gap_below_the_header_panel are the fuller
|
||||
## non-overlap/gap proof against the REAL viewer-owned legend; this one stays
|
||||
## a narrow smoke check on a standalone legend instance, matching this file's
|
||||
## own "smoke coverage" scope note at the top).
|
||||
func test_reposition_derives_y_from_the_headers_measured_bottom_plus_gap() -> void:
|
||||
var v: StepCanvasViewer = _make_viewer()
|
||||
add_child(v)
|
||||
var legend = LegendScript.new(v)
|
||||
auto_free(legend)
|
||||
legend.reposition()
|
||||
assert_that(legend.position).is_equal(Vector2(LegendScript.PANEL_MARGIN, 60.0))
|
||||
var expected_y: float = v.get_header_panel_bottom_y() + v.get_header_legend_gap_px()
|
||||
assert_that(legend.position).is_equal(Vector2(LegendScript.PANEL_MARGIN, expected_y))
|
||||
|
||||
|
||||
## T-1192 review fix: RESERVED_COLUMN_PX is now a direct read of
|
||||
|
||||
@@ -837,3 +837,81 @@ func test_get_current_canvas_summary_counts_match_a_fixture_canvas() -> void:
|
||||
var by_class: Dictionary = summary.get("course_count_by_class", {})
|
||||
assert_int(int(by_class.get(0, 0))).is_equal(2)
|
||||
assert_int(int(by_class.get(2, 0))).is_equal(1)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# T-1197 PR #217 review (Hoshe): header-panel-vs-legend-panel vertical
|
||||
# non-overlap — the exact regression this review round caught. Mirrors the
|
||||
# T-1192 precedent above (test_global_canvas_left_edge_never_overlaps_the_legend_column,
|
||||
# line ~674): a geometric non-overlap invariant against the REAL viewer
|
||||
# wiring, not a hand-computed expected pixel value that could silently drift
|
||||
# out of sync with the production layout the same way the old hardcoded
|
||||
# Vector2(PANEL_MARGIN, 60.0) drifted out of sync with the header's real
|
||||
# grown footprint.
|
||||
# =============================================================================
|
||||
|
||||
|
||||
## The screen header panel and the legend panel must never vertically
|
||||
## overlap: the legend's TOP edge (position.y) must be at or below the
|
||||
## header's BOTTOM edge (position.y + size.y). Before the T-1197 PR #217 fix,
|
||||
## step_canvas_legend.gd's reposition() hardcoded Y=60.0 — a constant tuned
|
||||
## for the OLD bare-ImplantHeader footprint — so once the header grew its own
|
||||
## ImplantPanel wrapper (border + content margins), the legend's fixed Y sat
|
||||
## INSIDE the header panel's new, taller footprint: the two fused into one
|
||||
## unbroken double-height box with zero terrain gap between them (pixel-
|
||||
## proven independently by both PR #217 reviewers). This test pins the
|
||||
## invariant directly against the real _screen_header_panel/_legend_panel
|
||||
## Controls the production layout builds, not a copy of the geometry math.
|
||||
func test_legend_panel_never_overlaps_the_header_panel_vertically() -> void:
|
||||
var v: StepCanvasViewer = _make_viewer()
|
||||
add_child(v)
|
||||
v.enter({"body_id": "GJ380c", "body_radius_km": 6238.4}, {})
|
||||
|
||||
# Both panels are manually positioned (implant_panel.gd's own doc: "not
|
||||
# itself inside a parent Container, so nothing else forces a re-measure"),
|
||||
# and reset_to_content_size()/reposition() are deferred — award one idle
|
||||
# frame so the REAL settled sizes are in place before asserting, exactly
|
||||
# like the deferred-resize idiom both panels already rely on in production
|
||||
# (see _build_screen_header()'s own call to reset_to_content_size(), and
|
||||
# _ready()'s own deferred reposition() call added alongside this test).
|
||||
await get_tree().process_frame
|
||||
await get_tree().process_frame
|
||||
|
||||
var header_top: float = v._screen_header_panel.position.y
|
||||
var header_bottom: float = header_top + v._screen_header_panel.size.y
|
||||
var legend_top: float = v._legend_panel.position.y
|
||||
|
||||
assert_float(legend_top).override_failure_message(
|
||||
(
|
||||
"the legend panel's top edge (y=%.1f) must be AT OR BELOW the header"
|
||||
+ " panel's measured bottom edge (y=%.1f) — a smaller value means the"
|
||||
+ " two panels overlap/fuse into one box, the exact PR #217 regression"
|
||||
)
|
||||
% [legend_top, header_bottom]
|
||||
).is_greater_equal(header_bottom - 0.01)
|
||||
|
||||
|
||||
## The gap must be a REAL, visible gap — not just "touching at exactly the
|
||||
## same pixel" (which would still satisfy >= but reads as fused on screen).
|
||||
## Pins the fixed HEADER_LEGEND_GAP_PX constant is actually being applied,
|
||||
## not merely that overlap happens to be avoided by coincidence of content
|
||||
## size on this particular test body.
|
||||
func test_legend_panel_leaves_a_real_gap_below_the_header_panel() -> void:
|
||||
var v: StepCanvasViewer = _make_viewer()
|
||||
add_child(v)
|
||||
v.enter({"body_id": "GJ380c", "body_radius_km": 6238.4}, {})
|
||||
await get_tree().process_frame
|
||||
await get_tree().process_frame
|
||||
|
||||
var header_bottom: float = v._screen_header_panel.position.y + v._screen_header_panel.size.y
|
||||
var legend_top: float = v._legend_panel.position.y
|
||||
var gap: float = legend_top - header_bottom
|
||||
|
||||
assert_float(gap).override_failure_message(
|
||||
(
|
||||
"expected a visible gap of at least %.1fpx between the header panel's"
|
||||
+ " bottom (y=%.1f) and the legend panel's top (y=%.1f), got %.1fpx —"
|
||||
+ " panels that merely touch still read as one fused box on screen"
|
||||
)
|
||||
% [v.get_header_legend_gap_px(), header_bottom, legend_top, gap]
|
||||
).is_greater_equal(v.get_header_legend_gap_px() - 0.01)
|
||||
|
||||
@@ -52,8 +52,21 @@ func _init(viewer_ref = null) -> void:
|
||||
visible = false
|
||||
|
||||
|
||||
## T-1197 PR #217 review (both reviewers, pixel-proven): this used to hardcode
|
||||
## Vector2(PANEL_MARGIN, 60.0) — a constant tuned for the bare-ImplantHeader
|
||||
## era, before T-1197 wrapped the header in its own ImplantPanel (border +
|
||||
## content margins grew its real footprint). The two panels fused into one
|
||||
## unbroken double-height box with zero terrain gap. Now DERIVED from the
|
||||
## header panel's own MEASURED height (StepCanvasViewer.get_header_panel_bottom_y(),
|
||||
## which reads `_screen_header_panel.size.y` after that panel's own
|
||||
## reset_to_content_size() has settled) plus a fixed gap constant — never a
|
||||
## second hardcoded Y that can drift out of sync with the header again.
|
||||
func reposition() -> void:
|
||||
position = Vector2(PANEL_MARGIN, 60.0)
|
||||
if _viewer == null:
|
||||
position = Vector2(PANEL_MARGIN, 60.0) # pre-viewer fallback, unreachable in practice (_init always takes a viewer)
|
||||
return
|
||||
var header_bottom_y: float = _viewer.get_header_panel_bottom_y()
|
||||
position = Vector2(PANEL_MARGIN, header_bottom_y + _viewer.get_header_legend_gap_px())
|
||||
|
||||
|
||||
func refresh() -> void:
|
||||
|
||||
@@ -194,6 +194,19 @@ func _ready() -> void:
|
||||
_build_overlay_bar()
|
||||
_build_legend_panel()
|
||||
|
||||
# T-1197 PR #217 review: the legend's FIRST reposition() (called
|
||||
# synchronously above, inside _build_legend_panel()'s refresh()) reads
|
||||
# _screen_header_panel.size.y before that panel's own deferred
|
||||
# reset_to_content_size() has fired (call_deferred runs at end-of-frame,
|
||||
# per implant_panel.gd's own doc) — so it can still see a pre-layout
|
||||
# height on the very first frame. Deferring ONE more reposition() call
|
||||
# after this _ready() returns guarantees it re-reads the header panel's
|
||||
# real settled height at least once, the same "defer past the transient
|
||||
# wrong value" idiom implant_panel.gd already uses for exactly this class
|
||||
# of problem.
|
||||
if _legend_panel:
|
||||
_legend_panel.reposition.call_deferred()
|
||||
|
||||
_disk_sweep_timer = Timer.new()
|
||||
_disk_sweep_timer.name = "DiskSweepTimer"
|
||||
_disk_sweep_timer.wait_time = DISK_SWEEP_INTERVAL_SEC
|
||||
@@ -714,11 +727,26 @@ func _apply_transform() -> void:
|
||||
## was the only outlier) gives the header the same opaque scrim as every
|
||||
## sibling chrome element, guaranteeing contrast regardless of what terrain
|
||||
## renders underneath.
|
||||
## T-1197 PR #217 review (both reviewers, pixel-proven): the ImplantPanel wrap
|
||||
## below grew the header's on-screen footprint (border + content_margin_* from
|
||||
## the theme's panel stylebox) beyond the bare ImplantHeader's old, smaller
|
||||
## size — but step_canvas_legend.gd's own reposition() still hardcoded its Y
|
||||
## at PANEL_MARGIN + 60.0, a constant tuned for the OLD bare-header height.
|
||||
## Result: the two panels fused into one unbroken double-height box, zero
|
||||
## terrain gap, in every capture and all 13 goldens. Fixed via
|
||||
## HEADER_PANEL_TOP_Y/HEADER_LEGEND_GAP_PX below + get_header_panel_bottom_y()
|
||||
## — the legend now DERIVES its Y from the header panel's own MEASURED height
|
||||
## (never a second hardcoded constant that would drift again the next time
|
||||
## either panel's content changes shape).
|
||||
const HEADER_PANEL_TOP_Y: float = 16.0
|
||||
const HEADER_LEGEND_GAP_PX: float = 12.0
|
||||
|
||||
|
||||
func _build_screen_header() -> void:
|
||||
var PanelScript := load("res://ui/implant/implant_panel.gd")
|
||||
_screen_header_panel = PanelScript.new()
|
||||
_screen_header_panel.name = "ScreenHeaderPanel"
|
||||
_screen_header_panel.position = Vector2(PANEL_MARGIN, 16.0)
|
||||
_screen_header_panel.position = Vector2(PANEL_MARGIN, HEADER_PANEL_TOP_Y)
|
||||
_screen_header_panel.mouse_filter = Control.MOUSE_FILTER_IGNORE
|
||||
_screen_header_panel.custom_minimum_size.x = 320.0
|
||||
_screen_header_panel.theme_resource = _implant_theme # set BEFORE add_child(), matching _build_legend_panel()'s own ordering
|
||||
@@ -729,6 +757,13 @@ func _build_screen_header() -> void:
|
||||
_screen_header.mouse_filter = Control.MOUSE_FILTER_IGNORE
|
||||
_screen_header_panel.add_component(_screen_header)
|
||||
|
||||
# Same "manually positioned, not inside a parent Container, so nothing else
|
||||
# forces a re-measure" situation implant_panel.gd's own
|
||||
# reset_to_content_size() doc describes for the legend — this panel needs
|
||||
# the identical deferred re-measure so get_header_panel_bottom_y() reads a
|
||||
# settled `size.y`, not a transient pre-layout value.
|
||||
_screen_header_panel.reset_to_content_size()
|
||||
|
||||
|
||||
func _refresh_screen_header() -> void:
|
||||
if _screen_header == null:
|
||||
@@ -738,6 +773,35 @@ func _refresh_screen_header() -> void:
|
||||
var title := "ATLAS — %s" % name_label.to_upper()
|
||||
var subtitle := "%s · %.3f km/gridunit" % [_held_rung.to_upper(), spacing_km]
|
||||
_screen_header.set_content(title, subtitle)
|
||||
# Title/subtitle text length never changes the panel's HEIGHT (autowrap is
|
||||
# word-smart but both lines are short, fixed-shape content — only the
|
||||
# legend's row COUNT genuinely varies), but re-measuring here is free
|
||||
# insurance against exactly the class of drift this review round caught,
|
||||
# and keeps the legend's derived Y correct even if header content ever
|
||||
# grows a line.
|
||||
_screen_header_panel.reset_to_content_size()
|
||||
if _legend_panel:
|
||||
_legend_panel.reposition()
|
||||
|
||||
|
||||
## T-1197 PR #217 review: the ONE place step_canvas_legend.gd's reposition()
|
||||
## reads the header panel's footprint from — never a second hardcoded
|
||||
## constant. Falls back to HEADER_PANEL_TOP_Y-only (as if the header panel had
|
||||
## zero height) before the header panel exists, mirroring
|
||||
## _centered_view_offset()'s own "nothing to measure yet" pre-arrival pattern.
|
||||
func get_header_panel_bottom_y() -> float:
|
||||
if _screen_header_panel == null:
|
||||
return HEADER_PANEL_TOP_Y
|
||||
return _screen_header_panel.position.y + _screen_header_panel.size.y
|
||||
|
||||
|
||||
## Plain-method accessor for HEADER_LEGEND_GAP_PX (not a bare const read) —
|
||||
## step_canvas_legend.gd holds `_viewer` UNTYPED (its own doc: "avoid cyclic
|
||||
## ref"), and this cluster's convention is a method call across that boundary,
|
||||
## matching get_header_panel_bottom_y()/get_held_rung()/is_overlay_visible()
|
||||
## rather than relying on GDScript's duck-typed const-through-Variant access.
|
||||
func get_header_legend_gap_px() -> float:
|
||||
return HEADER_LEGEND_GAP_PX
|
||||
|
||||
|
||||
func _build_overlay_bar() -> void:
|
||||
|
||||