Revert auto-assigned ids back to letters; only detected ids are numeric
Auto-assignment (no real id known: a manual add, or an accepted proposal with no confident marker-id read) must stay visually distinct from a genuinely detected id, or a made-up number could collide with or be mistaken for a real one. Reverts the previous commit's switch to numeric auto-assignment (_next_free_numeric_id) -- that was wrong, caught by the user immediately. _next_free_id (letters, rolling over to "AA"/"AB"/... past 26) is back as the fallback, still scoped per type (that part of the previous change was correct and stays). Plain numbers are reserved for an id _accept_proposal is actually confident was read off the marker itself (Proposal.detected_id), passed straight through and never touching auto-assignment. Also fixes detected_id's own collision pre-check in _accept_proposal, which wasn't scoped per type either -- same bug as the Change ID popover fix, just in a second place: a detected id could get needlessly discarded because an unrelated type already used that number, not because of a real collision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
6e18d60eb5
commit
5a35ea7776
48
TODO.md
48
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
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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
|
||||
|
||||
@ -1,11 +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 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():
|
||||
|
||||
@ -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():
|
||||
|
||||
Loading…
Reference in New Issue
Block a user