diff --git a/shell/plugins/panels/audio/Model.js b/shell/plugins/panels/audio/Model.js index 94b7cda3..03b2f8aa 100644 --- a/shell/plugins/panels/audio/Model.js +++ b/shell/plugins/panels/audio/Model.js @@ -18,8 +18,31 @@ function isAudioSource(node) { || mediaClass.indexOf("Source") !== -1 } -function listSnapshot(list) { - return list && list.slice ? list.slice() : [] +// Project a node list to the primitive rows a Repeater model can hold. A row +// holding the PwNode itself is a QObject that Qt dereferences without a null +// check while it builds the delegate, and PipeWire deletes nodes outright, so a +// graph teardown segfaults the shell. Primitives cannot dangle, which is what +// closes that race -- not the check below, since the node usually dies after the +// snapshot is taken. That is also why filtering the list here never worked. +// +// The name rides along because PipeWire reuses object ids: without it a row +// could resolve to whatever inherited its id. PwNode.name is CONSTANT, so it +// identifies the node it was taken from for as long as that node lives. +function nodeRow(node) { + if (!node) return null + var id = node.id + if (id === undefined || id === null) return null + return { id: id, name: String(node.name || "") } +} + +function rowSnapshot(list) { + var values = list && list.length ? list : [] + var rows = [] + for (var i = 0; i < values.length; i++) { + var row = nodeRow(values[i]) + if (row) rows.push(row) + } + return rows } function outputVolumeName(volume, muted) { @@ -237,7 +260,8 @@ if (typeof module !== "undefined") { module.exports = { isPlaybackStream: isPlaybackStream, isAudioSource: isAudioSource, - listSnapshot: listSnapshot, + nodeRow: nodeRow, + rowSnapshot: rowSnapshot, outputVolumeName: outputVolumeName, parseSinkAvailability: parseSinkAvailability, friendlyDeviceLabel: friendlyDeviceLabel, diff --git a/shell/plugins/panels/audio/Panel.qml b/shell/plugins/panels/audio/Panel.qml index 96047e0f..548c409b 100644 --- a/shell/plugins/panels/audio/Panel.qml +++ b/shell/plugins/panels/audio/Panel.qml @@ -100,11 +100,13 @@ Panel { return list } - // Feed Repeaters with panel-local snapshots instead of the live PipeWire - // model. PipeWire can remove nodes while Quickshell is dispatching the - // removal signal; rebuilding a Repeater from that signal path has crashed - // in Quickshell's PipeWire service. The snapshot timer lets that mutation - // settle first, and closed panels keep their repeaters detached entirely. + // Feed Repeaters with panel-local snapshots of primitives instead of the live + // PipeWire model. PipeWire can remove nodes while Quickshell is dispatching + // the removal signal, and a row still holding the PwNode is a QObject Qt + // dereferences without a null check, which segfaults the shell. Rows carry id + // and name only; every delegate resolves its own node through nodeFor(). The + // snapshot timer lets the mutation settle first, and closed panels keep their + // repeaters detached entirely. property var displayAudioSinks: [] property var displayAudioSources: [] property var displayAudioStreams: [] @@ -281,7 +283,7 @@ Panel { return } if (focusSection === "streams" && selectedIndex >= 0 && selectedIndex < displayAudioStreams.length) { - var s = displayAudioStreams[selectedIndex] + var s = nodeFor(displayAudioStreams[selectedIndex]) if (s && s.audio) s.audio.volume = Math.max(0, Math.min(1.5, s.audio.volume + delta)) } } @@ -291,18 +293,18 @@ Panel { if (focusSection === "header") { toggleAllMuted(); return } if (focusSection === "output") { if (selectedIndex === -1) { toggleOutputMute(); return } - var sink = displayAudioSinks[selectedIndex] + var sink = nodeFor(displayAudioSinks[selectedIndex]) if (sink) setDefaultSink(sink) return } if (focusSection === "input") { if (selectedIndex === -1) { toggleInputMute(); return } - var src = displayAudioSources[selectedIndex] + var src = nodeFor(displayAudioSources[selectedIndex]) if (src) setDefaultSource(src) return } if (focusSection === "streams" && selectedIndex >= 0) { - var st = displayAudioStreams[selectedIndex] + var st = nodeFor(displayAudioStreams[selectedIndex]) if (st && st.audio) st.audio.muted = !st.audio.muted } } @@ -324,15 +326,27 @@ Panel { onAudioSourcesChanged: scheduleDisplayAudioModelRefresh() onAudioStreamsChanged: scheduleDisplayAudioModelRefresh() - function listSnapshot(list) { - return Model.listSnapshot(list) + function rowSnapshot(list) { + return Model.rowSnapshot(list) + } + + // A row that outlives its node resolves to null, or to a node PipeWire recreated + // under the same id and name; every binding below guards for null, and a row + // that never held the node cannot dangle. + function nodeFor(row) { + if (!row) return null + for (var i = 0; i < nodes.length; i++) { + var n = nodes[i] + if (n && n.id === row.id && String(n.name || "") === row.name) return n + } + return null } function refreshDisplayAudioModels() { if (!opened) return - displayAudioSinks = listSnapshot(audioSinks) - displayAudioSources = listSnapshot(audioSources) - displayAudioStreams = listSnapshot(audioStreams) + displayAudioSinks = rowSnapshot(audioSinks) + displayAudioSources = rowSnapshot(audioSources) + displayAudioStreams = rowSnapshot(audioStreams) clampCursor() } @@ -559,15 +573,18 @@ Panel { // Spotify exposes its PipeWire stream as "audio-src". For generic stream // names, use the one MPRIS player not already represented by another audio // stream (e.g. Chromium, or ALSA apps like cliamp). - return Model.unmatchedMprisStreamLabel(label, mprisPlayers, displayAudioStreams) + return Model.unmatchedMprisStreamLabel(label, mprisPlayers, audioStreams) } + // These read the other streams' node properties, so they take the live list -- + // displayAudioStreams carries primitives now, and resolving each row back + // would only rebuild what audioStreams already is. function streamLabel(node) { - return Model.streamLabel(node, mprisPlayers, displayAudioStreams) + return Model.streamLabel(node, mprisPlayers, audioStreams) } function streamRepresentsPlayer(node, player) { - return Model.streamRepresentsPlayer(node, player, mprisPlayers, displayAudioStreams) + return Model.streamRepresentsPlayer(node, player, mprisPlayers, audioStreams) } implicitWidth: button.implicitWidth @@ -675,7 +692,7 @@ Panel { if (!root.cursorActive) return if (root.focusSection === "streams" && root.selectedIndex >= 0 && root.selectedIndex < root.displayAudioStreams.length) { - var s = root.displayAudioStreams[root.selectedIndex] + var s = root.nodeFor(root.displayAudioStreams[root.selectedIndex]) if (s && s.audio) s.audio.muted = !s.audio.muted } else if (root.focusSection === "input") { root.toggleInputMute() @@ -857,7 +874,7 @@ Panel { required property var modelData required property int index width: panelColumn.width - node: modelData + node: root.nodeFor(modelData) rowIndex: index } } @@ -965,7 +982,7 @@ Panel { required property var modelData required property int index width: panelColumn.width - node: modelData + node: root.nodeFor(modelData) rowIndex: index } } @@ -995,7 +1012,7 @@ Panel { required property var modelData required property int index width: panelColumn.width - node: modelData + node: root.nodeFor(modelData) rowIndex: index } } diff --git a/test/shell.d/audio-test.sh b/test/shell.d/audio-test.sh index 710835d8..c0c962cb 100644 --- a/test/shell.d/audio-test.sh +++ b/test/shell.d/audio-test.sh @@ -13,6 +13,17 @@ assert(!audio.isPlaybackStream({ isStream: false, isSink: true }), 'audio reject assert(audio.isAudioSource({ audio: {} }), 'audio detects nodes with audio as sources') assert(audio.isAudioSource({ type: 'Audio/Source' }), 'audio detects typed source nodes') +// A destroyed PwNode stays truthy but reads back no id, which is the shape the +// third entry stands in for: it must not reach a Repeater row. The first carries +// an object like a live node's audio, so a row that is the node itself fails here. +assertDeepEqual( + audio.rowSnapshot([{ id: 0, name: 'alsa_output', audio: { volume: 1 } }, { id: 42 }, {}, null]), + [{ id: 0, name: 'alsa_output' }, { id: 42, name: '' }], + 'audio projects nodes to primitive rows and drops nodes without an id' +) +assertDeepEqual(audio.rowSnapshot(undefined), [], 'audio projects a missing list to no rows') +assert(audio.nodeRow({ id: 7, name: 'bluez_output' }).name === 'bluez_output', 'audio rows carry the node name that identifies them') + assertEqual(audio.outputVolumeName(0, false), 'Silenced', 'audio labels silent output') assertEqual(audio.outputVolumeName(0.9, false), 'Party mode', 'audio labels loud output') assertEqual(audio.outputVolumeName(0.5, true), 'Muted', 'audio labels muted output')