fix(app): stop the process-list refresh from stomping on scroll position and typed input - #84
Merged
Merged
Conversation
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
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.
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.
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.
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`.
`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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#75.
The picker's 3 s auto-refresh — and, it turned out, its Filter box and its own click handling — could take the user's scroll position, their typed input, and even their choice of process. All of it lives in one small cluster of handlers in
open_process_dialog.py.1. The scroll position (the issue)
Each tick restored the previously selected row with
QTableView.selectRow. That moves the current index, andQAbstractItemView::currentChangedscrolls the view to the current index — so once the user had clicked any row, the list jumped back to that row on every tick and scrolling through a long process list was impossible.Measured offscreen with 300 rows: click row 2, scroll to offset 150, one refresh tick → offset 2.
The fix takes the scroll offset before the model is rebuilt and puts it 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
setRowCount(0)plus 300appendRows (150 before, 150 after, with no selection). That is why the restore has to sit after theselectRowcall rather than merely around the rebuild.Two alternatives were rejected:
selectionModel().select()instead, so the current index is never moved. It does preserve the scroll position, but it leaves the current index invalid after every tick, which costs keyboard navigation.scrollTo(..., PositionAtTop). More exact — restoring a rawScrollPerItemvalue is a row index, so the view drifts by one row when a process sorting above the viewport starts or exits — but the picker now matches the save/restore pattern already used inthreads_dialog.py,modules_dialog.pyandmemory_map_dialog.py. Worth revisiting for all four at once if the drift ever matters.The auto-refresh itself is kept: the issue also suggests dropping it in favour of the existing Refresh button, but with the viewport preserved it no longer interrupts anyone.
2. The typed process name
Restoring the selection emits
selectionChanged, and_on_selection_changedmirrored the selected PID into the "Process:" field unconditionally. Click a row, type a process name, and 3 s later the field held the old PID again — so pressing Enter opened the process that was clicked instead of the one that was typed. Against a live process list the typed text survived 1.7 s.This one needed the click first: typing without ever selecting a row was always safe, and the Filter box at the top was never affected, which is why it went unnoticed.
The echo is right for a click and wrong for a refresh, so the restore now runs inside
_programmatic_selection(), and_on_selection_changedignores selection changes made under it. Blocking the selection model's signals instead would also have suppressed the view's own repaint of the row it just selected.3. Clicking a row that is already highlighted
_try_openonly ever reads the entry, and clicking a row that is already selected emits noselectionChangedin single-selection mode — so nothing re-synced the entry. Fixing (2) is what made this reachable: before, the next tick overwrote the entry and papered over it.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 saysnotepad.exewhile 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.clickednow aims the entry at the row it carries, and_on_row_activated(double-click) reuses that step before opening. A real double-click emitsclickedthendoubleClicked— verified by delivering the four mouse events — so the entry is aimed on the first release and_try_openstill runs exactly once.4. The Filter box retargeting the selection
Hiding the selected row does not clear the selection: Qt remaps it onto whatever row took that index. Measured — pick pid 1179, type a name, filter to
proc12, and the entry reads1129, a process the user never chose; the next tick then cements it. The remap is now refused (the selection is dropped when the filter hides it, kept when it doesn't) and neither the remap nor that cleanup reaches the entry.Tests
New
tests/app/test_open_process_dialog.py, following the conventions intest_auto_refresh_dialog.py(module-scopedqapp, offscreen platform, the enumeration worker stubbed out so the live process table can't land mid-test). Ten tests covering: the #75 repro (asserting the restored selection stayed outside the viewport, not just the scrollbar number), the selection surviving a tick, the typed name surviving a tick, a click and an arrow-key pick both still filling the entry, a click on the already-highlighted row re-aiming it without opening anything, the double-click opening the row it carries, the filter dropping a hidden pick instead of retargeting it, the filter keeping a visible one, and a tick under an active filter finding the picked PID among the filtered rows.Each fix was mutation-checked: dropping the scroll restore, clearing the selection on any filter keystroke, swallowing every echo, un-wiring
clicked, wiringclickedto the open path, and givingdoubleClickedits old index-discarding lambda back each fail the tests that cover them.tests/appis 113 passed; mypy and flake8 are clean. The 15 failures in the full suite are pre-existing macOS permission failures, unrelated (same 15 on a clean tree).