address review: name gap sentinel, hoist init guard, scope power-on force

- GAP_ZONE_ID names the canvas' phantom gap-cell zone and the delta
  filter tests keys against it explicitly, so it can't be confused with
  the -1 "No change" color value; _sanitize_map's docstring states it
  filters keys only.
- The _value init guard in PerKeyLighting.write_key_value moves above
  both branches — the un-set branch also mutates _value — matching the
  base class, with a note on why it tests `is None`.
- A powered-on reconfig forces the apply only when the device was
  already marked active: on the inactive→active transition changed()
  just applied everything, so forcing would push a redundant duplicate
  frame. Regression test covers both cases.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
grechmarlon 2026-06-12 06:27:29 +02:00
parent c26f3cd6ec
commit 19865f6b54
5 changed files with 53 additions and 18 deletions

View File

@ -473,9 +473,12 @@ class Device:
match a stored cookie from a prior session and skip the apply the
device actually needs.
`force` skips the cookie comparison (but still re-records): pass it
when the notification says the device just powered on volatile
state (RGB buffers, host-mode lighting) is gone even on devices
whose ConfigChange cookie survives the power cycle (e.g. G915 TKL).
when the device powered on while already marked active no
activation apply ran in changed(), yet volatile state (RGB buffers,
host-mode lighting) is gone even on devices whose ConfigChange
cookie survives the power cycle (e.g. G915 TKL). When the device
was inactive, changed() just applied everything, so forcing here
would only push a redundant duplicate.
Returns True if apply ran, False if it was skipped."""
if not self.online:
return False

View File

@ -326,14 +326,17 @@ def _process_feature_notification(device: Device, notification: HIDPPNotificatio
reason = "powered on" if powered_on else None
if notification.data[1] == 1: # device is asking for software reconfiguration
alert = Alert.NONE
was_active = bool(device.online) # before changed() updates it
device.changed(active=True, alert=alert, reason=reason)
# changed(active=True) already runs apply_settings_if_needed on
# the first transition; for follow-up reconfig notifications
# on an already-active device, fire the gate here so the
# cookie comparison decides whether to re-push. A power-on
# forces the apply: volatile state (RGB buffers, host-mode
# lighting) is gone even when the cookie survived the cycle.
device.apply_settings_if_needed(force=powered_on)
# changed(active=True) applies all settings unconditionally on
# the inactive→active transition; for follow-up reconfig
# notifications on an already-active device, fire the gate here
# so the cookie comparison decides whether to re-push. A
# power-on while already marked active forces the apply: no
# activation apply ran, yet volatile state (RGB buffers,
# host-mode lighting) is gone even when the cookie survived
# the cycle (e.g. G915 TKL).
device.apply_settings_if_needed(force=powered_on and was_active)
else:
logger.warning("%s: unknown WIRELESS %s", device, notification)

View File

@ -3878,9 +3878,11 @@ class PerKeyLighting(settings.Settings):
return {int(k) for k in choices} if choices is not None else None
def _sanitize_map(self, value):
"""Drop zone keys the device never reported (e.g. the editor's -1
"""Drop zone keys the device never reported (e.g. the editor's gap
placeholder, or junk from an older persisted config). A single bad
key would otherwise abort the whole frame at pack time."""
key would otherwise abort the whole frame at pack time.
Filters map KEYS only the -1 "No change" sentinel as a map VALUE
is legitimate and handled separately in _send_perkey_frame."""
valid = self._valid_zones()
if not isinstance(value, dict) or valid is None:
return value
@ -4131,9 +4133,14 @@ class PerKeyLighting(settings.Settings):
if valid is not None and zone_id not in valid:
logger.warning("%s: ignoring write to invalid per-key zone %s", self.name, zone_id)
return value
if self._value is None:
# 0x8081 is write-only — read() loads persisted state or the all-
# "No change" sentinel map. Both branches below mutate _value, so
# guard here (like the base class does). `is None` rather than the
# base class's falsy test: read() never yields an empty map here,
# so None is precisely "never initialized".
self.read()
if value != no_change:
if self._value is None:
self.read() # 0x8081 is write-only — read() loads persisted state or the sentinel map
self.update_key_value(zone_id, value, save)
if not self._device.online:
return value

View File

@ -54,6 +54,11 @@ GUTTER_PX = 4
STRIP_GAP_PX = 16
PADDING_PX = 8
# Phantom anchor zone for matrix gaps (positions with no key). Never a real
# zone, never sent to the device — distinct from the -1 "No change" color
# VALUE (special_keys.COLORSPLUS / control.NO_CHANGE).
GAP_ZONE_ID = -1
class KeyboardCanvas(Gtk.DrawingArea):
__gsignals__ = {
@ -178,7 +183,7 @@ class KeyboardCanvas(Gtk.DrawingArea):
col = int((x - PADDING_PX) // (CELL_PX + GUTTER_PX))
row = int((y - PADDING_PX) // (CELL_PX + GUTTER_PX))
if 0 <= col < cols and 0 <= row < rows:
return BoundCell(cell=Cell(zone_id=-1, row=row, col=col), bound=False)
return BoundCell(cell=Cell(zone_id=GAP_ZONE_ID, row=row, col=col), bound=False)
return None
# ---- draw ----
@ -438,9 +443,10 @@ class KeyboardCanvas(Gtk.DrawingArea):
delta: dict[int, int] = {}
if tool is not None:
delta = tool.compute(self._press_cell, self._motion_cell, list(self._brush_path), ctx)
# Strokes can sweep over layout gaps, which are placeholder cells
# with zone_id -1 — not real zones; they must never reach the sink.
delta = {z: c for z, c in delta.items() if int(z) >= 0}
# Strokes can sweep over layout gaps; drop their phantom cells so
# they never reach the sink. This filters KEYS — "No change" color
# VALUES (-1) pass through untouched.
delta = {z: c for z, c in delta.items() if z != GAP_ZONE_ID}
self._press_cell = None
self._motion_cell = None
self._brush_path = []

View File

@ -279,6 +279,22 @@ def test_process_feature_notification(mocker, hidpp_notification, feature):
assert result is True
@pytest.mark.parametrize("was_active, expected_force", [(True, True), (False, False)])
def test_wireless_power_on_reconfig_forces_apply_only_when_already_active(mocker, was_active, expected_force):
"""A powered-on reconfig request must bypass the config-cookie gate
exactly when changed() ran no activation apply (device already active).
On the inactiveactive transition changed() just applied everything, so
forcing there would push a redundant duplicate."""
device = mocker.Mock()
device.online = was_active # state before changed() updates it
device.features.get_feature.return_value = SupportedFeature.WIRELESS_DEVICE_STATUS
hidpp_notification = HIDPPNotification(0, 0, sub_id=0x05, address=0x00, data=b"\x00\x01\x01")
notifications._process_feature_notification(device, hidpp_notification)
device.apply_settings_if_needed.assert_called_once_with(force=expected_force)
def test_process_receiver_notification_invalid(mocker):
invalid_sub_id = 0x30
notification_data = b"\x02"