Stop the menu from opening on the previous evaluation's answers (#6601)
* Evaluate menu guards one run at a time A second evaluation starting while one was in flight could not replace it. Process ignores a command change until the next run and `running = true` is a no-op while running, so setting them did nothing -- but clearing `collected` first threw away the lines the running script had already emitted. Its tail then landed as the entire result, and every id missing from it went back to showing, since `when:` only hides a row on an explicit false. That is how Setup > Defaults > Browser ends up listing browsers that are not installed. Queue the evaluation instead and run it once the one in flight lands, the way provider enumeration already waits its turn. * Answer repeated menu guard questions once per evaluation The menu opens on the last evaluation's answers, so however long the guard batch takes is how long a row can contradict the state it describes: stop a recording and Screenrecord still offers to stop it, because the `pgrep` that would hide it is queued behind fifty package lookups. Almost none of that time is the questions, it is asking them one process at a time. The shipped menu runs `omarchy-pkg-present` 54 times and `omarchy-cmd-present` 23, and reads `omarchy-default-browser` once per row in Defaults > Browser. Prepend a prelude that answers all of it inside the one guard process, off a single package listing, bash's own PATH lookup, and one capture per reader command. The captures are eager because `checked:` reads them inside `$()`, where a lazy memo would not outlive the subshell. Takes the shipped batch from 1.49s to 0.25s with identical answers for all 175 guards. * Make the guard prelude answer exactly as the commands it stands in for The prelude only helps if it is indistinguishable from the commands it shadows, and it was not: - `pacman -Q` resolves a name through what installed packages provide, so with gvim installed it reports `vim` as present. A set built from `pacman -Qq` sees only names, so `install.editor.vim` came back and offered to install what was already there. Build the set from provides too, and send version constraints, which no set can answer, to pacman. - `omarchy-cmd-present` uses `command -v`, which finds builtins; `type -P` searches PATH alone and disagreed on every one of them. - Shadowing a reader with a function caught far more than the plain `$(reader)` the rows use: `command -v omarchy-dns` got the function name, and `VAR=x omarchy-channel-current` got an answer captured without the variable. Substitute the captured value into the expression instead and leave every other form to run the real command. - A reader that exits nonzero could take the batch down under a login shell with errexit set. Also keep the results of a batch that was killed rather than finished, since a row whose `when:` went unanswered shows, which is the failure this set of changes exists to remove. Costs 0.25s -> 0.33s against 1.49s before any of this, still with answers identical to evaluating each guard on its own. * Read every provide pacman reports, wrapped or not `pacman -Qi` wraps a long list onto indented continuation lines whenever COLUMNS is set in the environment, which the login shell the batch runs under may well have done. Reading only the line that starts with `Provides` dropped the rest: at COLUMNS=80 that is 537 of 856 provides on this machine, which puts back exactly the "offers to install what is already there" failure the provides lookup was added to prevent. Follow the continuation lines instead. The version-constraint case was also not testing what it claimed. Interpolating the argument into the shadow's script text let `bash>=1` parse as a redirection, so the shadow was handed `bash` and quietly agreed for the wrong reason -- and left an `=1` file behind, which got committed. Pass arguments as argv to both sides, drop the file, and wrap gvim's provides in the stub so the parser is held to the format pacman actually emits.
This commit is contained in:
David Heinemeier Hansson
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
// `<id>:<w|c>:<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,
|
||||
|
||||
Executable
+191
@@ -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 <id>: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 <id>: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"
|
||||
Reference in New Issue
Block a user