From 38cc27265cc40e6aa5b19aff7f0ed32a21e5cadd Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 22:44:17 -0300 Subject: [PATCH 1/7] fix(app): keep the process list scroll position across refreshes The process picker re-enumerates every 3 s, and each tick restored the previously selected row with `QTableView.selectRow`. That moves the current index, and `QAbstractItemView::currentChanged` scrolls the view to it - so once any row had been clicked, the list jumped back to that row on every tick and scrolling through a long process list was impossible. The scroll offset is now taken before the model is rebuilt and put back after the selection is restored. Rebuilding the model is not the problem on its own: Qt defers the scrollbar range update, so the offset survives it - measured at 150 before and after a 300-row rebuild with no selection, which is also why the fix has to sit after the `selectRow` call rather than around the rebuild. The regression test asserts the property the issue is about, not just the scrollbar number: after a refresh tick the restored selection must still be outside the viewport. Closes #75 --- PyMemoryEditor/app/open_process_dialog.py | 11 +++ tests/app/test_open_process_dialog.py | 109 ++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 tests/app/test_open_process_dialog.py diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index dcdc2c5..6873403 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -296,6 +296,15 @@ def _populate_processes(self) -> None: def _on_rows_ready(self, rows) -> None: selected_pid = self._selected_pid() + # Restoring the selection below re-triggers Qt's scrollTo(current), + # which yanked the list back to the selected row on every refresh tick: + # once any row had been clicked, scrolling through a long process list + # was impossible (#75). Remember where the user was and put them back + # afterwards. Rebuilding the model itself is not the problem — Qt defers + # the scrollbar range update, so the offset survives it. + scrollbar = self._table.verticalScrollBar() + scroll_offset = scrollbar.value() + self._model.setRowCount(0) for pid, name, mem, user in rows: pid_item = NumericItem(str(pid)) @@ -321,6 +330,8 @@ def _on_rows_ready(self, rows) -> None: self._table.selectRow(row) break + scrollbar.setValue(scroll_offset) + def _on_scan_finished(self, worker) -> None: """Retire the enumeration that just finished — and only that one. diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py new file mode 100644 index 0000000..3854eab --- /dev/null +++ b/tests/app/test_open_process_dialog.py @@ -0,0 +1,109 @@ +# -*- coding: utf-8 -*- + +""" +Tests for ``PyMemoryEditor/app/open_process_dialog.py``. + +Regression cover for issue #75: the picker re-enumerates processes every few +seconds, and each tick re-selected the previously picked row. Re-selecting moves +the viewport — Qt ``scrollTo``s the current index — so as soon as the user had +clicked any row, scrolling through the list snapped back to that row on every +tick. + +The contract now: a refresh may replace the rows and restore the selection, but +it must leave the user's scroll position where they put it. + +Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via +the ``app`` extra). +""" + +import os +from typing import List, Tuple + +import pytest + + +pytest.importorskip("PySide6", reason="App tests require PySide6 (install with [app] extra).") + +# Offscreen platform plugin: no display server needed, runs on CI. +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + + +@pytest.fixture(scope="module") +def qapp(): + """A single QApplication for the module (Qt allows only one per process).""" + from PySide6.QtWidgets import QApplication + + return QApplication.instance() or QApplication([]) + + +def _rows(count: int) -> List[Tuple[int, str, int, str]]: + """Rows shaped like ``_ProcessListWorker.rows_ready`` emits them.""" + return [(1000 + i, f"proc{i:03d}", 1024 * (i + 1), "user") for i in range(count)] + + +@pytest.fixture +def dialog(qapp, monkeypatch): + """A shown picker whose own enumeration is inert. + + The real worker walks the live process table on a thread, so its results + could land mid-test and overwrite the rows we feed in. The tests call + ``_on_rows_ready`` directly instead, which is exactly what a refresh tick + does once the scan returns. + """ + from PyMemoryEditor.app import open_process_dialog + + class _InertWorker(open_process_dialog._ProcessListWorker): + def run(self) -> None: + return + + monkeypatch.setattr(open_process_dialog, "_ProcessListWorker", _InertWorker) + + dlg = open_process_dialog.OpenProcessDialog() + dlg._refresh_timer.stop() # refreshes are driven by hand below + dlg.show() + qapp.processEvents() + try: + yield dlg + finally: + dlg.close() + dlg.deleteLater() + qapp.processEvents() + + +def test_refresh_keeps_the_scroll_position_after_a_row_was_clicked(qapp, dialog): + """The #75 repro: click a row near the top, scroll away, wait for a tick.""" + rows = _rows(300) + dialog._on_rows_ready(rows) + dialog._table.selectRow(2) # the single click from the report + qapp.processEvents() + + scrollbar = dialog._table.verticalScrollBar() + scrollbar.setValue(150) # the user scrolls down, away from the selection + qapp.processEvents() + assert scrollbar.value() == 150, "the list must be scrollable for this to mean anything" + + dialog._on_rows_ready(rows) # the auto-refresh tick + qapp.processEvents() + assert scrollbar.value() == 150 + + # The user-facing property behind that number: the restored selection must + # not have been dragged into view. + selected = dialog._table.selectionModel().selectedRows() + assert selected, "the tick is supposed to restore the selection" + row_rect = dialog._table.visualRect(selected[0]) + assert not row_rect.intersects(dialog._table.viewport().rect()) + + +def test_refresh_still_restores_the_selection(qapp, dialog): + """Preserving the viewport must not cost the selection the picker keeps.""" + rows = _rows(300) + dialog._on_rows_ready(rows) + dialog._table.selectRow(2) + qapp.processEvents() + + selected_pid = dialog._selected_pid() + assert selected_pid is not None + + dialog._on_rows_ready(rows) + qapp.processEvents() + assert dialog._selected_pid() == selected_pid From 7be6b8f267e652e5ddf47a15367eac80b3164ed2 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 22:52:57 -0300 Subject: [PATCH 2/7] fix(app): stop the refresh tick from overwriting the typed process name Same tick, second casualty. Restoring the selection emits `selectionChanged`, and `_on_selection_changed` mirrors the selected PID into the "Process:" field unconditionally. Click a row, type a process name, and 3 s later the field holds the old PID again - so pressing Enter opens the process that was clicked rather than the one that was typed. Measured against a live process list: the typed text survived 1.7 s. The echo is right for a click and wrong for a refresh, so the restore now goes through `_restore_selection`, which flags the selection change as programmatic; `_on_selection_changed` ignores those. Blocking the selection model's signals instead would also have suppressed the view's own repaint of the row it just selected. The clearing half of the rebuild never had the bug: it leaves nothing selected, and the handler already declines to write an empty selection into the field. --- PyMemoryEditor/app/open_process_dialog.py | 21 ++++++++++++++- tests/app/test_open_process_dialog.py | 33 ++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index 6873403..c110387 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -173,6 +173,7 @@ def __init__(self, parent: Optional[QWidget] = None) -> None: super().__init__(parent) self.process: Optional[AbstractProcess] = None self._scan_worker: Optional[_ProcessListWorker] = None + self._restoring_selection = False self.setWindowTitle("Select a Process") self.setWindowIcon(app_icon()) @@ -327,7 +328,7 @@ def _on_rows_ready(self, rows) -> None: for row in range(self._proxy.rowCount()): idx = self._proxy.index(row, self.COL_PID) if self._proxy.data(idx, Qt.UserRole) == selected_pid: - self._table.selectRow(row) + self._restore_selection(row) break scrollbar.setValue(scroll_offset) @@ -354,7 +355,25 @@ def _teardown(self) -> None: def _on_filter_changed(self, text: str) -> None: self._proxy.setFilterFixedString(text) + def _restore_selection(self, row: int) -> None: + """Re-select a row on the user's behalf, without echoing it into the entry. + + ``_on_selection_changed`` mirrors the selected PID into the "Process:" + field, which is what a click should do and what a refresh tick must not: + a user who clicked a row and then typed a process name had their text + replaced by the old PID within 3 s, so pressing Enter opened the wrong + target. + """ + self._restoring_selection = True + try: + self._table.selectRow(row) + finally: + self._restoring_selection = False + def _on_selection_changed(self, *_args) -> None: + if self._restoring_selection: + return + pid = self._selected_pid() if pid is not None: self._entry.setText(str(pid)) diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 3854eab..7346547 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -9,8 +9,11 @@ clicked any row, scrolling through the list snapped back to that row on every tick. +The same tick also echoed the restored selection's PID into the "Process:" field, +overwriting whatever the user had typed there. + The contract now: a refresh may replace the rows and restore the selection, but -it must leave the user's scroll position where they put it. +it must leave the user's scroll position and typed input where they put them. Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via the ``app`` extra). @@ -107,3 +110,31 @@ def test_refresh_still_restores_the_selection(qapp, dialog): dialog._on_rows_ready(rows) qapp.processEvents() assert dialog._selected_pid() == selected_pid + + +def test_refresh_does_not_overwrite_a_typed_process_name(qapp, dialog): + """Clicking a row fills the entry; a refresh tick must not refill it. + + Otherwise a user who clicks a row, then types a name and presses Enter more + than one tick later opens the PID they clicked instead of what they typed. + """ + rows = _rows(300) + dialog._on_rows_ready(rows) + dialog._table.selectRow(2) + qapp.processEvents() + assert dialog._entry.text(), "clicking a row is supposed to fill the entry" + + dialog._entry.setText("notepad.exe") + dialog._on_rows_ready(rows) # the auto-refresh tick + qapp.processEvents() + assert dialog._entry.text() == "notepad.exe" + + +def test_clicking_a_row_still_fills_the_entry(qapp, dialog): + """The guard above must only cover the refresh, not a real selection.""" + dialog._on_rows_ready(_rows(300)) + qapp.processEvents() + + dialog._table.selectRow(2) + qapp.processEvents() + assert dialog._entry.text() == str(dialog._selected_pid()) From 5ae6bb829d53cd93368cf2c02b533e0b6676f348 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Wed, 19 Aug 2026 00:04:24 -0300 Subject: [PATCH 3/7] fix(app): let only a real pick retarget the process picker Two ways the picker still pointed somewhere the user hadn't chosen, both found reviewing the previous commit. Double-clicking the already-selected row opened whatever the entry held. Clicking a row that is already selected emits no `selectionChanged` in single-selection mode, so nothing re-synced the entry, and `_try_open` only ever reads the entry. Before the previous commit the 3 s tick bounded that divergence by overwriting the entry; suppressing the echo made it permanent. `doubleClicked` already carries the index, so the click is now authoritative: it fills the entry with that row's PID and opens it. Typing in the Filter box retargeted the selection silently. Hiding the selected row doesn't clear the selection - Qt remaps it onto whatever row took that index - and the remap echoed a different process's PID into the entry. Measured: pick pid 1179, type a name, filter to "proc12", and the entry read 1129, a process never chosen. The remap is now refused (the selection is dropped when the filter hides it) and neither it nor the cleanup reaches the entry. The refresh-tick guard grew into `_programmatic_selection`, since the filter path needs the same suppression and the reason is identical: the entry mirrors a *pick*, and neither a tick nor a keystroke is one. --- PyMemoryEditor/app/open_process_dialog.py | 54 +++++++++++----- tests/app/test_open_process_dialog.py | 76 ++++++++++++++++++++++- 2 files changed, 113 insertions(+), 17 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index c110387..ac7f2a7 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -8,6 +8,7 @@ """ import ctypes import sys +from contextlib import contextmanager from typing import Callable, List, Optional, Tuple import psutil @@ -173,7 +174,7 @@ def __init__(self, parent: Optional[QWidget] = None) -> None: super().__init__(parent) self.process: Optional[AbstractProcess] = None self._scan_worker: Optional[_ProcessListWorker] = None - self._restoring_selection = False + self._selection_is_programmatic = False self.setWindowTitle("Select a Process") self.setWindowIcon(app_icon()) @@ -240,7 +241,7 @@ def _build_ui(self) -> None: self._table.horizontalHeader().setSectionResizeMode( self.COL_NAME, QHeaderView.Stretch ) - self._table.doubleClicked.connect(lambda _i: self._try_open()) + self._table.doubleClicked.connect(self._on_row_activated) self._table.selectionModel().selectionChanged.connect( self._on_selection_changed ) @@ -328,7 +329,8 @@ def _on_rows_ready(self, rows) -> None: for row in range(self._proxy.rowCount()): idx = self._proxy.index(row, self.COL_PID) if self._proxy.data(idx, Qt.UserRole) == selected_pid: - self._restore_selection(row) + with self._programmatic_selection(): + self._table.selectRow(row) break scrollbar.setValue(scroll_offset) @@ -353,25 +355,49 @@ def _teardown(self) -> None: self._scan_worker = None def _on_filter_changed(self, text: str) -> None: - self._proxy.setFilterFixedString(text) + selected_pid = self._selected_pid() + + # Filtering the selected row out of view doesn't clear the selection: Qt + # remaps it onto whatever row took that index, so the picker would point + # at a process the user never chose. Neither that remap nor the cleanup + # after it is a pick, so neither may reach the entry. + with self._programmatic_selection(): + self._proxy.setFilterFixedString(text) + if selected_pid is not None and self._selected_pid() != selected_pid: + self._table.selectionModel().clearSelection() + + def _on_row_activated(self, index) -> None: + """Open the row that was double-clicked, whatever the entry holds. + + Clicking a row that is already selected emits no ``selectionChanged`` + (the table is single-selection), so the entry can still hold a name the + user typed earlier. The double-click is the more specific instruction of + the two, so it wins — and goes through the entry, which keeps one open + path and shows what is being opened. + """ + pid = self._proxy.data(self._proxy.index(index.row(), self.COL_PID), Qt.UserRole) + if pid is not None: + self._entry.setText(str(pid)) + self._try_open() - def _restore_selection(self, row: int) -> None: - """Re-select a row on the user's behalf, without echoing it into the entry. + @contextmanager + def _programmatic_selection(self): + """Mark a selection change the user didn't make. ``_on_selection_changed`` mirrors the selected PID into the "Process:" - field, which is what a click should do and what a refresh tick must not: - a user who clicked a row and then typed a process name had their text - replaced by the old PID within 3 s, so pressing Enter opened the wrong - target. + field, which is what a click should do and what a refresh tick or a + filter keystroke must not: a user who clicked a row and then typed a + process name had their text replaced by a PID, so pressing Enter opened + the wrong target. """ - self._restoring_selection = True + self._selection_is_programmatic = True try: - self._table.selectRow(row) + yield finally: - self._restoring_selection = False + self._selection_is_programmatic = False def _on_selection_changed(self, *_args) -> None: - if self._restoring_selection: + if self._selection_is_programmatic: return pid = self._selected_pid() diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 7346547..55eac89 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -10,10 +10,14 @@ tick. The same tick also echoed the restored selection's PID into the "Process:" field, -overwriting whatever the user had typed there. +overwriting whatever the user had typed there. Typing in the Filter box did the +same thing by a different route: hiding the selected row makes Qt remap the +selection onto whatever row took that index, so the picker silently pointed at a +process the user never chose. -The contract now: a refresh may replace the rows and restore the selection, but -it must leave the user's scroll position and typed input where they put them. +The contract now: only a real pick — a click, or a double-click — may write to the +"Process:" field or change what the picker targets. A refresh or a filter +keystroke must leave the user's scroll position, selection and typed input alone. Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via the ``app`` extra). @@ -31,6 +35,9 @@ os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") +from PySide6.QtCore import Qt # noqa: E402 — import guarded by importorskip above + + @pytest.fixture(scope="module") def qapp(): """A single QApplication for the module (Qt allows only one per process).""" @@ -138,3 +145,66 @@ def test_clicking_a_row_still_fills_the_entry(qapp, dialog): dialog._table.selectRow(2) qapp.processEvents() assert dialog._entry.text() == str(dialog._selected_pid()) + + +def test_double_clicking_a_row_opens_that_row(qapp, dialog): + """The click is authoritative, even when the entry says something else. + + Clicking a row that is already selected emits no ``selectionChanged``, so + the entry can still hold a name typed earlier — and ``_try_open`` only reads + the entry. Without this, double-clicking the highlighted row opened whatever + had been typed instead. + """ + dialog._on_rows_ready(_rows(300)) + dialog._table.selectRow(2) + qapp.processEvents() + clicked_pid = dialog._selected_pid() + + dialog._entry.setText("notepad.exe") + opened = [] + dialog._try_open = lambda: opened.append(dialog._entry.text()) + + dialog._table.doubleClicked.emit(dialog._proxy.index(2, dialog.COL_PID)) + qapp.processEvents() + assert opened == [str(clicked_pid)] + + +def test_filtering_away_the_selection_drops_it_instead_of_retargeting(qapp, dialog): + """Qt remaps a selection whose row the filter hid; the picker must not follow.""" + dialog._on_rows_ready(_rows(300)) + dialog._table.selectRow(120) + qapp.processEvents() + picked_pid = dialog._selected_pid() + assert picked_pid is not None + + dialog._entry.setText("notepad.exe") + dialog._filter_edit.setText("proc12") # hides the picked row + qapp.processEvents() + + assert dialog._selected_pid() is None, "a hidden pick must not survive as another row" + assert dialog._entry.text() == "notepad.exe" + + # And the tick that follows must not resurrect it either. + dialog._on_rows_ready(_rows(300)) + qapp.processEvents() + assert dialog._entry.text() == "notepad.exe" + + +def test_filtering_keeps_a_selection_that_survives_the_filter(qapp, dialog): + """Dropping the selection is only right when the filter actually hides it.""" + rows = _rows(300) + dialog._on_rows_ready(rows) + + # proc129 stays visible under the "proc12" filter (proc120..proc129 match). + target_pid = 1129 + for row in range(dialog._proxy.rowCount()): + index = dialog._proxy.index(row, dialog.COL_PID) + if dialog._proxy.data(index, Qt.UserRole) == target_pid: + dialog._table.selectRow(row) + break + qapp.processEvents() + assert dialog._selected_pid() == target_pid + + dialog._filter_edit.setText("proc12") + qapp.processEvents() + assert dialog._selected_pid() == target_pid From 884533116a15053030c7f78cf9351ae64fa2dd68 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Wed, 19 Aug 2026 00:15:17 -0300 Subject: [PATCH 4/7] test(app): tighten the picker's regression cover Review pass over the three commits before this one. No behaviour changes - the fixes hold under probing (an active filter, a user-chosen sort order, offsets 0/1/mid/clamped, an exception thrown inside the guard, arrow-key picks, a double-click on a row other than the selected one), and every fix is caught by mutating it: dropping the scroll restore, clearing the selection on any filter keystroke, swallowing every echo, or giving `doubleClicked` its old index-discarding lambda back each fail the tests that cover them. Two gaps in the cover, both interactions rather than single behaviours: * Arrowing onto a row is a pick too, and the echo guard has to let it through. Nothing tested the keyboard path, so a guard that swallowed every selection change would have passed. * A tick under an active filter has to find the picked PID among the *filtered* rows. The two fixes compose there, and over-clearing on the filter side would have gone unnoticed. Also: annotate `_programmatic_selection`, the only method in the class without a return type, and move the test's `Qt` import into the test that uses it - every other PySide6 import under tests/app is function-local, and this was the one module-level exception. The double-click test uses `monkeypatch.setattr` rather than assigning over the bound method. --- PyMemoryEditor/app/open_process_dialog.py | 4 +- tests/app/test_open_process_dialog.py | 59 ++++++++++++++++++++--- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index ac7f2a7..3067111 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -9,7 +9,7 @@ import ctypes import sys from contextlib import contextmanager -from typing import Callable, List, Optional, Tuple +from typing import Callable, Iterator, List, Optional, Tuple import psutil @@ -381,7 +381,7 @@ def _on_row_activated(self, index) -> None: self._try_open() @contextmanager - def _programmatic_selection(self): + def _programmatic_selection(self) -> Iterator[None]: """Mark a selection change the user didn't make. ``_on_selection_changed`` mirrors the selected PID into the "Process:" diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 55eac89..99729a8 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -15,9 +15,10 @@ selection onto whatever row took that index, so the picker silently pointed at a process the user never chose. -The contract now: only a real pick — a click, or a double-click — may write to the -"Process:" field or change what the picker targets. A refresh or a filter -keystroke must leave the user's scroll position, selection and typed input alone. +The contract now: only a real pick — clicking a row, arrowing onto one, or +double-clicking one — may write to the "Process:" field or change what the picker +targets. A refresh tick or a filter keystroke must leave the user's scroll +position, selection and typed input alone. Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via the ``app`` extra). @@ -35,9 +36,6 @@ os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") -from PySide6.QtCore import Qt # noqa: E402 — import guarded by importorskip above - - @pytest.fixture(scope="module") def qapp(): """A single QApplication for the module (Qt allows only one per process).""" @@ -147,7 +145,7 @@ def test_clicking_a_row_still_fills_the_entry(qapp, dialog): assert dialog._entry.text() == str(dialog._selected_pid()) -def test_double_clicking_a_row_opens_that_row(qapp, dialog): +def test_double_clicking_a_row_opens_that_row(qapp, dialog, monkeypatch): """The click is authoritative, even when the entry says something else. Clicking a row that is already selected emits no ``selectionChanged``, so @@ -162,7 +160,7 @@ def test_double_clicking_a_row_opens_that_row(qapp, dialog): dialog._entry.setText("notepad.exe") opened = [] - dialog._try_open = lambda: opened.append(dialog._entry.text()) + monkeypatch.setattr(dialog, "_try_open", lambda: opened.append(dialog._entry.text())) dialog._table.doubleClicked.emit(dialog._proxy.index(2, dialog.COL_PID)) qapp.processEvents() @@ -192,6 +190,8 @@ def test_filtering_away_the_selection_drops_it_instead_of_retargeting(qapp, dial def test_filtering_keeps_a_selection_that_survives_the_filter(qapp, dialog): """Dropping the selection is only right when the filter actually hides it.""" + from PySide6.QtCore import Qt + rows = _rows(300) dialog._on_rows_ready(rows) @@ -208,3 +208,46 @@ def test_filtering_keeps_a_selection_that_survives_the_filter(qapp, dialog): dialog._filter_edit.setText("proc12") qapp.processEvents() assert dialog._selected_pid() == target_pid + + +def test_arrowing_onto_a_row_fills_the_entry(qapp, dialog): + """A keyboard pick is a pick too — the guard must not swallow it.""" + from PySide6.QtCore import Qt + from PySide6.QtTest import QTest + + dialog._on_rows_ready(_rows(300)) + dialog._table.selectRow(2) + qapp.processEvents() + before = dialog._entry.text() + + dialog._table.setFocus() + QTest.keyClick(dialog._table, Qt.Key_Down) + qapp.processEvents() + + assert dialog._entry.text() == str(dialog._selected_pid()) + assert dialog._entry.text() != before + + +def test_refresh_under_an_active_filter_keeps_the_selection(qapp, dialog): + """The two fixes have to compose: the restore walks the *filtered* rows. + + A tick that can't find the picked PID among the visible rows would silently + drop the selection. + """ + rows = _rows(300) + dialog._on_rows_ready(rows) + dialog._table.selectRow(2) + qapp.processEvents() + picked_pid = dialog._selected_pid() + assert picked_pid is not None + + # `_rows` names row i "proc{i:03d}" and gives it PID 1000 + i, so this + # filter leaves exactly the picked row visible. + dialog._filter_edit.setText(f"proc{picked_pid - 1000:03d}") + qapp.processEvents() + assert dialog._proxy.rowCount() == 1 + assert dialog._selected_pid() == picked_pid + + dialog._on_rows_ready(rows) # the auto-refresh tick + qapp.processEvents() + assert dialog._selected_pid() == picked_pid From 605fd6f95b9c88d2425208caab4adbf907761ce0 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Wed, 19 Aug 2026 00:18:02 -0300 Subject: [PATCH 5/7] style(app): trim the commentary on the picker fixes The comments that came in with the three fixes restated the code or told the story of how each bug was found - the commit messages already carry that. Kept the notes a future edit would need: why the scroll restore has to sit after `selectRow`, that Qt remaps a selection whose row the filter hid, and that re-clicking a selected row emits no `selectionChanged`. Same pass over the tests: the module docstring states the contract instead of narrating three bugs, the per-test docstrings are one line each, and the inline notes that survive are the ones tying a magic value to `_rows`. --- PyMemoryEditor/app/open_process_dialog.py | 33 ++++-------- tests/app/test_open_process_dialog.py | 61 +++++++---------------- 2 files changed, 27 insertions(+), 67 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index 3067111..cb117c9 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -298,12 +298,9 @@ def _populate_processes(self) -> None: def _on_rows_ready(self, rows) -> None: selected_pid = self._selected_pid() - # Restoring the selection below re-triggers Qt's scrollTo(current), - # which yanked the list back to the selected row on every refresh tick: - # once any row had been clicked, scrolling through a long process list - # was impossible (#75). Remember where the user was and put them back - # afterwards. Rebuilding the model itself is not the problem — Qt defers - # the scrollbar range update, so the offset survives it. + # Restoring the selection below re-triggers Qt's scrollTo(current), which + # jumped the list back to the selected row on every tick (#75). The + # rebuild is harmless on its own: Qt defers the scrollbar range update. scrollbar = self._table.verticalScrollBar() scroll_offset = scrollbar.value() @@ -357,23 +354,18 @@ def _teardown(self) -> None: def _on_filter_changed(self, text: str) -> None: selected_pid = self._selected_pid() - # Filtering the selected row out of view doesn't clear the selection: Qt - # remaps it onto whatever row took that index, so the picker would point - # at a process the user never chose. Neither that remap nor the cleanup - # after it is a pick, so neither may reach the entry. + # Hiding the selected row doesn't clear the selection: Qt remaps it onto + # whatever row took that index, retargeting the picker silently. with self._programmatic_selection(): self._proxy.setFilterFixedString(text) if selected_pid is not None and self._selected_pid() != selected_pid: self._table.selectionModel().clearSelection() def _on_row_activated(self, index) -> None: - """Open the row that was double-clicked, whatever the entry holds. + """Open the double-clicked row, whatever the entry holds. - Clicking a row that is already selected emits no ``selectionChanged`` - (the table is single-selection), so the entry can still hold a name the - user typed earlier. The double-click is the more specific instruction of - the two, so it wins — and goes through the entry, which keeps one open - path and shows what is being opened. + Clicking an already-selected row emits no ``selectionChanged``, so the + entry can still hold a name typed earlier. """ pid = self._proxy.data(self._proxy.index(index.row(), self.COL_PID), Qt.UserRole) if pid is not None: @@ -382,14 +374,7 @@ def _on_row_activated(self, index) -> None: @contextmanager def _programmatic_selection(self) -> Iterator[None]: - """Mark a selection change the user didn't make. - - ``_on_selection_changed`` mirrors the selected PID into the "Process:" - field, which is what a click should do and what a refresh tick or a - filter keystroke must not: a user who clicked a row and then typed a - process name had their text replaced by a PID, so pressing Enter opened - the wrong target. - """ + """Mark a selection change the user didn't make, so it can't reach the entry.""" self._selection_is_programmatic = True try: yield diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 99729a8..700ef6e 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -3,22 +3,14 @@ """ Tests for ``PyMemoryEditor/app/open_process_dialog.py``. -Regression cover for issue #75: the picker re-enumerates processes every few -seconds, and each tick re-selected the previously picked row. Re-selecting moves -the viewport — Qt ``scrollTo``s the current index — so as soon as the user had -clicked any row, scrolling through the list snapped back to that row on every -tick. - -The same tick also echoed the restored selection's PID into the "Process:" field, -overwriting whatever the user had typed there. Typing in the Filter box did the -same thing by a different route: hiding the selected row makes Qt remap the -selection onto whatever row took that index, so the picker silently pointed at a -process the user never chose. - -The contract now: only a real pick — clicking a row, arrowing onto one, or +Regression cover for issue #75 and its neighbours: the picker's re-enumeration +tick used to move the viewport, overwrite the "Process:" field, and — via the +Filter box — retarget the selection at a process the user never chose. + +The contract: only a real pick — clicking a row, arrowing onto one, or double-clicking one — may write to the "Process:" field or change what the picker -targets. A refresh tick or a filter keystroke must leave the user's scroll -position, selection and typed input alone. +targets. A refresh tick and a filter keystroke must leave both alone, along with +the scroll position. Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via the ``app`` extra). @@ -54,9 +46,8 @@ def dialog(qapp, monkeypatch): """A shown picker whose own enumeration is inert. The real worker walks the live process table on a thread, so its results - could land mid-test and overwrite the rows we feed in. The tests call - ``_on_rows_ready`` directly instead, which is exactly what a refresh tick - does once the scan returns. + could land mid-test; the tests call ``_on_rows_ready`` directly instead, + which is what a refresh tick does once the scan returns. """ from PyMemoryEditor.app import open_process_dialog @@ -79,14 +70,14 @@ def run(self) -> None: def test_refresh_keeps_the_scroll_position_after_a_row_was_clicked(qapp, dialog): - """The #75 repro: click a row near the top, scroll away, wait for a tick.""" + """The #75 repro: click a row, scroll away, wait for a tick.""" rows = _rows(300) dialog._on_rows_ready(rows) - dialog._table.selectRow(2) # the single click from the report + dialog._table.selectRow(2) qapp.processEvents() scrollbar = dialog._table.verticalScrollBar() - scrollbar.setValue(150) # the user scrolls down, away from the selection + scrollbar.setValue(150) qapp.processEvents() assert scrollbar.value() == 150, "the list must be scrollable for this to mean anything" @@ -94,8 +85,7 @@ def test_refresh_keeps_the_scroll_position_after_a_row_was_clicked(qapp, dialog) qapp.processEvents() assert scrollbar.value() == 150 - # The user-facing property behind that number: the restored selection must - # not have been dragged into view. + # The property behind that number: the restored selection stayed off-screen. selected = dialog._table.selectionModel().selectedRows() assert selected, "the tick is supposed to restore the selection" row_rect = dialog._table.visualRect(selected[0]) @@ -118,11 +108,7 @@ def test_refresh_still_restores_the_selection(qapp, dialog): def test_refresh_does_not_overwrite_a_typed_process_name(qapp, dialog): - """Clicking a row fills the entry; a refresh tick must not refill it. - - Otherwise a user who clicks a row, then types a name and presses Enter more - than one tick later opens the PID they clicked instead of what they typed. - """ + """A tick must not refill the entry the user typed into.""" rows = _rows(300) dialog._on_rows_ready(rows) dialog._table.selectRow(2) @@ -130,7 +116,7 @@ def test_refresh_does_not_overwrite_a_typed_process_name(qapp, dialog): assert dialog._entry.text(), "clicking a row is supposed to fill the entry" dialog._entry.setText("notepad.exe") - dialog._on_rows_ready(rows) # the auto-refresh tick + dialog._on_rows_ready(rows) qapp.processEvents() assert dialog._entry.text() == "notepad.exe" @@ -146,13 +132,7 @@ def test_clicking_a_row_still_fills_the_entry(qapp, dialog): def test_double_clicking_a_row_opens_that_row(qapp, dialog, monkeypatch): - """The click is authoritative, even when the entry says something else. - - Clicking a row that is already selected emits no ``selectionChanged``, so - the entry can still hold a name typed earlier — and ``_try_open`` only reads - the entry. Without this, double-clicking the highlighted row opened whatever - had been typed instead. - """ + """The click wins over the entry, which ``_try_open`` is all that reads.""" dialog._on_rows_ready(_rows(300)) dialog._table.selectRow(2) qapp.processEvents() @@ -229,11 +209,7 @@ def test_arrowing_onto_a_row_fills_the_entry(qapp, dialog): def test_refresh_under_an_active_filter_keeps_the_selection(qapp, dialog): - """The two fixes have to compose: the restore walks the *filtered* rows. - - A tick that can't find the picked PID among the visible rows would silently - drop the selection. - """ + """The two fixes compose: the restore walks the *filtered* rows.""" rows = _rows(300) dialog._on_rows_ready(rows) dialog._table.selectRow(2) @@ -241,8 +217,7 @@ def test_refresh_under_an_active_filter_keeps_the_selection(qapp, dialog): picked_pid = dialog._selected_pid() assert picked_pid is not None - # `_rows` names row i "proc{i:03d}" and gives it PID 1000 + i, so this - # filter leaves exactly the picked row visible. + # `_rows` pairs PID 1000 + i with the name "proc{i:03d}". dialog._filter_edit.setText(f"proc{picked_pid - 1000:03d}") qapp.processEvents() assert dialog._proxy.rowCount() == 1 From c49c66668a2348f53e390cbda542f4774cabe0be Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Wed, 19 Aug 2026 00:22:25 -0300 Subject: [PATCH 6/7] test(app): repair two docstrings the commentary trim broke `test_clicking_a_row_still_fills_the_entry` opened with "The guard above must only cover the refresh" - the explanation it pointed at was in the previous test's docstring, which the trim cut to one line, so the reference dangled. The double-click test's docstring lost its verb in the same pass and no longer parsed. The other two now say what they check instead of naming "the guard" and "the two fixes", neither of which a reader can resolve from this file. --- tests/app/test_open_process_dialog.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 700ef6e..3084524 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -122,7 +122,7 @@ def test_refresh_does_not_overwrite_a_typed_process_name(qapp, dialog): def test_clicking_a_row_still_fills_the_entry(qapp, dialog): - """The guard above must only cover the refresh, not a real selection.""" + """A click is a pick: the refresh guard must not swallow it too.""" dialog._on_rows_ready(_rows(300)) qapp.processEvents() @@ -132,7 +132,7 @@ def test_clicking_a_row_still_fills_the_entry(qapp, dialog): def test_double_clicking_a_row_opens_that_row(qapp, dialog, monkeypatch): - """The click wins over the entry, which ``_try_open`` is all that reads.""" + """The double-click wins over the entry, which is all ``_try_open`` reads.""" dialog._on_rows_ready(_rows(300)) dialog._table.selectRow(2) qapp.processEvents() @@ -191,7 +191,7 @@ def test_filtering_keeps_a_selection_that_survives_the_filter(qapp, dialog): def test_arrowing_onto_a_row_fills_the_entry(qapp, dialog): - """A keyboard pick is a pick too — the guard must not swallow it.""" + """Arrowing onto a row is a pick too, so it must reach the entry.""" from PySide6.QtCore import Qt from PySide6.QtTest import QTest @@ -209,7 +209,7 @@ def test_arrowing_onto_a_row_fills_the_entry(qapp, dialog): def test_refresh_under_an_active_filter_keeps_the_selection(qapp, dialog): - """The two fixes compose: the restore walks the *filtered* rows.""" + """A tick under a filter must find the picked PID among the *filtered* rows.""" rows = _rows(300) dialog._on_rows_ready(rows) dialog._table.selectRow(2) From 971ce7cf06013e4ab2a019a5e3e7d999490dd26f Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Wed, 19 Aug 2026 00:40:47 -0300 Subject: [PATCH 7/7] fix(app): re-aim the entry on a single click too Review follow-up. The previous commit fixed the double-click path and left the single click behind: clicking the row that is already highlighted emits no `selectionChanged`, so nothing re-synced the "Process:" field. Reproduced with real mouse events - click a row (entry becomes 1296), type a name, let a tick restore the selection, then click that same highlighted row: the entry still says `notepad.exe` while the row for 1296 is selected, and Open Process opens the name. With an emptied entry the same click leaves Open warning "Type a PID or process name first" while a row sits highlighted. `clicked` now aims the entry at the row it carries, which is the step the double-click already needed, so `_on_row_activated` reuses it. A real double-click emits `clicked` then `doubleClicked` (verified by delivering the four mouse events), so the entry is aimed on the first release and `_try_open` still runs exactly once. That also settles the other half of the review: a failed open no longer "eats" typed text, because the click that preceded it already replaced the text - aiming the picker is what a click means, not a side effect of the open attempt. --- PyMemoryEditor/app/open_process_dialog.py | 14 ++++++++++---- tests/app/test_open_process_dialog.py | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index cb117c9..0c00a9e 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -241,6 +241,7 @@ def _build_ui(self) -> None: self._table.horizontalHeader().setSectionResizeMode( self.COL_NAME, QHeaderView.Stretch ) + self._table.clicked.connect(self._point_entry_at) self._table.doubleClicked.connect(self._on_row_activated) self._table.selectionModel().selectionChanged.connect( self._on_selection_changed @@ -361,15 +362,20 @@ def _on_filter_changed(self, text: str) -> None: if selected_pid is not None and self._selected_pid() != selected_pid: self._table.selectionModel().clearSelection() - def _on_row_activated(self, index) -> None: - """Open the double-clicked row, whatever the entry holds. + def _point_entry_at(self, index) -> None: + """Aim the entry at a clicked row. - Clicking an already-selected row emits no ``selectionChanged``, so the - entry can still hold a name typed earlier. + ``selectionChanged`` doesn't fire when the row was already selected, so + without this the entry keeps whatever it held — a name typed earlier, or + nothing — while the row sits highlighted as if it were the target. """ pid = self._proxy.data(self._proxy.index(index.row(), self.COL_PID), Qt.UserRole) if pid is not None: self._entry.setText(str(pid)) + + def _on_row_activated(self, index) -> None: + """Open the double-clicked row, whatever the entry holds.""" + self._point_entry_at(index) self._try_open() @contextmanager diff --git a/tests/app/test_open_process_dialog.py b/tests/app/test_open_process_dialog.py index 3084524..c104984 100644 --- a/tests/app/test_open_process_dialog.py +++ b/tests/app/test_open_process_dialog.py @@ -147,6 +147,24 @@ def test_double_clicking_a_row_opens_that_row(qapp, dialog, monkeypatch): assert opened == [str(clicked_pid)] +def test_clicking_the_selected_row_re_aims_the_entry(qapp, dialog, monkeypatch): + """Clicking the highlighted row emits no ``selectionChanged`` — but still counts.""" + dialog._on_rows_ready(_rows(300)) + dialog._table.selectRow(2) + qapp.processEvents() + clicked_pid = dialog._selected_pid() + + dialog._entry.setText("notepad.exe") + opened = [] + monkeypatch.setattr(dialog, "_try_open", lambda: opened.append(dialog._entry.text())) + + dialog._table.clicked.emit(dialog._proxy.index(2, dialog.COL_PID)) + qapp.processEvents() + + assert dialog._entry.text() == str(clicked_pid) + assert opened == [], "a single click must not open anything" + + def test_filtering_away_the_selection_drops_it_instead_of_retargeting(qapp, dialog): """Qt remaps a selection whose row the filter hid; the picker must not follow.""" dialog._on_rows_ready(_rows(300))