john/McRogueFace

Fork 0

[Bugfix] Stale last_clicked_cell can be dispatched for a GridView with no grid data #365

New issue

Closed

opened 2026-07-12 22:05:15 +00:00 by john · 1 comment

john commented

2026-07-12 22:05:15 +00:00

Owner

Found by adversarial review of the #355 branch (feat/355-grid-input-revival, 85e4bbe). Reported as minor; not fixed there.

Defect

UIGridView::click_at has an early return for the no-data case:

if (!grid_data) {
    // returns `this` when click_callable || is_python_subclass
}

It returns this without clearing last_clicked_cell. Since #355 made last_clicked_cell the channel by which click_at hands the resolved cell to dispatchCellClick (the point cannot be recomputed later — click_at works in parent-local space, PyScene only has global), a cell captured by an earlier click survives in that slot.

Consequence

If a view's grid is set to None after a click has already landed on a cell, the next click on that (now dataless) view dispatches on_cell_click with the stale cell from before. A callback fires for a grid that has no cells at all.

Narrow, but it is a wrong-data dispatch rather than a missing one, which makes it worth closing.

Fix

Clear last_clicked_cell on the !grid_data path (and audit any other early return in click_at for the same omission).

Test

Click a grid cell, set grid.grid = None, click again, assert on_cell_click does not fire (or fires with no cell).

Found by adversarial review of the #355 branch (`feat/355-grid-input-revival`, 85e4bbe). Reported as minor; not fixed there. ## Defect `UIGridView::click_at` has an early return for the no-data case: ```cpp if (!grid_data) { // returns `this` when click_callable || is_python_subclass } ``` It returns `this` **without clearing `last_clicked_cell`**. Since #355 made `last_clicked_cell` the channel by which `click_at` hands the resolved cell to `dispatchCellClick` (the point cannot be recomputed later — `click_at` works in parent-local space, `PyScene` only has global), a cell captured by an *earlier* click survives in that slot. ## Consequence If a view's `grid` is set to `None` after a click has already landed on a cell, the next click on that (now dataless) view dispatches `on_cell_click` with the stale cell from before. A callback fires for a grid that has no cells at all. Narrow, but it is a wrong-data dispatch rather than a missing one, which makes it worth closing. ## Fix Clear `last_clicked_cell` on the `!grid_data` path (and audit any other early return in `click_at` for the same omission). ## Test Click a grid cell, set `grid.grid = None`, click again, assert `on_cell_click` does not fire (or fires with no cell).

john added the

Bugfix

priority:tier2-foundation

system:grid

system:input

Tiny Feature

labels

2026-07-12 22:05:46 +00:00

john commented

2026-07-13 00:29:50 +00:00

Author

Owner

Fixed on `feat/355-grid-input-revival` (`11f8394`) — but the filed repro was wrong

The defect is real. The mechanism described in the body is not.

"Click a cell, set grid = None, click again" does not reproduce. dispatchCellClick consumes last_clicked_cell (UIGridView.cpp:1326 sets it to nullopt after reading), so a cell that was actually dispatched cannot survive to a later click. I wrote the test straight from the body and it passed against the unfixed build — which is what exposed the gap in the analysis.

The real path has two halves. click_at stashed the cell at step 3 even when it went on to return nullptr because the view had no handlers. dispatchCellClick only runs on the drawable PyScene got back, and it got nullptr — so nothing consumed that stash and the cell sat there indefinitely. Then the no-grid_data early return hands back this without clearing it.

Reproducer (verified failing pre-fix, passing post-fix):

Click a cell on a grid with no handlers at all → cell stashed, nothing consumes it.
Assign on_cell_click + on_click, then set grid_data = None.
Click the now cell-less view → on_cell_click fires with the stale cell from step 1.

Fix: stash only when the view is actually being returned as the click target, and clear on the dataless path. Regression test tests/regression/issue_365_stale_clicked_cell_test.py covers that sequence plus a guard that a click resolving to no cell leaves nothing behind. Suite 324/324.

Also worth noting for #361: the property is grid_data but the constructor kwarg is grid (UIGridView.cpp:573 vs :1482). Inconsistent, and a natural thing to reconcile when Grid/GridView/GridData get their final names.

## Fixed on `feat/355-grid-input-revival` (11f8394) — but the filed repro was wrong The defect is real. The mechanism described in the body is not. **"Click a cell, set `grid = None`, click again" does not reproduce.** `dispatchCellClick` **consumes** `last_clicked_cell` (`UIGridView.cpp:1326` sets it to `nullopt` after reading), so a cell that was actually dispatched cannot survive to a later click. I wrote the test straight from the body and it passed against the unfixed build — which is what exposed the gap in the analysis. **The real path has two halves.** `click_at` stashed the cell at step 3 *even when it went on to return `nullptr`* because the view had no handlers. `dispatchCellClick` only runs on the drawable PyScene got back, and it got `nullptr` — so nothing consumed that stash and the cell sat there indefinitely. *Then* the no-`grid_data` early return hands back `this` without clearing it. Reproducer (verified failing pre-fix, passing post-fix): 1. Click a cell on a grid with **no handlers at all** → cell stashed, nothing consumes it. 2. Assign `on_cell_click` + `on_click`, then set `grid_data = None`. 3. Click the now cell-less view → `on_cell_click` fires with the stale cell from step 1. Fix: stash only when the view is actually being returned as the click target, and clear on the dataless path. Regression test `tests/regression/issue_365_stale_clicked_cell_test.py` covers that sequence plus a guard that a click resolving to no cell leaves nothing behind. Suite 324/324. Also worth noting for #361: the property is `grid_data` but the constructor kwarg is `grid` (`UIGridView.cpp:573` vs `:1482`). Inconsistent, and a natural thing to reconcile when `Grid`/`GridView`/`GridData` get their final names.

john referenced this issue from a commit

2026-07-13 11:44:13 +00:00

fix(grid): never dispatch a stale cell from UIGridView::click_at

john closed this issue

2026-07-13 11:44:13 +00:00

john referenced this issue from a commit

2026-07-13 11:44:13 +00:00

Merge branch 'feat/355-grid-input-revival': revive Grid input, split GridData from GridView

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

john/McRogueFace#365

No description provided.

Rows
Columns

[Bugfix] Stale last_clicked_cell can be dispatched for a GridView with no grid data #365

Defect

Consequence

Fix

Test

Fixed on feat/355-grid-input-revival (11f8394) — but the filed repro was wrong

Fixed on `feat/355-grid-input-revival` (`11f8394`) — but the filed repro was wrong