fix(app): stop the process-list refresh from stomping on scroll position and typed input

PyMemoryEditor/app/open_process_dialog.py

@@ -8,7 +8,8 @@
 """
 import ctypes
 import sys
-from typing import Callable, List, Optional, Tuple
+from contextlib import contextmanager
+from typing import Callable, Iterator, List, Optional, Tuple
 
 import psutil
 
@@ -173,6 +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._selection_is_programmatic = False
 
         self.setWindowTitle("Select a Process")
         self.setWindowIcon(app_icon())
@@ -239,7 +241,8 @@ 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.clicked.connect(self._point_entry_at)
+        self._table.doubleClicked.connect(self._on_row_activated)
         self._table.selectionModel().selectionChanged.connect(
             self._on_selection_changed
         )
@@ -296,6 +299,12 @@ 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
+        # 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()
+
         self._model.setRowCount(0)
         for pid, name, mem, user in rows:
             pid_item = NumericItem(str(pid))
@@ -318,9 +327,12 @@ 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)
+                    with self._programmatic_selection():
+                        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.
 
@@ -341,9 +353,44 @@ 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()
+
+        # 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 _point_entry_at(self, index) -> None:
+        """Aim the entry at a clicked row.
+
+        ``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
+    def _programmatic_selection(self) -> Iterator[None]:
+        """Mark a selection change the user didn't make, so it can't reach the entry."""
+        self._selection_is_programmatic = True
+        try:
+            yield
+        finally:
+            self._selection_is_programmatic = False
 
     def _on_selection_changed(self, *_args) -> None:
+        if self._selection_is_programmatic:
+            return
+
         pid = self._selected_pid()
         if pid is not None:
             self._entry.setText(str(pid))

tests/app/test_open_process_dialog.py

@@ -0,0 +1,246 @@
+# -*- coding: utf-8 -*-
+
+"""
+Tests for ``PyMemoryEditor/app/open_process_dialog.py``.
+
+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 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).
+"""
+
+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; 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
+
+    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, scroll away, wait for a tick."""
+    rows = _rows(300)
+    dialog._on_rows_ready(rows)
+    dialog._table.selectRow(2)
+    qapp.processEvents()
+
+    scrollbar = dialog._table.verticalScrollBar()
+    scrollbar.setValue(150)
+    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 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])
+    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
+
+
+def test_refresh_does_not_overwrite_a_typed_process_name(qapp, dialog):
+    """A tick must not refill the entry the user typed into."""
+    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)
+    qapp.processEvents()
+    assert dialog._entry.text() == "notepad.exe"
+
+
+def test_clicking_a_row_still_fills_the_entry(qapp, dialog):
+    """A click is a pick: the refresh guard must not swallow it too."""
+    dialog._on_rows_ready(_rows(300))
+    qapp.processEvents()
+
+    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, monkeypatch):
+    """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()
+    clicked_pid = dialog._selected_pid()
+
+    dialog._entry.setText("notepad.exe")
+    opened = []
+    monkeypatch.setattr(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_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))
+    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."""
+    from PySide6.QtCore import Qt
+
+    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
+
+
+def test_arrowing_onto_a_row_fills_the_entry(qapp, dialog):
+    """Arrowing onto a row is a pick too, so it must reach the entry."""
+    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):
+    """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)
+    qapp.processEvents()
+    picked_pid = dialog._selected_pid()
+    assert picked_pid is not None
+
+    # `_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
+    assert dialog._selected_pid() == picked_pid
+
+    dialog._on_rows_ready(rows)  # the auto-refresh tick
+    qapp.processEvents()
+    assert dialog._selected_pid() == picked_pid