From d2f70675b899f367b441e66d320b6922dbca7f30 Mon Sep 17 00:00:00 2001 From: Dominik Roth Date: Thu, 13 Aug 2026 19:28:01 +0200 Subject: [PATCH] Auto-ids per type not per group; show detected_id in proposal UI Auto-assignment (Board.add_target/add_ally with no explicit id_) now scopes its 1/2/3... sequence per TYPE within each group, not one sequence shared across every type in the group -- Tank#1/Infantry#1 rather than Tank#A/Infantry#B, matching the game's own numbering. Reverses the type-scoping half of an earlier fix in this file (see TODO.md's "Allies and enemies seem to share indices" entry) per explicit user direction; the targets-vs-allies namespace split that fix also made is untouched, still correct. _next_free_id (letters, rolling over to "AA" past 26) is replaced by _next_free_numeric_id -- a plain counter can't run out the way a fixed alphabet could, so there's no equivalent rollover concern. test_models.py updated to match (one test asserts the opposite of before, renamed accordingly). detected_id (map_vision.read_marker_id) was being logged but never shown anywhere a human could actually check it against the screenshot before now: added to the proposal popover's heading and the pending-proposal's own on-map label. Co-Authored-By: Claude Sonnet 5 --- TODO.md | 30 ++++++++++++++ src/fenigma/app.py | 10 ++++- src/fenigma/grid_widget.py | 8 +++- src/fenigma/models.py | 67 ++++++++++++++++--------------- tests/test_app_accept_proposal.py | 10 +++-- tests/test_models.py | 48 +++++++++++----------- 6 files changed, 111 insertions(+), 62 deletions(-) diff --git a/TODO.md b/TODO.md index b789aac..b6a9354 100644 --- a/TODO.md +++ b/TODO.md @@ -198,3 +198,33 @@ Status legend: [x] fixed+tested, [~] partially addressed, [ ] open/needs input code exists in this repo at all yet, real scope work (find/access the game's log, agree a "# Destroyed" grammar, wire it into a dedup key) rather than a quick pass. + + Follow-ups from user feedback after the above landed: + - `_accept_proposal` now actually USES `detected_id` (it was only + being logged before, never applied) -- an accepted proposal's + entity id prefers the detected number over auto-assignment, + falling back on a collision. 4 new regression tests + (`tests/test_app_accept_proposal.py`). + - Auto-assignment itself (`Board.add_target`/`add_ally` with no + `id_`/no usable detection) changed from one shared letter + sequence per group (targets, or allies) to its own 1/2/3... + sequence per TYPE within each group -- Tank#1/Infantry#1 rather + than Tank#A/Infantry#B, matching the game's own numbering (and + what `detected_id` looks like when it IS read). This directly + reverses an earlier deliberate fix in this same file (see the + "Allies and enemies seem to share indices" entry above, which + moved FROM per-type TO shared-per-group) -- that fix is still + correct for what it fixed (targets-vs-allies must stay separate + namespaces), just not for per-type-vs-shared, which the user has + now clarified the other way. `_next_free_id` (the letter + sequence, with its own StopIteration-safe rollover to "AA" past + 26) is gone, replaced by `_next_free_numeric_id` -- a plain + counter can't run out the way a fixed alphabet could, so there's + no equivalent rollover concern to carry forward. Tests in + `test_models.py` updated to match (renamed + `test_auto_id_is_shared_across_types...` -> + `test_auto_id_is_per_type...`, since it now asserts the opposite). + - `detected_id` is now shown, not just logged: the proposal + popover's heading (", id #8") and the pending-proposal's own + on-map label (`? #8 G8 5:4`) both show it while there's still a + screenshot up to check it against by eye. diff --git a/src/fenigma/app.py b/src/fenigma/app.py index 3ff067c..ea2d83f 100644 --- a/src/fenigma/app.py +++ b/src/fenigma/app.py @@ -850,9 +850,17 @@ class MainWindow(Adw.ApplicationWindow): box = page() lbl = Gtk.Label(xalign=0, margin_start=4, margin_bottom=2) side = "friendly" if proposal.side == "friendly" else "hostile" + # detected_id (map_vision.read_marker_id's best-effort read of + # the marker's own "#" label, see its own docstring) is + # shown here so it's visible right when there's still a + # screenshot to actually check it against -- accept already + # uses it for the entity's id when present (see + # _accept_proposal), this is just making that fact visible + # before the click, not a separate signal. + id_part = f", id #{proposal.detected_id}" if proposal.detected_id else "" lbl.set_markup( f"{GLib.markup_escape_text(proposal.coord)} — {side}, " - f"{detected.value if detected else 'type unknown'}") + f"{detected.value if detected else 'type unknown'}{GLib.markup_escape_text(id_part)}") box.append(lbl) box.append(Gtk.Separator(margin_top=2, margin_bottom=2)) button(box, f"Accept as {detected.value if detected else TargetType.UNKNOWN.value}", diff --git a/src/fenigma/grid_widget.py b/src/fenigma/grid_widget.py index 005a4ed..8e56187 100644 --- a/src/fenigma/grid_widget.py +++ b/src/fenigma/grid_widget.py @@ -638,8 +638,14 @@ class GridCanvas(Gtk.DrawingArea): what a proposal is until the user accepts it.""" for p, coord in self._pending_proposals(): color = CATEGORY_COLOR["ally" if p.side == "friendly" else "target"] + # detected_id (map_vision.read_marker_id's best-effort read of + # the marker's own "#" label) shown right on the map while + # the screenshot backing it is still up, so it's checkable + # against the actual pixels -- same id _accept_proposal will + # use for the entity if this gets accepted, see its own comment. + id_part = f" #{p.detected_id}" if p.detected_id else "" self._draw_marker(cr, view, coord.as_fraction(), color, - f"? {coord.label()}", width, height, + f"?{id_part} {coord.label()}", width, height, hollow=True, coord=coord) def _hit_test(self, view: _View, x: float, y: float): diff --git a/src/fenigma/models.py b/src/fenigma/models.py index 2a9b26f..db3c36e 100644 --- a/src/fenigma/models.py +++ b/src/fenigma/models.py @@ -18,7 +18,6 @@ Coord) to work out everything else. This module just defines the shape. from __future__ import annotations -import itertools import string from dataclasses import dataclass, field from enum import Enum @@ -503,27 +502,28 @@ class ScoutFlight: return f"ScoutFlight#{self.id}" -def _next_free_id(used: set[str]) -> str: - """Next unused id in a short, human-friendly sequence: single - uppercase letters (A..Z) first, then two-letter combinations - (AA..ZZ, spreadsheet-column style) once those run out, and so on. +def _next_free_numeric_id(used: set[str]) -> str: + """Next unused id in a plain 1, 2, 3... sequence -- matches the small + integer ids the game itself shows per unit (see + map_vision.read_marker_id and app.py's _accept_proposal, which + prefers that detected id over this auto-assignment whenever it has + one), scoped per TYPE, not per group: add_target()/add_ally() only + look at existing entities of the SAME type when building `used`, so + a Tank and an Infantry added back to back both start at '1', each + type keeping its own independent count (Infantry #1/#2/#3, + Mechanized #1/#2/#3, ..., same as the game's own numbering) rather + than sharing one sequence across every type in the group. - A real regression lived here: `next(c for c in string.ascii_uppercase - if c not in used)` raises StopIteration the instant all 26 letters - are taken, which used to need 26+ auto-added entities of one TYPE - (rare) but, once add_target()/add_ally() moved to one shared id - sequence per GROUP instead of per type (so a Tank and an Infantry - added back to back get 'A'/'B', not both 'A', see their own - comments), needs only 26 auto-added entities of ANY type in that - group -- reachable in a single big screenshot import. This can't run - out: it just grows the id length instead.""" - length = 1 - while True: - for combo in itertools.product(string.ascii_uppercase, repeat=length): - candidate = "".join(combo) - if candidate not in used: - return candidate - length += 1 + Older versions of this used a letter sequence (A, B, C..., rolling + over to AA/AB/... past 26) shared across a whole group instead of + per type -- switched away from per-type once before (see TODO.md) + because sharing made it too easy to run past 26 letters. That's not + a concern here: a plain counter never runs out, there's no fixed + alphabet to exhaust regardless of how it's scoped.""" + n = 1 + while str(n) in used: + n += 1 + return str(n) SAVE_FORMAT_VERSION = 3 @@ -596,14 +596,15 @@ class Board: location: Location | Coord | None = None, id_: str | None = None, ) -> Target: - # One shared A/B/C... sequence across every target regardless of - # type, not one sequence per type -- a Tank and an Infantry auto- - # assigned back to back get 'A' and 'B', never both 'A'. Only - # targets-vs-allies is a separate id namespace (see add_ally), - # type never subdivides it further. + # Own 1/2/3... sequence per TYPE, not one shared across every + # target regardless of type -- a Tank and an Infantry auto- + # assigned back to back both start at '1' (Tank#1, Infantry#1), + # matching how the game itself numbers units. targets-vs-allies + # is still its own separate id namespace (see add_ally); type + # now subdivides it further too. See _next_free_numeric_id. if not id_: - used = {t.id for t in self.targets} - id_ = _next_free_id(used) + used = {t.id for t in self.targets if t.type == type_} + id_ = _next_free_numeric_id(used) t = Target(type=type_, id=id_, location=_as_location(location)) self.targets.append(t) return t @@ -620,12 +621,12 @@ class Board: ) -> Ally: # A separate id namespace from add_target()'s: an ally Tank#1 # and a hostile Target Tank#1 are unrelated, so auto-assignment - # here only looks at other allies, never self.targets. Same as - # add_target though, that's the ONLY split: one shared A/B/C... - # sequence across every ally regardless of type, not one per type. + # here only looks at other allies, never self.targets. Own + # 1/2/3... sequence per TYPE too, same as add_target -- see its + # own comment and _next_free_numeric_id. if not id_: - used = {a.id for a in self.allies} - id_ = _next_free_id(used) + used = {a.id for a in self.allies if a.type == type_} + id_ = _next_free_numeric_id(used) a = Ally(type=type_, id=id_, location=_as_location(location)) self.allies.append(a) return a diff --git a/tests/test_app_accept_proposal.py b/tests/test_app_accept_proposal.py index c2777a9..a810154 100644 --- a/tests/test_app_accept_proposal.py +++ b/tests/test_app_accept_proposal.py @@ -1,9 +1,11 @@ """_accept_proposal: an accepted proposal's entity id should prefer the marker's own detected "#" id (map_vision.read_marker_id, via -Proposal.detected_id) over an auto-assigned letter, so ids on the board +Proposal.detected_id) over an auto-assigned number, so ids on the board match what's actually on screen -- falling back to auto-assign only when -there's no detection, or it collides with an id already used in that -group (see _accept_proposal's own docstring). +there's no detection, or it collides with an id already used for that +type in that group (see _accept_proposal's own docstring, and +models.py's _next_free_numeric_id for the per-type auto-assignment +these fall back to). Needs a real Adw/Gtk init (MainWindow.__new__ skips __init__, so no window/widgets are actually built, but Adw.init() is still required for @@ -44,7 +46,7 @@ def test_accept_uses_the_detected_id_when_present(): def test_accept_falls_back_to_auto_id_with_no_detection(): win = _window() win._accept_proposal(_proposal(detected_id=None)) - assert win.board.targets[0].id == "A" + assert win.board.targets[0].id == "1" def test_accept_falls_back_to_auto_id_on_a_detected_id_collision(): diff --git a/tests/test_models.py b/tests/test_models.py index a5643c5..6f47979 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -54,56 +54,58 @@ def test_clear_units_keeps_recon_infrastructure(): def test_ally_and_target_ids_are_independent_namespaces(): """An ally Tank#1 and a hostile Target Tank#1 are unrelated -- adding one must never be influenced by the other's ids, and auto-assignment - on each side starts from 'A' independently.""" + on each side starts from '1' independently.""" board = Board() t1 = board.add_target(TargetType.TANK, _coord(), id_="1") a1 = board.add_ally(TargetType.TANK, _coord(), id_="1") assert t1.id == a1.id == "1" assert t1 is not a1 + # '1' is already taken (explicitly) for TANK on each side, so the next + # auto-assigned Tank on each side must skip it and land on '2'. t_auto = board.add_target(TargetType.TANK, _coord()) a_auto = board.add_ally(TargetType.TANK, _coord()) - assert t_auto.id == "A" # first free letter among *targets* only - assert a_auto.id == "A" # first free letter among *allies* only, unaffected by the target above + assert t_auto.id == "2" # first free number among *target* Tanks only + assert a_auto.id == "2" # first free number among *ally* Tanks only, unaffected by the target above -def test_auto_id_is_shared_across_types_within_targets_and_within_allies(): - """The id namespace split is targets-vs-allies ONLY -- different types - within the same group (all targets, or all allies) share one A/B/C... - sequence, they do NOT each get their own independent sequence. A Tank - and an Infantry auto-assigned back to back must get 'A' and 'B', never - both 'A'.""" +def test_auto_id_is_per_type_within_targets_and_within_allies(): + """Each TYPE gets its own independent 1/2/3... sequence within a group + (all targets, or all allies) -- a Tank and an Infantry auto-assigned + back to back both start at '1' (Tank#1, Infantry#1), matching how the + game itself numbers units, rather than sharing one sequence across + every type in the group.""" board = Board() tank = board.add_target(TargetType.TANK, _coord()) infantry = board.add_target(TargetType.INFANTRY, _coord()) - assert tank.id == "A" - assert infantry.id == "B" # not 'A' again just because it's a different type + assert tank.id == "1" + assert infantry.id == "1" # own sequence, not '2' just because a Tank came first + + second_tank = board.add_target(TargetType.TANK, _coord()) + assert second_tank.id == "2" # but a SECOND Tank does advance the Tank sequence ally_tank = board.add_ally(TargetType.TANK, _coord()) ally_infantry = board.add_ally(TargetType.INFANTRY, _coord()) - assert ally_tank.id == "A" - assert ally_infantry.id == "B" + assert ally_tank.id == "1" + assert ally_infantry.id == "1" -def test_auto_id_survives_past_26_entities_in_one_group(): - """A real crash: `next(c for c in string.ascii_uppercase if c not in - used)` raises StopIteration the instant all 26 letters are taken -- - reachable after accepting 26+ map-screenshot proposals into the same - group (targets, or allies) in one session, since the fix making the - id sequence shared across types (not per-type) made 26 much easier - to hit. Must roll over to two-letter ids ('AA', 'AB', ...) instead of - raising.""" +def test_auto_id_keeps_counting_past_26_entities_of_one_type(): + """The id sequence is a plain integer counter now (see + _next_free_numeric_id), not the old letter sequence that could raise + StopIteration past 26 (see TODO.md/git history) -- nothing special + should happen crossing 26, it just keeps counting.""" board = Board() for _ in range(26): board.add_target(TargetType.TANK, _coord()) twenty_seventh = board.add_target(TargetType.TANK, _coord()) - assert twenty_seventh.id == "AA" + assert twenty_seventh.id == "27" board2 = Board() for _ in range(26): board2.add_ally(TargetType.TANK, _coord()) twenty_seventh_ally = board2.add_ally(TargetType.TANK, _coord()) - assert twenty_seventh_ally.id == "AA" + assert twenty_seventh_ally.id == "27" def test_find_by_name_prefers_target_over_same_named_ally():