feat(manager): auto-compact player numbers after a pad stays off

controller-manager.py

@@ -850,13 +850,31 @@ def stop(self):
 # returns within the window is rebound in place instead of removed and re-added.
 REMOVE_GRACE = 5.0   # seconds; ~2× the 2 s poll interval
 
+# Grace before a GAP in the player numbering (a pad switched off long enough to
+# be dropped) is compacted away, closing the hole so the remaining players are
+# renumbered to a contiguous 1..N. Deliberately long: a controller whose
+# battery/accu dies and is swapped returns within this window, reclaims its old
+# number (see _poll's adoption path) and never triggers a renumber — only a pad
+# that stays off past it gives up its slot. Auto-compaction lives in _poll; the
+# tray 'Renumber players' entry forces it immediately.
+COMPACT_GRACE = 300.0   # seconds (5 minutes)
+
+def _numbering_gap(players):
+    """True when a set of player numbers is not a contiguous 1..N — i.e. a
+    removed pad left a hole compaction can close. Falsy numbers are ignored."""
+    nums = sorted(p for p in players if p)
+    return nums != list(range(1, len(nums) + 1))
+
 class ControllerManager:
     def __init__(self, on_change_cb):
         self._lock       = threading.Lock()
         self._instances  = {}      # ident → ControllerInstance
         self._config     = load_config()
         self._on_change  = on_change_cb   # called (from thread) when list changes
         self._monitor_th = threading.Thread(target=self._monitor, daemon=True)
+        # Monotonic deadline at which a standing numbering gap is compacted, or
+        # None when the numbers are contiguous. Armed/cleared each _poll pass.
+        self._compact_due = None
 
     def start(self):
         # Populate instances synchronously so the first menu we publish already
@@ -960,6 +978,22 @@ def _poll(self):
                 churned = True
             if churned:
                 self._arm_player_reassert()
+            # Auto-compaction: once a dropped pad leaves the connected numbers
+            # non-contiguous, close the gap after COMPACT_GRACE — long enough
+            # for a battery/accu swap to return and reclaim the number first,
+            # so a brief power-off never renumbers the remaining players. A gap
+            # that heals on its own (pad came back) disarms the timer.
+            if self._gap_present():
+                if self._compact_due is None:
+                    self._compact_due = now + COMPACT_GRACE
+                elif now >= self._compact_due:
+                    if self._renumber_locked():
+                        self._arm_player_reassert()
+                        changed = True
+                        config_dirty = True
+                    self._compact_due = None
+            else:
+                self._compact_due = None
         if config_dirty:
             save_config(self._config)
         if changed:
@@ -1006,6 +1040,47 @@ def _arm_player_reassert(self):
             if inst.family == "ps5":
                 inst._player_repaint_at = time.monotonic() + 6.0
 
+    def _gap_present(self):
+        """True when the connected pads' player numbers are not a contiguous
+        1..N. Caller holds the lock."""
+        return _numbering_gap(i.player for i in self._instances.values())
+
+    def _renumber_locked(self):
+        """Compact the connected pads' player numbers to a contiguous 1..N,
+        preserving their relative order: the numbers shift, but two continuously
+        connected pads never swap places and nobody overtakes anybody. The
+        stale reservations of absent pads in _players are left untouched — a
+        returning pad finds its old number now worn by a lower-numbered peer,
+        so it takes the next free one instead of reopening the closed gap.
+        Returns True if any number changed. Caller holds the lock and is
+        responsible for persisting the config and notifying afterwards."""
+        changed = False
+        ordered = sorted((i for i in self._instances.values() if i.player),
+                         key=lambda i: i.player)
+        for new_num, inst in enumerate(ordered, start=1):
+            if inst.player != new_num:
+                inst.player = new_num
+                self._config.setdefault("_players", {})[inst.ident] = new_num
+                changed = True
+        return changed
+
+    def renumber(self):
+        """Compact player numbers now (order-preserving), bypassing the
+        auto-compaction timer. Driven by the tray 'Renumber players' entry."""
+        with self._lock:
+            changed = self._renumber_locked()
+            if changed:
+                self._arm_player_reassert()
+            self._compact_due = None
+            instances = list(self._instances.values())
+        if changed:
+            save_config(self._config)
+            # Immediate feedback on the white player LEDs; the armed re-assert
+            # above outlasts Steam's slot rewrite a few seconds later.
+            for inst in instances:
+                inst._apply_player_leds()
+            GLib.idle_add(self._on_change)
+
     def set_mode(self, ident, mode):
         with self._lock:
             inst = self._instances.get(ident)
@@ -1040,6 +1115,7 @@ def __init__(self, bus, path, manager, on_quit):
         # GetLayout and its Event, misrouting the click.
         self._items    = []    # ordered [(id, props-dict)], Quit last
         self._lookup   = {}    # id → (controller ident, mode) for radio items
+        self._actions  = {}    # id → action name for plain command items
         self._sig      = None  # structural signature of the cached model
         # Item ids come from this counter and are NEVER reused for a different
         # item. The GNOME appindicator host caches item properties per id and
@@ -1100,6 +1176,11 @@ def _semantic_items(self):
                                     MODE_LABELS[mode], inst.mode == mode))
                 if inst is not instances[-1]:
                     sem.append(("sep",))
+        # Offer a manual compaction only while a gap actually exists — with a
+        # contiguous numbering the entry would be a no-op and just clutter.
+        if _numbering_gap(getattr(i, "player", None) for i in instances):
+            sem.append(("sep",))
+            sem.append(("action", "renumber", "Renumber players"))
         sem.append(("sep",))   # final separator before Quit
         return sem
 
@@ -1117,6 +1198,9 @@ def _props_for(entry):
                     "enabled":      dbus.Boolean(True),
                     "toggle-type":  dbus.String("radio"),
                     "toggle-state": dbus.Int32(1 if entry[4] else 0)}
+        if kind == "action":
+            return {"label":   dbus.String(entry[2]),
+                    "enabled": dbus.Boolean(True)}
         return {"type": dbus.String("separator")}
 
     @staticmethod
@@ -1130,6 +1214,8 @@ def _structure_sig(sem):
                 sig.append(("radio", entry[1], entry[2]))
             elif entry[0] == "header":
                 sig.append(("header", entry[1]))
+            elif entry[0] == "action":
+                sig.append(("action", entry[1]))
             else:
                 sig.append((entry[0],))
         return tuple(sig)
@@ -1142,7 +1228,7 @@ def _rebuild(self):
         sig = self._structure_sig(sem)
 
         if sig != self._sig:
-            items, lookup = [], {}
+            items, lookup, actions = [], {}, {}
             for entry in sem:
                 if self._next_id == QUIT_ID:   # never hand out the Quit id
                     self._next_id += 1
@@ -1151,11 +1237,14 @@ def _rebuild(self):
                 items.append((id_, self._props_for(entry)))
                 if entry[0] == "radio":
                     lookup[id_] = (entry[1], entry[2])
+                elif entry[0] == "action":
+                    actions[id_] = entry[1]
             # Quit is the one constant item: same kind, label and props
             # forever, so its well-known id is safe to keep.
             items.append((QUIT_ID, {"label":   dbus.String("Quit"),
                                     "enabled": dbus.Boolean(True)}))
             self._items, self._lookup, self._sig = items, lookup, sig
+            self._actions = actions
             self._revision += 1
             self.LayoutUpdated(dbus.UInt32(self._revision), dbus.Int32(0))
             return
@@ -1245,6 +1334,9 @@ def Event(self, id_, eventId, data, timestamp):
         if hit:
             ctrl_ident, mode = hit
             self._mgr.set_mode(ctrl_ident, mode)
+            return
+        if self._actions.get(int(id_)) == "renumber":
+            self._mgr.renumber()
 
     @dbus.service.method("com.canonical.dbusmenu",
                          in_signature="a(isvu)", out_signature="ai")

docs/decisions/player-leds.md

@@ -28,12 +28,26 @@ The tray label shows the same number ("DualSense 1" = the pad with one lit LED),
 menu and hardware cannot disagree — a positional label would flip after a
 drop-and-readopt while the LEDs kept their number.
 
+When a pad stays off long enough to be dropped, its number leaves a hole in the
+connected set (e.g. 1, 3, 4). The daemon compacts that hole — renumbering the
+remaining pads to a contiguous 1..N, **order-preserving** so no two continuously
+connected pads swap and nobody overtakes anybody — but only after `COMPACT_GRACE`
+(5 min), far longer than the removal grace. That window is deliberately sized for a
+dead battery/accu swap: a pad that returns within it reclaims its old number (still
+free) and no renumber happens; only a pad that stays off past it gives up its slot.
+A tray entry, "Renumber players" (shown only while a hole exists), forces the
+compaction immediately. The absent pad's stale reservation is left in `_players`
+untouched — a much-later return finds its old number now worn by a lower-numbered
+peer and takes the next free one, so a reclaim never reopens a closed hole.
+
 ## Consequences
 
-- Numbers are stable across mode switches, reconnects and daemon restarts; the
-  first-connected pad stays player 1 for good.
-- A number is freed only by a full removal (grace elapsed) and reused by the next new
-  pad — a fixed set of pads keeps fixed numbers forever.
+- Numbers are stable across mode switches, reconnects and daemon restarts; a pad
+  keeps its number as long as the connected set is unchanged.
+- A number is freed by a full removal (grace elapsed); the resulting hole is then
+  compacted after `COMPACT_GRACE`, or at once via the tray entry, so the connected
+  pads stay a contiguous 1..N. A brief power-off (battery swap) inside the window
+  reclaims the number and triggers no renumber.
 - Steam's own count may still flash for a few seconds after churn until the re-assert
   wins; accepted, same policy as the lightbar reopen handling.
 - Xbox pads get a number too (for stable labels), but their LED ring is left to the

runbooks/verify-multi-controller.md

@@ -43,6 +43,20 @@ its nodes; use the fresh numbers below.
 Power-cycle DualSense 2 (or walk it out of BT range for >10 s), reconnect it, wait two
 poll ticks (~5 s).
 
+### 5. Renumber after a pad stays off
+
+Power **off** DualSense 1 (player 1) and leave it off. Within a few seconds its tray
+entry disappears; the remaining pads still read 2, 3, 4 — a hole at 1 — and a
+**"Renumber players"** entry appears near the bottom of the menu. Close the hole either
+way:
+
+- click **Renumber players** to compact immediately, or
+- wait `COMPACT_GRACE` (5 min) for the automatic compaction.
+
+Then power DualSense 1 back on and confirm it rejoins at the end, not by displacing a
+peer. For the battery-swap case, instead power a pad off and back on **within** the
+window (and before clicking Renumber).
+
 ## Verification
 
 **Menu is complete and numbered** (after step 2):
@@ -81,6 +95,17 @@ python3 -c "import evdev; print(*[evdev.InputDevice(p).name for p in evdev.list_
 - All menu items still clickable: switch DualSense 1 → Emulate Xbox and back — both
   clicks take effect (guards the id-recycling regression, `tests/test_menu_ids.py`).
 
+**Compaction closes the hole, order-preserving** (after step 5):
+
+- Once compacted the connected pads are numbered 1..N with no gap, relative order
+  preserved (former player 3 → 2, former 4 → 3 — nobody leapfrogs). White player LEDs
+  and menu labels both follow.
+- The "Renumber players" entry is gone once the numbering is contiguous.
+- DualSense 1 powered back on **after** the window takes the next free number — it does
+  NOT evict a currently-connected pad.
+- Battery-swap safety: a pad powered off and back on **within** the window (before any
+  Renumber) reclaims its old number, and nothing else renumbers.
+
 **Input reaches applications per pad** — with a gamepad tester (or `evtest`), press
 buttons on each pad in turn: each event stream stays on its own device, DualSense 2's
 events arrive under the virtual X-Box identity with X/Y correct (top button = Y).
@@ -96,6 +121,8 @@ events arrive under the virtual X-Box identity with X/Y correct (top button = Y)
 | Player LEDs rotate with mode switches | daemon re-assert missing (old build) — Steam rewrites its slot count on every rebind; update to the `_players` build |
 | One pad's lightbar never lights in ANY mode, but its player LEDs follow the daemon | lightbar defect on that unit — player LEDs share report and transport, so the write path is proven; cross-check the pad's own firmware-driven animations (pairing pulse, charging pulse) with no host involved |
 | Menu items dead after churn | the id-recycling bug class ([tray-menu-model](../docs/decisions/tray-menu-model.md)) — capture `dbus-monitor` output and reopen |
+| Hole never closes after a pad stays off >5 min | auto-compaction disabled/old build — check the `COMPACT_GRACE` path in `_poll`; "Renumber players" forces it |
+| A pad returning after a long absence steals a connected pad's number | compaction/reclaim regression — it should take the next free number; capture `_players` from the config and file an issue |
 | Second identical pad collapses into the first (USB) | both report empty `uniq` AND identical `phys` — should not happen on distinct ports; file an issue with the step-1 output |
 
 ## Rollback

tests/test_multi_pad.py

@@ -55,6 +55,7 @@ def virtual_path(self): return None
     def remap_healthy(self): return True
     def refresh_led(self): pass
     def watch_holders(self, steam_game=False): pass
+    def _apply_player_leds(self): pass
 
 cm.ControllerInstance = FakeInst
 
@@ -280,6 +281,95 @@ def close(self): pass
 check("event54" in found and found["event54"]["name"] == "DualSense",
       "real DualSense adopted with its display name")
 
+# ── Scenario H: auto-compaction closes a numbering gap after COMPACT_GRACE ───
+print("Scenario H: a pad left off past grace, then the gap is compacted on time")
+saved_configs.clear(); config_store[0] = {}
+clock[0] = 5000.0
+two = [ds("event60", "AA:AA"), ds("event61", "BB:BB")]
+mgr = make_mgr([list(two), [two[1]], [two[1]], [two[1]], [two[1]]])
+mgr._poll()                                    # adopt AA=1, BB=2
+insts = by_ident(mgr)
+check(insts["AA:AA"].player == 1 and insts["BB:BB"].player == 2,
+      "two pads adopted as players 1 and 2")
+mgr._poll()                                    # AA absent, within grace
+check(mgr._compact_due is None, "no gap while the dropped pad is still in grace")
+clock[0] += cm.REMOVE_GRACE + 0.1
+mgr._poll()                                    # AA dropped -> gap {2}, arm timer
+check(len(mgr._instances) == 1, "AA dropped past grace")
+check(by_ident(mgr)["BB:BB"].player == 2, "BB keeps its number while the timer runs")
+check(mgr._compact_due is not None, "a standing gap arms the compaction timer")
+clock[0] += 1.0
+mgr._poll()                                    # still before due
+check(by_ident(mgr)["BB:BB"].player == 2, "no compaction before COMPACT_GRACE")
+clock[0] += cm.COMPACT_GRACE
+mgr._poll()                                    # due -> compact
+check(by_ident(mgr)["BB:BB"].player == 1, "gap compacted: BB renumbered to 1")
+check(mgr._compact_due is None, "timer disarmed once the numbering is contiguous")
+check(saved_configs and saved_configs[-1].get("_players", {}).get("BB:BB") == 1,
+      "compacted number persisted under _players")
+
+# ── Scenario I: a returning pad within grace reclaims its number, no renumber ─
+print("Scenario I: battery swap inside the window reclaims the number, no compaction")
+config_store[0] = {}
+clock[0] = 5500.0
+two = [ds("event62", "CC:CC"), ds("event63", "DD:DD")]
+back = ds("event64", "CC:CC")                   # CC returns on a new node
+mgr = make_mgr([list(two), [two[1]], [back, two[1]], [back, two[1]]])
+mgr._poll()                                     # CC=1, DD=2
+mgr._poll()                                     # CC absent, within grace
+clock[0] += 1.0
+mgr._poll()                                     # CC back before grace elapses
+check(mgr._compact_due is None, "reclaim leaves the numbering contiguous, no timer")
+insts = by_ident(mgr)
+check(insts["CC:CC"].player == 1 and insts["DD:DD"].player == 2,
+      "returning pad kept its number; nobody was renumbered")
+
+# ── Scenario J: manual renumber compacts immediately, order-preserving ───────
+print("Scenario J: 'Renumber players' compacts now, preserving order")
+saved_configs.clear(); config_store[0] = {}
+clock[0] = 6000.0
+three = [ds("e70", "AA:AA"), ds("e71", "BB:BB"), ds("e72", "CC:CC")]
+mgr = make_mgr([list(three), [three[0], three[2]], [three[0], three[2]]])
+mgr._poll()                                     # AA=1, BB=2, CC=3
+mgr._poll()                                     # BB absent, within grace
+clock[0] += cm.REMOVE_GRACE + 0.1
+mgr._poll()                                     # BB dropped -> gap {1,3}
+check(by_ident(mgr)["CC:CC"].player == 3, "CC still 3 before the manual renumber")
+mgr.renumber()
+insts = by_ident(mgr)
+check(insts["AA:AA"].player == 1 and insts["CC:CC"].player == 2,
+      "manual renumber compacts to 1,2 preserving order (AA before CC)")
+check(mgr._compact_due is None, "manual renumber clears any pending timer")
+
+# ── Scenario K: the gap surfaces a 'Renumber players' entry that routes ──────
+print("Scenario K: a numbering gap surfaces a Renumber players entry")
+class GapMgr:
+    def __init__(self, instances):
+        self.instances = instances; self.renumber_calls = 0
+    def get_instances(self): return list(self.instances)
+    def set_mode(self, ident, mode): pass
+    def renumber(self): self.renumber_calls += 1
+def build_menu(mgr):
+    m = object.__new__(cm.DbusmenuServer)
+    m._mgr = mgr; m._on_quit = lambda: None; m._revision = 1
+    m._items = []; m._lookup = {}; m._actions = {}; m._sig = None; m._next_id = 1
+    m.LayoutUpdated = lambda rev, parent: None
+    m.ItemsPropertiesUpdated = lambda updated, removed: None
+    m.notify_update()
+    return m
+gap_menu = build_menu(GapMgr([Pad("AA:AA", "DualSense", player=1),
+                              Pad("BB:BB", "DualSense", player=3)]))  # missing 2
+action_ids = [i for i, p in gap_menu._items
+              if str(p.get("label", "")) == "Renumber players"]
+check(len(action_ids) == 1, "gap surfaces exactly one Renumber players entry")
+gap_menu.Event(action_ids[0], "clicked", 0, 0)
+check(gap_menu._mgr.renumber_calls == 1, "clicking Renumber players calls mgr.renumber")
+contig_menu = build_menu(GapMgr([Pad("AA:AA", "DualSense", player=1),
+                                 Pad("BB:BB", "DualSense", player=2)]))
+check(not any(str(p.get("label", "")) == "Renumber players"
+              for _, p in contig_menu._items),
+      "contiguous numbering shows no Renumber players entry")
+
 print()
 print("RESULT:", "ALL PASS" if not fails else f"{len(fails)} FAILED: {fails}")
 sys.exit(1 if fails else 0)