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")"