diff --git a/shell/plugins/menu/Menu.qml b/shell/plugins/menu/Menu.qml index 715bb22c..aa984eb8 100644 --- a/shell/plugins/menu/Menu.qml +++ b/shell/plugins/menu/Menu.qml @@ -947,16 +947,22 @@ Item { property var whenResults: ({}) // id → true|false (allow visibility) property var checkedResults: ({}) // id → true|false (show ✓) + property bool guardsPending: false function evaluateGuards() { - var script = "" - var ids = Object.keys(root.items) - for (var i = 0; i < ids.length; i++) { - var entry = root.items[ids[i]] - if (!entry) continue - if (entry.when) script += "if { " + entry.when + "; } >/dev/null 2>&1; then echo " + ids[i] + ":w:1; else echo " + ids[i] + ":w:0; fi\n" - if (entry.checked) script += "if { " + entry.checked + "; } >/dev/null 2>&1; then echo " + ids[i] + ":c:1; else echo " + ids[i] + ":c:0; fi\n" + // Process ignores a command change while it is running, and `collected` + // belongs to the run in flight, so a second evaluation cannot overwrite + // the first: it would throw away the lines already read and never start. + // The surviving tail then lands as the whole answer, and every id lost + // with it goes back to showing, since a `when:` only hides on an explicit + // false. Wait for the run in flight and evaluate once it lands instead. + if (guardProc.running) { + root.guardsPending = true + return } + root.guardsPending = false + + var script = MenuModel.guardScript(root.items) if (!script) { root.whenResults = ({}) root.checkedResults = ({}) @@ -973,7 +979,16 @@ Item { stdout: SplitParser { onRead: function(data) { guardProc.collected += data + "\n" } } - onExited: { + onExited: function(exitCode, exitStatus) { + // A batch that was killed rather than finished has only told us about + // the rows it reached, and a row whose `when:` went unanswered shows. + // Keep the last complete set rather than let a half-read one through. + // A signal leaves the exit code at 0, so the status is what tells us. + if (exitCode !== 0 || exitStatus !== 0) { + if (root.guardsPending) Qt.callLater(function() { root.evaluateGuards() }) + return + } + var nextWhen = ({}) var nextChecked = ({}) var lines = guardProc.collected.split("\n") @@ -994,6 +1009,9 @@ Item { root.whenResults = nextWhen root.checkedResults = nextChecked if (root.opened) root.rebuildDisplay() + // Run the evaluation that had to stand aside. Deferred by a turn so the + // process is settled before its command is set again. + if (root.guardsPending) Qt.callLater(function() { root.evaluateGuards() }) } } PanelWindow { diff --git a/shell/plugins/menu/MenuModel.js b/shell/plugins/menu/MenuModel.js index 1908dbe0..c6dc7ed3 100644 --- a/shell/plugins/menu/MenuModel.js +++ b/shell/plugins/menu/MenuModel.js @@ -372,8 +372,115 @@ function displayRow(items, itemOrder, checkedResults, entry, detail, score, sect } } +// Commands a `checked:` expression reads a value out of. Every sibling row +// asks the same one -- Defaults > Browser has seven rows all comparing +// against `omarchy-default-browser` -- so the batch runs it once and the rows +// read the captured answer. +// +// The capture has to be eager. These are read inside `$(...)`, and a value +// cached while one expression runs lives in that subshell only, so a lazy +// memo never survives to the expression after it. +var GUARD_READERS = [ + "omarchy-channel-current", + "omarchy-default-agent", + "omarchy-default-browser", + "omarchy-default-editor", + "omarchy-default-terminal", + "omarchy-dns" +] + +// Package and command presence account for most of what the guards ask, and +// asked one at a time they are almost all fork: the shipped menu spends over +// a second on them. Answer them inside the guard process instead. These +// shadow the real commands for the batch only, so they have to agree with +// them everywhere, including for no arguments at all (present is true of +// nothing, missing is not). +// +// `pacman -Q` resolves a name through what installed packages provide, not +// just what they are called -- with gvim installed it reports `vim` as +// present -- so the set has to carry provides too, or `install.editor.vim` +// comes back and offers to install what is already there. A version +// constraint (`bash>=1`) is not a name any set can answer, so it goes to +// pacman itself; no shipped guard writes one. +// +// `pacman -Qi` wraps a long list across continuation lines whenever COLUMNS +// is set in the environment, which a login shell may well have done, so the +// parser follows the indented lines rather than reading the first one and +// dropping half of what is installed. +function guardHelpers() { + return 'declare -A __omarchy_pkgs=()\n' + + 'mapfile -t __omarchy_pkg_names < <({ pacman -Qq; LC_ALL=C pacman -Qi' + + " | awk '/^[A-Za-z]/ { provides = ($0 ~ /^Provides/); sub(/^[^:]*: /, \"\") }" + + ' provides && $0 != "None" { n = split($0, p, " ");' + + ' for (i = 1; i <= n; i++) { sub(/[<>=].*/, "", p[i]); print p[i] } }\'; } 2>/dev/null)\n' + + 'for __omarchy_pkg in "${__omarchy_pkg_names[@]}"; do __omarchy_pkgs[$__omarchy_pkg]=1; done\n' + + '__omarchy_pkg_has() { [[ -n ${__omarchy_pkgs[$1]-} ]] && return 0; ' + + '[[ $1 == *[\\<\\>=]* ]] && { pacman -Q "$1" &>/dev/null; return; }; return 1; }\n' + + 'omarchy-pkg-present() { local p; for p in "$@"; do __omarchy_pkg_has "$p" || return 1; done; return 0; }\n' + + 'omarchy-pkg-missing() { local p; for p in "$@"; do __omarchy_pkg_has "$p" || return 0; done; return 1; }\n' + + 'omarchy-cmd-present() { local c; for c in "$@"; do command -v "$c" &>/dev/null || return 1; done; return 0; }\n' + + 'omarchy-cmd-missing() { local c; for c in "$@"; do command -v "$c" &>/dev/null || return 0; done; return 1; }\n' +} + +// Substitute the captured answer into the expression rather than shadowing +// the reader with a function. `$(reader)` and the variable holding what it +// printed are interchangeable -- both strip trailing newlines, both split the +// same way unquoted -- while a function would also catch `command -v reader`, +// `VAR=x reader`, and every other form, and answer those wrong. Anything but +// the plain substitution is left alone to run the real command. +function guardPrelude(guards) { + var prelude = guardHelpers() + + for (var i = 0; i < GUARD_READERS.length; i++) { + // The guards arrive already substituted, so what marks a reader as wanted + // is the slot standing in for it, not the call it replaced. + if (guards.indexOf(guardReaderSlot(i)) < 0) continue + // `|| :` so a reader that exits nonzero cannot take the batch down with + // it under a login shell that turned on errexit. + prelude += "__omarchy_read_" + i + "=$(" + GUARD_READERS[i] + " 2>/dev/null) || :\n" + } + + return prelude +} + +function guardReaderSlot(index) { + return "${__omarchy_read_" + index + "}" +} + +function substituteGuardReaders(expression) { + for (var i = 0; i < GUARD_READERS.length; i++) + expression = expression.split("$(" + GUARD_READERS[i] + ")").join(guardReaderSlot(i)) + + return expression +} + +function guardLine(id, tag, expression) { + return "if { " + substituteGuardReaders(expression) + "; } >/dev/null 2>&1; then echo " + + id + ":" + tag + ":1; else echo " + id + ":" + tag + ":0; fi\n" +} + +// One bash script for every `when:` and `checked:` in the menu, reporting +// `::<0|1>` per line. Speed is the whole point: the menu opens on +// the last evaluation's answers, so however long this takes is how long a row +// can contradict the state it describes. +function guardScript(items) { + var guards = "" + var ids = Object.keys(items || {}) + + for (var i = 0; i < ids.length; i++) { + var entry = items[ids[i]] + if (!entry) continue + if (entry.when) guards += guardLine(ids[i], "w", entry.when) + if (entry.checked) guards += guardLine(ids[i], "c", entry.checked) + } + + return guards ? guardPrelude(guards) + guards : "" +} + if (typeof module !== "undefined") { module.exports = { + guardReaders: GUARD_READERS, + guardScript: guardScript, stripJsonc: stripJsonc, normalizeAliases: normalizeAliases, normalizeItem: normalizeItem, diff --git a/test/shell.d/menu-guards-test.sh b/test/shell.d/menu-guards-test.sh new file mode 100755 index 00000000..4062f492 --- /dev/null +++ b/test/shell.d/menu-guards-test.sh @@ -0,0 +1,191 @@ +#!/bin/bash + +set -euo pipefail + +source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh" + +run_node_test <<'JS' +const menu = requireFromRoot('shell/plugins/menu/MenuModel.js') + +const items = { + 'setup.default.browser.brave': { id: 'setup.default.browser.brave', when: 'omarchy-pkg-present brave-bin', checked: '[[ "$(omarchy-default-browser)" == "brave" ]]' }, + 'setup.default.browser.zen': { id: 'setup.default.browser.zen', when: 'omarchy-pkg-present zen-browser-bin', checked: '[[ "$(omarchy-default-browser)" == "zen" ]]' }, + 'plain': { id: 'plain', label: 'No guards' } +} +const script = menu.guardScript(items) +const browserSlot = `\${__omarchy_read_${menu.guardReaders.indexOf('omarchy-default-browser')}}` + +assert( + script.includes('if { omarchy-pkg-present brave-bin; } >/dev/null 2>&1; then echo setup.default.browser.brave:w:1; else echo setup.default.browser.brave:w:0; fi'), + 'guard script reports a when: as :w:<0|1>' +) +assert( + script.includes('then echo setup.default.browser.zen:c:1; else echo setup.default.browser.zen:c:0; fi'), + 'guard script reports a checked: as :c:<0|1>' +) +assert(!/\bplain:[wc]:/.test(script), 'guard script skips items with nothing to evaluate') +assertEqual(menu.guardScript({ plain: items.plain }), '', 'guard script is empty when no item carries a guard') + +// The cost the menu is paying is per fork, not per expression, so what makes +// the batch fast is asking each command once however many rows want it. +assertEqual( + (script.match(/^__omarchy_read_\d+=\$\(omarchy-default-browser /gm) || []).length, + 1, + 'guard script reads a value command once for the whole batch' +) +assert( + script.includes(`[[ "${browserSlot}" == "brave" ]]`) && !script.includes('"$(omarchy-default-browser)"'), + 'guard script substitutes the captured answer into the expression' +) +assert( + script.indexOf('__omarchy_read_') < script.indexOf('if { omarchy-pkg-present'), + 'guard script captures readers before any guard runs, since $() would trap a lazy memo in its subshell' +) + +// Substitution is confined to the plain `$(reader)` form on purpose. A +// function shadowing the name would also catch these, and answer them wrong. +const untouched = menu.guardScript({ + a: { id: 'a', when: 'command -v omarchy-dns' }, + b: { id: 'b', when: '[[ "$(OMARCHY_PATH=/usr/share/omarchy omarchy-channel-current)" == "stable" ]]' }, + c: { id: 'c', when: '(( $(omarchy-default-browser | wc -l) == 1 ))' } +}) +assert( + untouched.includes('command -v omarchy-dns') + && untouched.includes('$(OMARCHY_PATH=/usr/share/omarchy omarchy-channel-current)') + && untouched.includes('$(omarchy-default-browser | wc -l)'), + 'guard script leaves every form but the plain substitution to run the real command' +) +assert( + !/^__omarchy_read_/m.test(untouched), + 'guard script captures nothing when no guard uses the plain substitution' +) + +// Every reader named in the shipped menu has to be listed, or it silently +// keeps forking once per row that reads it. +const fs = require('fs') +const defaultItems = menu.parseMenuJsonc(fs.readFileSync(path.join(root, 'default/omarchy/omarchy-menu.jsonc'), 'utf8')) +const guardText = defaultItems.map(item => `${item.when}\n${item.checked}`).join('\n') +const repeated = [...new Set( + (guardText.match(/\$\((omarchy-[a-z0-9-]+)\)/g) || []).map(match => match.slice(2, -1)) +)].filter(command => guardText.split(`$(${command})`).length > 2) +assertDeepEqual( + repeated.filter(command => !menu.guardReaders.includes(command)), + [], + 'guard readers cover every command the shipped menu reads from more than one row' +) +JS + +prelude() { + node -e ' + const path = require("path") + const menu = require(path.join(process.env.ROOT, "shell/plugins/menu/MenuModel.js")) + process.stdout.write(menu.guardScript({ probe: { id: "probe", when: "true" } })) + ' | command grep -v '^if {' +} + +# The prelude shadows the real commands for the length of the batch, so it has +# to answer exactly as they do -- including for arguments no shipped guard +# passes today, which an extension is free to write tomorrow. +stub_dir=$(mktemp -d) +trap 'rm -rf "$stub_dir"' EXIT + +# `pacman -Q` resolves a name through what installed packages provide, so gvim +# answers for vim and bash answers for sh. A set built from `pacman -Qq` alone +# would miss both and offer to install what is already there. +# +# `-Qi` wraps a long list onto indented continuation lines whenever COLUMNS is +# set, so gvim's provides arrive the way a wrapped terminal would emit them. +cat >"$stub_dir/pacman" <<'STUB' +#!/bin/bash +case "$1" in +-Qq) + printf '%s\n' bash gvim + ;; +-Qi) + cat <<'INFO' +Name : bash +Provides : sh +Version : 5.3.0-1 +Name : gvim +Provides : vim=9.2.0849-1 + xxd +Version : 9.2-1 +INFO + ;; +-Q) + shift + for want in "$@"; do + case "${want%%[<>=]*}" in bash | gvim | sh | vim | xxd) ;; *) exit 1 ;; esac + done + ;; +esac +exit 0 +STUB +chmod +x "$stub_dir/pacman" +printf '#!/bin/bash\nexit 0\n' >"$stub_dir/gvim" +chmod +x "$stub_dir/gvim" + +guard_prelude=$(prelude) + +# Arguments reach both sides as argv. Interpolating them into the shadow's +# script text would let `bash>=1` parse as a redirection, so the case that +# exists to prove constraints work would quietly test `bash` instead. +assert_helper_agrees() { + local description="$1" helper="$2" + shift 2 + + local real=0 shadowed=0 + PATH="$stub_dir:$PATH" "$ROOT/bin/$helper" "$@" >/dev/null 2>&1 || real=$? + PATH="$stub_dir:$PATH" bash -c "$guard_prelude"$'\n'"$helper \"\$@\"" "$helper" "$@" >/dev/null 2>&1 || shadowed=$? + ((real == shadowed)) || fail "$description" "$helper $*: real=$real shadowed=$shadowed" +} + +# vim, sh and xxd are provided rather than installed, and xxd only appears on a +# wrapped continuation line; bash>=1 is a version constraint no set can answer. +pkg_cases=("bash" "vim" "sh" "xxd" "absent" "bash vim" "bash absent" "bash>=1" "vim>=1" "") +for helper in omarchy-pkg-present omarchy-pkg-missing; do + for case in "${pkg_cases[@]}"; do + read -r -a argv <<<"$case" + assert_helper_agrees "guard prelude resolves packages as pacman does" "$helper" "${argv[@]}" + done +done +pass "guard prelude resolves packages through provides, wrapping, and constraints as pacman does" + +# cd is a shell builtin `command -v` finds and a PATH search does not. +cmd_cases=("gvim" "cd" "absent" "gvim absent" "gvim cd" "") +for helper in omarchy-cmd-present omarchy-cmd-missing; do + for case in "${cmd_cases[@]}"; do + read -r -a argv <<<"$case" + assert_helper_agrees "guard prelude resolves commands as the real helper does" "$helper" "${argv[@]}" + done +done +pass "guard prelude resolves commands as omarchy-cmd-present and omarchy-cmd-missing do" + +# A reader is replaced by what it printed, which has to compare identically to +# the substitution it stood in for -- including the trailing newline $() drops. +reader_script=$(node -e ' + const path = require("path") + const menu = require(path.join(process.env.ROOT, "shell/plugins/menu/MenuModel.js")) + process.stdout.write(menu.guardScript({ + hit: { id: "hit", checked: "[[ \"$(omarchy-dns)\" == \"Cloudflare\" ]]" }, + miss: { id: "miss", checked: "[[ \"$(omarchy-dns)\" == \"Google\" ]]" } + })) +') +reader_result=$(bash -c ' +omarchy-dns() { printf "Cloudflare\n"; } +export -f omarchy-dns +'"$reader_script") +[[ $reader_result == $'hit:c:1\nmiss:c:0' ]] || + fail "guard prelude compares a captured reader as the substitution did" "got: $reader_result" +pass "guard prelude compares a captured reader exactly as the substitution it replaced" + +# The batch inherits whatever a login shell left set. A reader that exits +# nonzero must not take the rest of the menu's rows down with it. +errexit_result=$(bash -e -c ' +omarchy-dns() { printf "Cloudflare\n"; return 3; } +export -f omarchy-dns +'"$reader_script"' +printf "survived\n"' 2>/dev/null) +[[ $errexit_result == $'hit:c:1\nmiss:c:0\nsurvived' ]] || + fail "guard batch survives a failing reader under errexit" "got: $errexit_result" +pass "guard batch survives a reader that exits nonzero under errexit"