diff --git a/TODO.md b/TODO.md index b6a9354..e0caeac 100644 --- a/TODO.md +++ b/TODO.md @@ -207,23 +207,41 @@ Status legend: [x] fixed+tested, [~] partially addressed, [ ] open/needs input (`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 + sequence per group (targets, or allies) to its own sequence per + TYPE within each group -- Tank#A/Infantry#A rather than + Tank#A/Infantry#B. 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. 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). + + First pass at this ALSO switched auto-assignment from letters to + plain numbers (1/2/3...), reasoning that it should match what + `detected_id` looks like when read successfully. Wrong -- caught + by the user immediately: auto-assignment (no real id known, a + manual add or an accept with no confident read) and a genuinely + detected id need to stay visually distinct, or a made-up + auto-assigned number could collide with, or be mistaken for, a + real one. Reverted back to `_next_free_id` (letters, rolling + over to "AA"/"AB"/... past 26 rather than raising + `StopIteration`) as the auto-assignment fallback, scoped per + type same as above; plain numbers are reserved for an id + `_accept_proposal` is actually confident was read off the + marker itself (`Proposal.detected_id`), passed straight through + as `id_` and never touching auto-assignment at all. + - `_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 (scoped per type, same bug fixed + in two places: this collision pre-check, and the "Change ID" + popover's own check, which still enforced the OLD shared-per- + group rule after the auto-assignment change above and rejected + valid renames across types). 4 new regression tests + (`tests/test_app_accept_proposal.py`). - `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 diff --git a/src/fenigma/app.py b/src/fenigma/app.py index ef0fd87..5a20a83 100644 --- a/src/fenigma/app.py +++ b/src/fenigma/app.py @@ -760,7 +760,8 @@ class MainWindow(Adw.ApplicationWindow): # than losing this one accept's traceability to the game's own # number. id_ = proposal.detected_id - existing = {a.id for a in self.board.allies} if is_ally else {t.id for t in self.board.targets} + group = self.board.allies if is_ally else self.board.targets + existing = {o.id for o in group if o.type == type_} # per-type, same as Board.add_target/add_ally if id_ in existing: id_ = None if is_ally: diff --git a/src/fenigma/models.py b/src/fenigma/models.py index db3c36e..e5724ab 100644 --- a/src/fenigma/models.py +++ b/src/fenigma/models.py @@ -18,6 +18,7 @@ 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 @@ -502,28 +503,37 @@ class ScoutFlight: return f"ScoutFlight#{self.id}" -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. +def _next_free_id(used: set[str]) -> str: + """Next unused id in a short, human-friendly LETTER sequence: single + uppercase letters (A..Z) first, then two-letter combinations + (AA..ZZ, spreadsheet-column style) once those run out, and so on. + 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 'A' (Tank#A, + Infantry#A), each type keeping its own independent sequence. - 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) + Deliberately letters, not numbers: a manually-added entity (map + right-click "Add target", or an accepted screenshot proposal with no + confident id read) has no real game id to report, so it gets an + obviously-not-a-real-id placeholder instead -- app.py's + _accept_proposal reserves plain numbers for an id it's actually + confident was read off the marker itself (map_vision.read_marker_id + via Proposal.detected_id), passed straight through as this + function's caller's `id_` and never touching this auto-assignment at + all. Letters can't collide with a real (numeric) detected id either, + on top of just reading honestly as 'made up'. + + Can't run out the way `next(c for c in string.ascii_uppercase if c + not in used)` used to (a real regression, see TODO.md): rolls over to + two-letter ids ('AA', 'AB', ...) past 26 instead of raising + StopIteration.""" + 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 SAVE_FORMAT_VERSION = 3 @@ -596,15 +606,18 @@ class Board: location: Location | Coord | None = None, id_: str | None = None, ) -> Target: - # Own 1/2/3... sequence per TYPE, not one shared across every + # Own A/B/C... 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. + # assigned back to back both start at 'A' (Tank#A, Infantry#A). + # targets-vs-allies is still its own separate id namespace (see + # add_ally); type now subdivides it further too. Letters, not + # numbers, when auto-assigning here specifically: see + # _next_free_id's own docstring for why (a real detected id, when + # there is one, is passed in as `id_` and never reaches this + # auto-assignment at all). if not id_: used = {t.id for t in self.targets if t.type == type_} - id_ = _next_free_numeric_id(used) + id_ = _next_free_id(used) t = Target(type=type_, id=id_, location=_as_location(location)) self.targets.append(t) return t @@ -619,14 +632,14 @@ class Board: location: Location | Coord | None = None, id_: str | None = None, ) -> Ally: - # A separate id namespace from add_target()'s: an ally Tank#1 - # and a hostile Target Tank#1 are unrelated, so auto-assignment + # A separate id namespace from add_target()'s: an ally Tank#A + # and a hostile Target Tank#A are unrelated, so auto-assignment # 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. + # A/B/C... sequence per TYPE too, same as add_target -- see its + # own comment and _next_free_id. if not id_: used = {a.id for a in self.allies if a.type == type_} - id_ = _next_free_numeric_id(used) + id_ = _next_free_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 a810154..5acc42e 100644 --- a/tests/test_app_accept_proposal.py +++ b/tests/test_app_accept_proposal.py @@ -1,11 +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 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 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). +Proposal.detected_id) over an auto-assigned letter, so ids on the board +match what's actually on screen -- falling back to auto-assign (a +letter, deliberately not a number, so it can't collide with or be +mistaken for a real detected id -- see models.py's _next_free_id) only +when there's no detection, or it collides with an id already used for +that type in that group (see _accept_proposal's own docstring). Needs a real Adw/Gtk init (MainWindow.__new__ skips __init__, so no window/widgets are actually built, but Adw.init() is still required for @@ -46,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 == "1" + assert win.board.targets[0].id == "A" 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 6f47979..f11bdcd 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -54,58 +54,60 @@ 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 '1' independently.""" + on each side starts from 'A' independently. Explicit id_="1" here + (as an accepted screenshot proposal's detected_id would pass, see + app.py's _accept_proposal) to also check that auto-assignment + correctly skips a real numeric id already in use, not just other + letters.""" 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 == "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 + assert t_auto.id == "A" # first free letter among *target* Tanks only + assert a_auto.id == "A" # first free letter among *ally* Tanks only, unaffected by the target above 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 + """Each TYPE gets its own independent A/B/C... 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.""" + back to back both start at 'A' (Tank#A, Infantry#A), 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 == "1" - assert infantry.id == "1" # own sequence, not '2' just because a Tank came first + assert tank.id == "A" + assert infantry.id == "A" # own sequence, not 'B' 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 + assert second_tank.id == "B" # 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 == "1" - assert ally_infantry.id == "1" + assert ally_tank.id == "A" + assert ally_infantry.id == "A" -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.""" +def test_auto_id_survives_past_26_entities_of_one_type(): + """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/adding 26+ of the same type into one group + in a single session. 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 == "27" + assert twenty_seventh.id == "AA" 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 == "27" + assert twenty_seventh_ally.id == "AA" def test_find_by_name_prefers_target_over_same_named_ally():