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 <noreply@anthropic.com>

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 "<Type>#<id> 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.

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 "#<N>" 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"<b>{GLib.markup_escape_text(proposal.coord)}</b> — {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}",

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 "#<N>" 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):

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:
+def _next_free_numeric_id(used: set[str]) -> str:
-    """Next unused id in a short, human-friendly sequence: single
+    """Next unused id in a plain 1, 2, 3... sequence -- matches the small
-    uppercase letters (A..Z) first, then two-letter combinations
+    integer ids the game itself shows per unit (see
-    (AA..ZZ, spreadsheet-column style) once those run out, and so on.
+    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
+    Older versions of this used a letter sequence (A, B, C..., rolling
-    if c not in used)` raises StopIteration the instant all 26 letters
+    over to AA/AB/... past 26) shared across a whole group instead of
-    are taken, which used to need 26+ auto-added entities of one TYPE
+    per type -- switched away from per-type once before (see TODO.md)
-    (rare) but, once add_target()/add_ally() moved to one shared id
+    because sharing made it too easy to run past 26 letters. That's not
-    sequence per GROUP instead of per type (so a Tank and an Infantry
+    a concern here: a plain counter never runs out, there's no fixed
-    added back to back get 'A'/'B', not both 'A', see their own
+    alphabet to exhaust regardless of how it's scoped."""
-    comments), needs only 26 auto-added entities of ANY type in that
+    n = 1
-    group -- reachable in a single big screenshot import. This can't run
+    while str(n) in used:
-    out: it just grows the id length instead."""
+        n += 1
-    length = 1
+    return str(n)
-    while True:
-        for combo in itertools.product(string.ascii_uppercase, repeat=length):
-            candidate = "".join(combo)
-            if candidate not in used:
-                return candidate
-        length += 1
 
 
 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
+        # Own 1/2/3... sequence per TYPE, not one shared across every
-        # type, not one sequence per type -- a Tank and an Infantry auto-
+        # target regardless of type -- a Tank and an Infantry auto-
-        # assigned back to back get 'A' and 'B', never both 'A'. Only
+        # assigned back to back both start at '1' (Tank#1, Infantry#1),
-        # targets-vs-allies is a separate id namespace (see add_ally),
+        # matching how the game itself numbers units. targets-vs-allies
-        # type never subdivides it further.
+        # 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}
+            used = {t.id for t in self.targets if t.type == type_}
-            id_ = _next_free_id(used)
+            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
+        # here only looks at other allies, never self.targets. Own
-        # add_target though, that's the ONLY split: one shared A/B/C...
+        # 1/2/3... sequence per TYPE too, same as add_target -- see its
-        # sequence across every ally regardless of type, not one per type.
+        # own comment and _next_free_numeric_id.
         if not id_:
-            used = {a.id for a in self.allies}
+            used = {a.id for a in self.allies if a.type == type_}
-            id_ = _next_free_id(used)
+            id_ = _next_free_numeric_id(used)
         a = Ally(type=type_, id=id_, location=_as_location(location))
         self.allies.append(a)
         return a

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 "#<N>" 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
+there's no detection, or it collides with an id already used for that
-group (see _accept_proposal's own docstring).
+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():

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 t_auto.id == "2"  # first free number among *target* Tanks only
-    assert a_auto.id == "A"  # first free letter among *allies* only, unaffected by the target above
+    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():
+def test_auto_id_is_per_type_within_targets_and_within_allies():
-    """The id namespace split is targets-vs-allies ONLY -- different types
+    """Each TYPE gets its own independent 1/2/3... sequence within a group
-    within the same group (all targets, or all allies) share one A/B/C...
+    (all targets, or all allies) -- a Tank and an Infantry auto-assigned
-    sequence, they do NOT each get their own independent sequence. A Tank
+    back to back both start at '1' (Tank#1, Infantry#1), matching how the
-    and an Infantry auto-assigned back to back must get 'A' and 'B', never
+    game itself numbers units, rather than sharing one sequence across
-    both 'A'."""
+    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 tank.id == "1"
-    assert infantry.id == "B"  # not 'A' again just because it's a different type
+    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_tank.id == "1"
-    assert ally_infantry.id == "B"
+    assert ally_infantry.id == "1"
 
 
-def test_auto_id_survives_past_26_entities_in_one_group():
+def test_auto_id_keeps_counting_past_26_entities_of_one_type():
-    """A real crash: `next(c for c in string.ascii_uppercase if c not in
+    """The id sequence is a plain integer counter now (see
-    used)` raises StopIteration the instant all 26 letters are taken --
+    _next_free_numeric_id), not the old letter sequence that could raise
-    reachable after accepting 26+ map-screenshot proposals into the same
+    StopIteration past 26 (see TODO.md/git history) -- nothing special
-    group (targets, or allies) in one session, since the fix making the
+    should happen crossing 26, it just keeps counting."""
-    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."""
     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():