From 19865f6b54200869b977c4bef66a44442d8f54c4 Mon Sep 17 00:00:00 2001 From: grechmarlon <55017324+grechmarlon@users.noreply.github.com> Date: Fri, 12 Jun 2026 06:27:29 +0200 Subject: [PATCH] address review: name gap sentinel, hoist init guard, scope power-on force MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- lib/logitech_receiver/device.py | 9 ++++++--- lib/logitech_receiver/notifications.py | 17 ++++++++++------- lib/logitech_receiver/settings_templates.py | 15 +++++++++++---- lib/solaar/ui/perkey/canvas.py | 14 ++++++++++---- tests/logitech_receiver/test_notifications.py | 16 ++++++++++++++++ 5 files changed, 53 insertions(+), 18 deletions(-) diff --git a/lib/logitech_receiver/device.py b/lib/logitech_receiver/device.py index 4bff8bdf..575d6c7a 100644 --- a/lib/logitech_receiver/device.py +++ b/lib/logitech_receiver/device.py @@ -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 diff --git a/lib/logitech_receiver/notifications.py b/lib/logitech_receiver/notifications.py index 76516783..132fa354 100644 --- a/lib/logitech_receiver/notifications.py +++ b/lib/logitech_receiver/notifications.py @@ -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) diff --git a/lib/logitech_receiver/settings_templates.py b/lib/logitech_receiver/settings_templates.py index e35ebe3b..3bbc4896 100644 --- a/lib/logitech_receiver/settings_templates.py +++ b/lib/logitech_receiver/settings_templates.py @@ -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 diff --git a/lib/solaar/ui/perkey/canvas.py b/lib/solaar/ui/perkey/canvas.py index bdead4bc..cea32d07 100644 --- a/lib/solaar/ui/perkey/canvas.py +++ b/lib/solaar/ui/perkey/canvas.py @@ -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 = [] diff --git a/tests/logitech_receiver/test_notifications.py b/tests/logitech_receiver/test_notifications.py index cbe51aab..8dc8a4f4 100644 --- a/tests/logitech_receiver/test_notifications.py +++ b/tests/logitech_receiver/test_notifications.py @@ -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 inactive→active 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"