fix(qt): the stale-derived-read hazard, swept across the kiosk views

KioskArtist's TypeError was found by luck, not by a method, so an independent
agent swept every on<X>Changed handler in the kiosk tree for the same class:
a handler reading a property that is DERIVED from the very thing it fired for.
Three more, all silent:

- KioskDiscover.publishNav() read `navSectionCount` from inside
  onActiveTabChanged, so it published the PREVIOUS tab's section count (and
  NaN on the creation pass). publish() clamps `index` into the count it is
  handed, so a wrong count is a focus ring parked on nothing, not a cosmetic
  slip.
- KioskMyQBZ.publishNav() had the same shape against `cards` and
  onOnCollectionsChanged, counting the previous route's document.
- KioskArtist's shelf skip-cascade read QbzKioskNav.dir inside onIndexChanged.

That last one was MY defect, not the view's: sync() wrote `index` before
`dir`, and each cxx-qt setter emits its NOTIFY synchronously, so the handler
ran while `dir` still held the previous move. The first Down of a session read
dir == 0 and refused to skip, parking the ring on a shelf of zero height; a
direction reversal skipped back the way it came.

Fixed at the source: sync() now writes every CONTEXT field (zone, columns,
count, tabs, shelves, dir, hmove, nav_active) before the two fields consumers
actually react to (index, activate_seq). The QML keeps its Qt.callLater as
well -- that covers Qt's own binding re-evaluation order, which is a different
hazard from the setter order. Two independent guards, because this class is
invisible to every static check.

The general remedy, in all four sites: compute from the source value at call
time, never read a cached derived property inside its own handler. Written up
at qml/Main.qml:233-247 from the window-geometry work.

Gate: 9 warnings = baseline, 291 tests, both audits OK, qmllint clean on every
touched file, kiosk-profile smoke clean, and the shelf skip cascade re-driven
over VNC (scratchpad/kiosk/s-01-skip-cascade.png).

cc
This commit is contained in:
vicrodh committed 2026-08-03 23:59:42 -06:00
1 parent ea26d743ab
commit e5f93d6805
5 files changed
+102 -23

No files matched your search

+14 -1
View File
@@ -341,7 +341,20 @@ Flickable {
target: QbzKioskNav
function onIndexChanged() {
shelfSlot.skipIfUnrendered()
// DEFERRED, exactly as KioskDiscover.qml:339 defers its
// twin, and that is load-bearing. skipIfUnrendered()
// reads `QbzKioskNav.dir`, which the bridge pushes in
// the SAME sync() pass as `index` but LATER in the field
// order (kiosk_nav_bridge.rs: index 2nd, dir 9th), so
// cxx-qt has already emitted indexChanged while `dir`
// still carries the PREVIOUS move. Run inline, the first
// Down of a session read dir === 0 and refused to skip —
// parking the ring on a shelf whose height is 0 — and a
// direction reversal read the old sign and skipped back
// the way it came. Same hazard, same remedy, as the note
// at Main.qml:233-247: never read a value that is still
// in flight inside the handler that fired for it.
Qt.callLater(shelfSlot.skipIfUnrendered)
Qt.callLater(shelfSlot.scrollShelfIntoView)
}
+23 -6
View File
@@ -65,11 +65,18 @@ Rectangle {
readonly property var forYouSections: root.parseSections(QbzHome.forYouSectionsJson)
// KioskDiscover.slint:146-150.
readonly property var activeSections: root.activeTab === "editorPicks"
? root.editorSections
: root.activeTab === "forYou"
? root.forYouSections
: root.homeSections
//
// A FUNCTION behind the property, because `publishNav()` is called from
// `onActiveTabChanged` and must not read the property there — see the
// note on publishNav below.
function sectionsFor(tab) {
return tab === "editorPicks"
? root.editorSections
: tab === "forYou"
? root.forYouSections
: root.homeSections
}
readonly property var activeSections: root.sectionsFor(root.activeTab)
/// Does this descriptor render a shelf? Two terms, and the For You gate.
///
@@ -95,8 +102,18 @@ Rectangle {
// that render nothing — that is precisely what the skip cascade exists for.
// The index clamp lives inside publishNav (src/kiosk_nav_qt.rs:266-274).
readonly property int navSectionCount: root.activeSections.length
// Derives the count from `activeTab` AT CALL TIME rather than reading
// `navSectionCount`, and that is load-bearing. `onActiveTabChanged` below
// is connected to the very notify that dirties `activeSections` — and
// through it `navSectionCount` — and the handler runs BEFORE the dependent
// bindings re-evaluate, so a read of `navSectionCount` from inside it
// carries the PREVIOUS tab's descriptor count (undefined on the creation
// pass, which makes `count` NaN). `publish()` CLAMPS `index` into the
// count it is handed (src/kiosk_nav_qt.rs:266-274), so a wrong count is a
// wrong focus ring, not a cosmetic slip. Same hazard, same remedy, as
// KioskArtist.qml:102-110 and the note at Main.qml:233-247.
function publishNav() {
QbzKioskNav.publishNav(3, 1, 3 + root.navSectionCount, true)
QbzKioskNav.publishNav(3, 1, 3 + root.sectionsFor(root.activeTab).length, true)
}
Component.onCompleted: root.publishNav()
onNavSectionCountChanged: root.publishNav()
+16 -5
View File
@@ -54,11 +54,11 @@ Rectangle {
// ONE guarded parse per document. A raw JSON.parse inside a binding throws
// on the pre-publish frame and takes the whole view down
// (MyQbzGridView.qml:91-93).
readonly property var doc: root.parseDoc()
function parseDoc() {
readonly property var doc: root.docFor(root.onCollections)
function docFor(collections) {
try {
return JSON.parse(root.onCollections ? QbzMyQbz.collectionsJson
: QbzMyQbz.mixtapesJson)
return JSON.parse(collections ? QbzMyQbz.collectionsJson
: QbzMyQbz.mixtapesJson)
} catch (e) {
return ({})
}
@@ -66,8 +66,19 @@ Rectangle {
readonly property var cards: root.doc.cards || []
// ---- Nav geometry publish: 2 tabs + the active list, 6 columns.
//
// Re-derives the card count from `onCollections` AT CALL TIME rather than
// reading `cards`, and that is load-bearing. `onOnCollectionsChanged`
// below is connected to the very notify that dirties `doc` — and through
// it `cards` — and the handler runs BEFORE the dependent bindings
// re-evaluate, so a read of `root.cards` from inside it counts the
// PREVIOUS route's document. `publish()` CLAMPS `index` into the count it
// is handed (src/kiosk_nav_qt.rs:266-274), so the wrong count parks the
// focus ring on the wrong row. Same hazard, same remedy, as
// KioskArtist.qml:102-110 and the note at Main.qml:233-247.
function publishNav() {
QbzKioskNav.publishNav(2, 6, 2 + root.cards.length, false)
var d = root.docFor(root.onCollections)
QbzKioskNav.publishNav(2, 6, 2 + (d.cards || []).length, false)
}
Component.onCompleted: root.publishNav()
+24 -8
View File
@@ -123,21 +123,37 @@ Rectangle {
// Nav geometry (KioskSearch.slint:103-135). The clamp Slint writes by hand
// at :122 lives inside the model's own publish (kiosk_nav_qt.rs:266-274).
// ---------------------------------------------------------------------
//
// Re-derives BOTH the tab and the row counts from `root.doc` rather than
// reading `tab` / `albums` / `tracks` / `artists` / `playlists`, and that
// is load-bearing. `onTabChanged` below fires on a document commit, and
// the five list properties are siblings of `tab` under that same document
// — a handler runs BEFORE the dependent bindings re-evaluate, so read from
// there they carry the PREVIOUS document (the measured KioskArtist.qml
// defect, commit ea26d743a, and the same remedy KioskNowPlaying.qml:213-219
// already applies to its cover dispatch). A pure tab switch republishes the
// SAME five arrays, so `navLenProbe` does not move and `onTabChanged` is
// then the ONLY publish — there is no second pass to correct it, and
// `publish()` CLAMPS `index` into whatever count it is handed
// (src/kiosk_nav_qt.rs:266-274). Reading the SOURCE document is safe: it is
// the value that changed.
function publishNav() {
var d = root.doc
var tab = d.tab || 0
var columns = 1
var count = 5
if (root.tab === 1) {
if (tab === 1) {
columns = 6
count = 5 + root.albums.length
} else if (root.tab === 2) {
count = 5 + (d.albums || []).length
} else if (tab === 2) {
columns = 1
count = 5 + root.tracks.length
} else if (root.tab === 3) {
count = 5 + (d.tracks || []).length
} else if (tab === 3) {
columns = 7
count = 5 + root.artists.length
} else if (root.tab === 4) {
count = 5 + (d.artists || []).length
} else if (tab === 4) {
columns = 6
count = 5 + root.playlists.length
count = 5 + (d.playlists || []).length
}
QbzKioskNav.publishNav(5, columns, count, false)
}
+25 -3
View File
@@ -145,20 +145,42 @@ impl QbzKioskNav {
/// Copy the model onto the qproperties. cxx-qt's setters only emit their
/// NOTIFY when the value actually changes, so calling this after every
/// mutation costs nothing for the fields that did not move.
///
/// **The ORDER is load-bearing.** Each setter emits its NOTIFY
/// synchronously, so a QML handler on one property runs while the fields
/// written after it still hold their PREVIOUS values. The two properties
/// consumers react to are `index` (focus moved) and `activate_seq` (Enter),
/// and what those handlers need to read is the CONTEXT of the move — which
/// zone, which direction, how big the space is. So every context field is
/// written FIRST and those two LAST.
///
/// This is not hypothetical: with `dir` written after `index`, the shelf
/// skip-cascade in `KioskArtist.qml` read `dir == 0` on the first Down of a
/// a session and refused to skip, parking the focus ring on a shelf of zero
/// height; a direction reversal read the old sign and skipped back the way
/// it came.
///
/// The QML side does NOT rely on this alone — the skip handlers also defer
/// through `Qt.callLater`, which additionally covers Qt's own binding
/// re-evaluation order (the hazard written up at `qml/Main.qml:233-247`).
/// Two independent guards, because this class of bug is invisible to every
/// static check and shows up as a focus ring that lands somewhere silly.
fn sync(mut self: Pin<&mut Self>) {
let m = self.model.clone();
// --- context first ---
let zone = QString::from(m.zone.as_str());
if *self.zone() != zone {
self.as_mut().set_zone(zone);
}
self.as_mut().set_index(m.index);
self.as_mut().set_columns(m.columns);
self.as_mut().set_count(m.count);
self.as_mut().set_nav_active(m.nav_active);
self.as_mut().set_tabs(m.tabs);
self.as_mut().set_shelves(m.shelves);
self.as_mut().set_hmove(m.hmove);
self.as_mut().set_dir(m.dir);
self.as_mut().set_hmove(m.hmove);
self.as_mut().set_nav_active(m.nav_active);
// --- the two consumers react to, last ---
self.as_mut().set_index(m.index);
self.as_mut().set_activate_seq(m.activate_seq);
}