From 20b90b4f2fde2c9e3feede62629f9c4f3b58e3f2 Mon Sep 17 00:00:00 2001 From: I4cDeath Date: Sat, 25 Jul 2026 03:05:13 -0500 Subject: [PATCH] =?UTF-8?q?fix(env):=20=E2=9C=95=20button=20never=20remove?= =?UTF-8?q?d=20rows=20=E2=80=94=20override=20removeRows=20virtual=20(#52)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QML's model.removeRow(i) resolves to QAbstractItemModel's built-in C++ convenience, not the same-named Python slot, so the slot was unreachable and the call fell through to the default removeRows() stub that does nothing and returns false. Override the removeRows() virtual instead — the hook the C++ convenience actually delegates to. Adds a regression test that drives the call through a real QML engine (offscreen) so the broken dispatch path is the one under test. Bumps to 1.0.1 with changelog + metainfo release entry. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 10 +++ ...io.github.i4ctime.protonshift.metainfo.xml | 5 ++ protonshift/__init__.py | 2 +- protonshift/controllers/env_controller.py | 19 +++--- pyproject.toml | 2 +- tests/test_env_remove_row.py | 64 +++++++++++++++++++ 6 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 tests/test_env_remove_row.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9e2b3..c25ace2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.0.1] — 2026-07-25 + +### Fixed +- **Environment Variables: the ✕ button now removes the row** (#52). QML's + `model.removeRow(i)` was resolving to QAbstractItemModel's built-in C++ + convenience instead of the same-named Python slot, landing in the default + `removeRows()` stub that does nothing. The model now overrides the + `removeRows()` virtual — the supported hook — so removal works and marks + the editor dirty. + ### Docs - Dropped the stale-release caveat from the README now that `v1.0.0` is published. diff --git a/flatpak/io.github.i4ctime.protonshift.metainfo.xml b/flatpak/io.github.i4ctime.protonshift.metainfo.xml index 82f72dc..ba80021 100644 --- a/flatpak/io.github.i4ctime.protonshift.metainfo.xml +++ b/flatpak/io.github.i4ctime.protonshift.metainfo.xml @@ -56,6 +56,11 @@ + + +

Bug fix: the remove button in the Environment Variables editor now actually deletes the row.

+ +

First stable release of the native Qt Quick / PySide6 rewrite.

diff --git a/protonshift/__init__.py b/protonshift/__init__.py index 974c2fa..a277ef4 100644 --- a/protonshift/__init__.py +++ b/protonshift/__init__.py @@ -4,4 +4,4 @@ domain logic lives in :mod:`protonshift.core`. No Electron, no web server. """ -__version__ = "1.0.0" +__version__ = "1.0.1" diff --git a/protonshift/controllers/env_controller.py b/protonshift/controllers/env_controller.py index 8aabc98..e9da901 100644 --- a/protonshift/controllers/env_controller.py +++ b/protonshift/controllers/env_controller.py @@ -82,16 +82,17 @@ def addRow(self) -> None: # noqa: N802 self.endInsertRows() self.modified.emit() - # Properly overrides the QAbstractItemModel::removeRow(int, QModelIndex) - # virtual (compatible signature + bool return) instead of shadowing it with - # an incompatible Slot. QML's existing `model.removeRow(i)` calls still - # work — the extra parent argument defaults and the return is ignorable. - @Slot(int, result=bool) - def removeRow(self, row: int, parent: QModelIndex = QModelIndex()) -> bool: # noqa: N802 - if parent.isValid() or not (0 <= row < len(self._rows)): + # QML's ✕ button calls `model.removeRow(i)`. Never define a Python method + # named `removeRow` here: QAbstractItemModel already exposes an invokable + # C++ removeRow(int, parent) convenience, and QML dispatches to that + # built-in — a same-named Python slot is unreachable from QML (#52: the + # ✕ button was a silent no-op). The convenience delegates to the + # *virtual* removeRows(), so that is the hook to override. + def removeRows(self, row: int, count: int, parent: QModelIndex = QModelIndex()) -> bool: # noqa: N802 + if parent.isValid() or count < 1 or row < 0 or row + count > len(self._rows): return False - self.beginRemoveRows(QModelIndex(), row, row) - del self._rows[row] + self.beginRemoveRows(QModelIndex(), row, row + count - 1) + del self._rows[row : row + count] self.endRemoveRows() self.modified.emit() return True diff --git a/pyproject.toml b/pyproject.toml index 6218068..e1925db 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "protonshift" -version = "1.0.0" +version = "1.0.1" description = "Native Qt Quick edition of ProtonShift — a Linux gaming setup tool" readme = "README.md" requires-python = ">=3.11" diff --git a/tests/test_env_remove_row.py b/tests/test_env_remove_row.py new file mode 100644 index 0000000..0b126b5 --- /dev/null +++ b/tests/test_env_remove_row.py @@ -0,0 +1,64 @@ +"""Regression test for issue #52: the env editor's ✕ button was a no-op. + +QML calls ``model.removeRow(i)``. QAbstractItemModel already exposes that +name as an invokable C++ convenience, so a same-named Python slot is never +reached from QML — the call lands on the built-in, which delegates to the +virtual ``removeRows()``. The fix overrides ``removeRows``; this test drives +the call through a real QML engine (not Python) so the dispatch path that +broke is the one being tested. Runs headless via QT_QPA_PLATFORM=offscreen +(set in CI; forced here for local runs). +""" + +from __future__ import annotations + +import os + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + +from PySide6.QtCore import QUrl # noqa: E402 +from PySide6.QtGui import QGuiApplication # noqa: E402 +from PySide6.QtQml import QQmlApplicationEngine # noqa: E402 + +from protonshift.controllers.env_controller import EnvVarsModel # noqa: E402 + +_QML = b""" +import QtQml +QtObject { + required property var model + function removeAt(i) { return model.removeRow(i) } +} +""" + + +def _qml_root(model: EnvVarsModel) -> tuple[QQmlApplicationEngine, object]: + # engine returned so it outlives the root object in the caller's scope + engine = QQmlApplicationEngine() + engine.setInitialProperties({"model": model}) + engine.loadData(_QML, QUrl("test_env_remove_row.qml")) + roots = engine.rootObjects() + assert roots, "inline QML failed to load" + return engine, roots[0] + + +def test_qml_remove_row_removes_from_model() -> None: + QGuiApplication.instance() or QGuiApplication([]) + model = EnvVarsModel() + model.reset_rows([("FOO", "1"), ("BAR", "2")]) + modified: list[bool] = [] + model.modified.connect(lambda: modified.append(True)) + + engine, root = _qml_root(model) + assert root.removeAt(0) is True + assert model.to_dict() == {"BAR": "2"} + assert modified, "removal must emit modified so the editor turns dirty" + + +def test_qml_remove_row_out_of_range_is_rejected() -> None: + QGuiApplication.instance() or QGuiApplication([]) + model = EnvVarsModel() + model.reset_rows([("FOO", "1")]) + + engine, root = _qml_root(model) + assert root.removeAt(5) is False + assert root.removeAt(-1) is False + assert model.to_dict() == {"FOO": "1"}