From b743f44c5ba8020be584e3091bdb17249bdebe51 Mon Sep 17 00:00:00 2001 From: Gabriel Brown Date: Wed, 19 Aug 2026 11:59:53 -0400 Subject: [PATCH] Make changing a default application actually take effect Three bugs, all silent, all in the same feature. The write always worked. What failed was the refresh after it. That refresh is called from the mutation's own onExited handler and was guarded on `busy`, a binding over both processes -- and a binding hands back its cached value until the change notification feeding it has been delivered, which inside that handler has not happened yet. So `busy` read true, refresh returned immediately, and the page kept showing the old application with no error anywhere. Guards now read the Process objects directly, where the value is current, and a refresh is no longer blocked by the mutation that asked for it. The service also kept its own list of which roles it would accept. It stayed at seven when the helper and the page grew documents, text and archives, so choosing a PDF viewer set an error and changed nothing. It is derived from the snapshot now. And category matching never worked. DesktopEntries returns a QML list, for which Array.isArray is false, so the code stringified it into "Network,WebBrowser" and split on ";" alone -- one token matching no category. Browsers still appeared because their generic name contains "web browser" and the terms fallback carried the role by itself. Archives matched nothing at all, so that row could only ever offer the application it already had. The harness that should have caught the first bug passed while it was live: it set each role to the value it already had and asserted no error appeared, and the bug produces no error. It now changes a role to a genuinely different application, requires the service to observe the new value, and changes it back -- with the contract restoring the original from the outside however the run ends. Claude-Session: https://claude.ai/code/session_01BRvzt4H8XXLPVH5MyYdk9L --- .../modules/settings/ApplicationsPage.qml | 28 ++-- .../dot/quickshell/services/DefaultApps.qml | 35 ++++- tests/quickshell/DefaultAppsHarness.qml | 134 ++++++++++++++++++ .../quickshell/default-apps-roles-contract.sh | 119 ++++++++++++++++ 4 files changed, 301 insertions(+), 15 deletions(-) create mode 100644 tests/quickshell/DefaultAppsHarness.qml create mode 100755 tests/quickshell/default-apps-roles-contract.sh diff --git a/config/dot/quickshell/modules/settings/ApplicationsPage.qml b/config/dot/quickshell/modules/settings/ApplicationsPage.qml index d7266a5..05172c1 100644 --- a/config/dot/quickshell/modules/settings/ApplicationsPage.qml +++ b/config/dot/quickshell/modules/settings/ApplicationsPage.qml @@ -50,16 +50,26 @@ SettingsPage { } function matchesRole(entry: var, role: var): bool { - const rawCategories = Array.isArray(entry.categories) - ? entry.categories - : [String(entry.categories ?? "")]; + // DesktopEntries hands back a QML list, not a JavaScript array, so + // Array.isArray is false for it. The old code took that as "this is a + // string", stringified the list into "Network,WebBrowser" and then split + // on ";" only -- producing the single token "network,webbrowser", which + // matches no category at all. + // + // Nothing failed loudly. Browsers still appeared because their generic + // name contains "web browser", so the terms fallback carried the role + // by itself. Archives matched NOTHING, which meant that row could only + // ever offer the application it already had. + // + // Joining first and splitting on both separators handles the list form + // and a plain string equally. + const raw = entry.categories; + const joined = Array.isArray(raw) ? raw.join(";") : String(raw ?? ""); const categories = []; - for (const rawCategory of rawCategories) { - for (const value of String(rawCategory).split(";")) { - const category = value.trim().toLowerCase(); - if (category !== "") - categories.push(category); - } + for (const value of joined.split(/[;,]/)) { + const category = value.trim().toLowerCase(); + if (category !== "") + categories.push(category); } const metadata = [entry.name, entry.genericName] .map(value => String(value ?? "").toLowerCase()) diff --git a/config/dot/quickshell/services/DefaultApps.qml b/config/dot/quickshell/services/DefaultApps.qml index 02ddc55..7ff6761 100644 --- a/config/dot/quickshell/services/DefaultApps.qml +++ b/config/dot/quickshell/services/DefaultApps.qml @@ -18,9 +18,28 @@ Singleton { property var luaAutostartEntries: [] property string lastError: "" + // For the UI, which wants one answer to "is anything happening". + // + // Guards inside this file do NOT use it. `busy` is a binding, and a binding + // hands back its cached value until the change notification that feeds it + // has been delivered. Inside a process's own onExited handler that has not + // happened yet, so `busy` still reads true there -- which silently turned + // the refresh after every successful write into a no-op. The write landed + // and the settings page never noticed, which looks exactly like a settings + // page that cannot change anything. Guards below read the Process objects + // directly, where the value is current. readonly property bool busy: snapshotProcess.running || mutationProcess.running readonly property string helper: Quickshell.shellDir + "/scripts/panama-default-apps" - readonly property var supportedRoles: ["browser", "mail", "files", "terminal", "music", "images", "video"] + // Derived from the snapshot, never restated. This was a hardcoded list of + // seven, and when the helper and the page grew documents, text and archives + // it stayed at seven -- so choosing a PDF viewer set lastError and did + // nothing, which reads exactly like a settings page that does not work. + // + // The snapshot already reports one handler per role the helper supports, so + // that IS the list. An empty one means no snapshot has landed yet; the + // helper validates the role itself and reports a failure, so there is + // nothing for this guard to add before then. + readonly property var supportedRoles: Object.keys(root.handlers ?? ({})) Process { id: snapshotProcess @@ -59,7 +78,9 @@ Singleton { } function refresh(): void { - if (root.busy) + // Only a snapshot already in flight is a reason not to start another. + // A mutation finishing is the single best reason TO refresh. + if (snapshotProcess.running) return; root.lastError = ""; snapshotProcess.exec([root.helper, "snapshot"]); @@ -76,9 +97,11 @@ Singleton { } function setDefault(role: string, desktopId: string): void { - if (root.busy) + if (mutationProcess.running) return; - if (!root.supportedRoles.includes(role) || !root.knownDesktopId(desktopId)) { + const roleIsKnown = root.supportedRoles.length === 0 + || root.supportedRoles.includes(role); + if (!roleIsKnown || !root.knownDesktopId(desktopId)) { root.lastError = "Choose an application from the available list." return; } @@ -87,7 +110,7 @@ Singleton { } function setAutostart(desktopId: string, enabled: bool): void { - if (root.busy) + if (mutationProcess.running) return; const known = root.autostartEntries.some(entry => entry.id === desktopId); if (!known) { @@ -99,7 +122,7 @@ Singleton { } function addAutostart(desktopId: string): void { - if (root.busy) + if (mutationProcess.running) return; if (!root.knownDesktopId(desktopId)) { root.lastError = "Choose an installed application."; diff --git a/tests/quickshell/DefaultAppsHarness.qml b/tests/quickshell/DefaultAppsHarness.qml new file mode 100644 index 0000000..a1d32bf --- /dev/null +++ b/tests/quickshell/DefaultAppsHarness.qml @@ -0,0 +1,134 @@ +import Quickshell +import Quickshell.Io +import QtQuick + +import qs.services + +// Does a change made through the service actually stick, and does the service +// SEE that it stuck? +// +// The first version of this harness set every role to the handler it already +// had and asserted no error appeared. It passed while the feature was broken: +// the write landed, the refresh afterwards silently no-oped, and the settings +// page kept showing the old value with no error anywhere. "No error" is not +// evidence of anything -- the value has to move. +// +// So this changes one role to a genuinely different application, waits for the +// service to report the new value, and changes it back. The contract restores +// the original from the outside regardless of how this ends. +ShellRoot { + id: root + + readonly property string role: "browser" + + property string original: "" + property string target: "" + property int phase: 0 + property var log: [] + property bool done: false + + function desktopId(entry) { + const id = String(entry?.id ?? ""); + return id.endsWith(".desktop") ? id : id + ".desktop"; + } + + function isBrowser(entry) { + // DesktopEntries returns a QML list, for which Array.isArray is false; + // joining first and splitting on both separators handles either form. + const raw = entry.categories; + const joined = Array.isArray(raw) ? raw.join(";") : String(raw ?? ""); + for (const value of joined.split(/[;,]/)) { + if (value.trim().toLowerCase() === "webbrowser") + return true; + } + return false; + } + + function finish(outcome) { + if (root.done) + return; + root.done = true; + Quickshell.execDetached(["sh", "-c", + "printf '%s' " + JSON.stringify(JSON.stringify({ + outcome: outcome, + original: root.original, + target: root.target, + log: root.log, + error: String(DefaultApps.lastError) + })) + " > " + Quickshell.env("PANAMA_HARNESS_OUT")]); + Qt.callLater(() => Qt.quit()); + } + + Component.onCompleted: DefaultApps.refresh() + + Timer { + interval: 300 + repeat: true + running: !root.done + onTriggered: { + const apps = DesktopEntries.applications.values; + const handlers = DefaultApps.handlers ?? ({}); + // DesktopEntries populates asynchronously; the service checks a + // desktop id against it, so starting early fails for the wrong + // reason. + if (apps.length === 0 || Object.keys(handlers).length === 0) + return; + if (DefaultApps.busy) + return; + + const current = String(handlers[root.role] ?? ""); + + if (root.phase === 0) { + root.original = current; + const other = apps.filter(entry => root.isBrowser(entry) + && root.desktopId(entry) !== current); + if (other.length === 0) { + root.finish("skipped-no-second-browser"); + return; + } + root.target = root.desktopId(other[0]); + root.log = root.log.concat(["start at " + current]); + root.phase = 1; + DefaultApps.setDefault(root.role, root.target); + return; + } + + if (root.phase === 1) { + if (current !== root.target) { + root.log = root.log.concat(["after change the service still reports " + current]); + root.phase = 4; + // Put it back before reporting the failure. + DefaultApps.setDefault(root.role, root.original); + return; + } + root.log = root.log.concat(["service observed " + current]); + root.phase = 2; + DefaultApps.setDefault(root.role, root.original); + return; + } + + if (root.phase === 2) { + if (current !== root.original) { + root.log = root.log.concat(["restore not observed, service reports " + current]); + root.finish("restore-not-observed"); + return; + } + root.log = root.log.concat(["restored to " + current]); + root.finish("ok"); + return; + } + + if (root.phase === 4) { + root.finish("change-not-observed"); + return; + } + } + } + + // A stuck harness fails rather than hangs. + Timer { + interval: 40000 + running: true + onTriggered: root.finish("timeout") + } +} diff --git a/tests/quickshell/default-apps-roles-contract.sh b/tests/quickshell/default-apps-roles-contract.sh new file mode 100755 index 0000000..63097dc --- /dev/null +++ b/tests/quickshell/default-apps-roles-contract.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash + +# Every role the settings page offers must actually be settable. +# +# The service kept its own list of which roles it would accept. When the helper +# and the page grew documents, text and archives, that list stayed at seven -- +# so choosing a PDF viewer set an error string and changed nothing. There is no +# crash and no log line; the row simply does not update, which reads as "the +# settings app does not work". +# +# This drives the REAL service through every role the helper reports, setting +# each to the handler it already has. Nothing on the machine changes, and the +# whole path is still exercised: the QML guard, the desktop-id lookup against +# DesktopEntries, the helper, and the refresh afterwards. + +set -uo pipefail + +repo_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +harness="$repo_dir/tests/quickshell/DefaultAppsHarness.qml" +helper="$repo_dir/config/dot/quickshell/scripts/panama-default-apps" +service="$repo_dir/config/dot/quickshell/services/DefaultApps.qml" +page="$repo_dir/config/dot/quickshell/modules/settings/ApplicationsPage.qml" + +fail() { + printf 'default apps roles contract: %s\n' "$1" >&2 + exit 1 +} + +for path in "$harness" "$helper" "$service" "$page"; do + [[ -r "$path" ]] || fail "missing $path" +done + +# ── Static: the service must not keep its own copy of the role list ───────── +grep -qE 'supportedRoles:\s*\[' "$service" \ + && fail 'the service hardcodes its own role list again; derive it from the snapshot instead' + +# ── Category matching must survive a QML list ─────────────────────────────── +# DesktopEntries returns a QML list for `categories`, and Array.isArray is false +# for it. Treating that as a string yields "Network,WebBrowser", which split on +# ";" alone becomes one token matching nothing -- so roles that rely on +# categories silently offered no applications to choose from, and the row could +# only ever show what it already had. +grep -q 'String(entry.categories ?? "")' "$page" \ + && fail 'category matching stringifies a QML list again; join the list instead' +grep -qE 'split\(/\[;,\]/\)' "$page" \ + || fail 'categories are not split on both ";" and "," so a stringified list still matches nothing' + +# ── The page and the helper must agree ────────────────────────────────────── +helper_roles="$(python3 - "$helper" <<'PYTHON' +import ast, re, sys +source = open(sys.argv[1]).read() +table = ast.literal_eval(re.search(r"ROLE_TARGETS = (\{.*?\n\})", source, re.S).group(1)) +print("\n".join(sorted(table))) +PYTHON +)" || fail 'could not read the helper roles' +page_roles="$(grep -oE 'key: "[a-z]+"' "$page" | sed 's/key: "//; s/"//' | sort -u)" +missing="$(comm -23 <(printf '%s\n' "$helper_roles") <(printf '%s\n' "$page_roles") | tr '\n' ' ')" +[[ -z "${missing// }" ]] || fail "the helper supports roles the page never offers: $missing" + +# ── Live: a change made through the service must stick and be seen ────────── +command -v qs >/dev/null 2>&1 || { printf 'default apps roles contract: SKIP (no quickshell)\n'; exit 0; } +[[ -n "${WAYLAND_DISPLAY:-}" ]] || { printf 'default apps roles contract: SKIP (no Wayland session)\n'; exit 0; } +command -v jq >/dev/null 2>&1 || { printf 'default apps roles contract: SKIP (no jq)\n'; exit 0; } + +# The value this test moves, captured before anything runs and restored on the +# way out no matter how the harness ends. A test that changes someone's browser +# and leaves it changed is worse than no test. +original_browser="$("$helper" snapshot | jq -r '.handlers.browser // ""')" + +work="$(mktemp -d /tmp/panama-default-roles.XXXXXX)" +staged="$repo_dir/config/dot/quickshell/default-apps-roles-harness.qml" +cleanup() { + if [[ -n "$original_browser" ]]; then + current="$("$helper" snapshot 2>/dev/null | jq -r '.handlers.browser // ""')" + [[ "$current" == "$original_browser" ]] \ + || "$helper" set-default browser "$original_browser" >/dev/null 2>&1 + fi + rm -rf "$work" + rm -f "$staged" +} +trap cleanup EXIT + +# The harness has to run from inside the shell tree, or `import qs.services` +# does not resolve. +cp "$harness" "$staged" +out="$work/result.json" + +PANAMA_HARNESS_OUT="$out" timeout 75 qs -p "$staged" >/dev/null 2>&1 + +[[ -s "$out" ]] || fail 'the harness produced no result; the service never reported a handler list' + +outcome="$(jq -r '.outcome' "$out")" +case "$outcome" in + ok) ;; + skipped-no-second-browser) + printf 'default apps roles contract: PASS (static only; this machine has one browser)\n' + exit 0 + ;; + change-not-observed) + jq -r '.log[] | " \(.)"' "$out" >&2 + fail 'a change made through the service never reached the settings page: the write lands and the refresh afterwards does nothing, so the row keeps showing the old application' + ;; + restore-not-observed) + jq -r '.log[] | " \(.)"' "$out" >&2 + fail 'the service did not observe the value being changed back' + ;; + *) + jq -r '.log[] | " \(.)"' "$out" >&2 + fail "the harness ended as \"$outcome\"" + ;; +esac + +# And it really is back where it started. +final="$("$helper" snapshot | jq -r '.handlers.browser // ""')" +[[ "$final" == "$original_browser" ]] \ + || fail "the default browser was left as $final instead of $original_browser" + +printf 'default apps roles contract: PASS (%d roles offered; a change through the service sticks and is observed)\n' \ + "$(grep -c . <<<"$helper_roles")"