From 454b67d95fd63c269c5315836a098038a0c61716 Mon Sep 17 00:00:00 2001 From: silversword411 Date: Sun, 4 Oct 2026 08:09:30 -0400 Subject: [PATCH] Fix the bar startup stall and the shell restart race (#11015) * Instantiate only the current orientation's indicator tree Indicators.qml built both the horizontal Row and the vertical Column and toggled them with `visible`, so every indicator existed twice per bar, and so did every process an indicator spawns: Dictation.qml ran two `voxtype status --follow` per monitor. On a six-monitor bar that is 72 indicator instances and twelve followers for six visible icons, and each instance registers a click target and re-syncs the active-indicator model as its state resolves at startup. A Loader now instantiates the tree that matches `root.vertical`. Each tree is wrapped in an Item that keeps the stock explicit implicit-size expressions, so the root's size still follows the blocks synchronously; a bare positioner only updates its implicit size on polish, which the indicator contract test's center-hover check catches. Measured on a six-monitor, 23-widget bar (three runs each, `omarchy restart shell`): time from "Configuration Loaded" to "polkit agent registered" 19.8-20.3s -> 15.5-15.7s, quickshell CPU 29-30s -> 24-25s, voxtype followers 12 -> 6. Co-Authored-By: Claude Fable 5.1 * Coalesce plugin API resyncs and key bar object ownership by target Every WidgetButton registers itself as a bar click target when it is created. registerClickTarget replaced the clickTargets array, the change handler ran syncAllPluginBarApiObjects() inline, and that walked every plugin API times every click target times a linear scan of pluginObjectOwners in pluginObjectRecord. Startup is a few hundred registrations, so the cost is quadratic in bar size and multiplied by the number of monitors: a six-monitor, 23-widget bar spent 16-20 seconds of pegged QML thread before it was populated, and an 8-second qmlprofiler capture showed 1.36 million pluginOwnsBarObject calls and 46-71ms per registration. - pluginObjectOwners is a Map keyed by target, so pluginObjectRecord, markPluginObject, unmarkPluginObject and releasePluginObjects are O(1) per object. Nothing outside Bar.qml read the array. - The activePopout, clickTargets and layoutConfig change handlers schedule one resync per event-loop turn through Qt.callLater, the way onModuleSlotsChanged already defers prunePluginBarApis. bindPluginBarApi still syncs a brand-new API inline, and requestPluginPopout and releasePluginPopout sync the owning API inline, so a plugin never reads a stale API on its own actions. - A flush serialises the layout once and hands each API its own parsed copy instead of deep-copying it once per API per sync. - ModuleSlot's cursorShape read clickTargets for every slot on every monitor (26,700 evaluations per start). It is now gated on the slot's HoverHandler, which is the only time the cursor is over it. Measured on the same bar, launching the shell from a checkout with this and the indicators change (two runs): "Configuration Loaded" to "polkit agent registered" 19.8-20.3s -> 0.70s, bar populated 1.7s after launch (from ~30s), quickshell CPU 29-30s -> 2.0s. Co-Authored-By: Claude Fable 5.1 * Wait for the old shell to exit before restarting it omarchy-restart-shell stopped the running shell with `quickshell kill` under a five-second timeout and launched the replacement as soon as the loop ended. A six-monitor bar takes 5.4-6.0 seconds to tear down (every widget button unregisters its click target on destruction, and each unregistration re-synced every plugin API), so the client timed out while the shell was still exiting, the fresh instance's no-duplicate check saw the dying one and quit, and the user was left with no bar and "Omarchy shell did not become ready after restart". Give the kill client thirty seconds, then wait, bounded, until `quickshell list` shows no instance of the session config before launching. The readiness check also waits on a sixty-second deadline instead of twenty attempts: a large bar answers ping only after its plugins have loaded, which on the stock bar was well past the old twelve-second window. Verified three consecutive restarts against the stock shell on the six-monitor machine: each returned 0 in about six seconds with exactly one instance and no "already running" in the journal. Co-Authored-By: Claude Fable 5.1 * Say what the bar's ownership Map actually saves Qt's V4 Map (ESTable::get) finds a key by scanning its keys, so ownership lookups are not O(1). The win is that a registration no longer copies the owner array and rescans it in QML. Co-Authored-By: Codex Medium Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Fable 5.1 Co-authored-by: Omarchybot <317366263+omarchybot@users.noreply.github.com> Co-authored-by: Codex Medium --- bin/omarchy-restart-shell | 23 ++- shell/plugins/bar/Bar.qml | 87 +++++++----- shell/plugins/bar/widgets/Indicators.qml | 174 +++++++++++++---------- test/shell.d/bar-test.sh | 23 +++ 4 files changed, 195 insertions(+), 112 deletions(-) diff --git a/bin/omarchy-restart-shell b/bin/omarchy-restart-shell index 8d3d6ce9..0794d6b0 100755 --- a/bin/omarchy-restart-shell +++ b/bin/omarchy-restart-shell @@ -77,15 +77,32 @@ if notifications_ready; then fi # Each kill stops the oldest matching instance and only returns once it has -# fully exited, so the no-duplicate launch below can't race a dying shell. -while timeout 5 quickshell kill -p "$CONFIG_DIR" --any-display >/dev/null 2>&1; do :; done +# fully exited. A large bar can take longer than a few seconds to tear down, +# so the client gets time to see that through, and the wait below covers a +# client that timed out anyway: the fresh shell refuses to start next to a +# still-dying instance (-n), which is how a restart used to end with no bar. +while timeout 30 quickshell kill -p "$CONFIG_DIR" --any-display >/dev/null 2>&1; do :; done + +shell_instance_running() { + quickshell list -a -j 2>/dev/null | + jq -e --arg dir "$CONFIG_DIR/" 'any(.[]; .config_path | startswith($dir))' >/dev/null 2>&1 +} + +deadline=$((SECONDS + 30)) +while (( SECONDS < deadline )) && shell_instance_running; do + sleep 0.1 +done # Spawn from Hyprland so the shell inherits the canonical session environment, # not transient variables from a terminal, SSH connection, or development tool. hyprctl dispatch 'hl.dsp.exec_cmd("omarchy-launch-shell")' >/dev/null shell_ready=0 -for (( attempt = 0; attempt < 20; attempt++ )); do +# The shell answers once its plugins have loaded, which on a large bar is +# well past ten seconds, so wait on a deadline rather than a fixed handful +# of attempts. +deadline=$((SECONDS + 60)) +while (( SECONDS < deadline )); do if OMARCHY_PATH="$session_omarchy_path" OMARCHY_SHELL_IPC_TIMEOUT=0.5s omarchy-shell shell ping >/dev/null 2>&1; then shell_ready=1 break diff --git a/shell/plugins/bar/Bar.qml b/shell/plugins/bar/Bar.qml index e5baec22..4d5771ab 100644 --- a/shell/plugins/bar/Bar.qml +++ b/shell/plugins/bar/Bar.qml @@ -110,7 +110,11 @@ Item { property var clickTargets: [] property var moduleSlots: [] property var pluginBarApis: ({}) - property var pluginObjectOwners: [] + // target -> { target, pluginId, clickTarget, popout }. Keyed by object so a + // registration edits one entry in place instead of copying and rescanning an + // array in QML; Qt's Map still finds a key by a native linear scan. + readonly property var pluginObjectOwners: new Map() + property bool pluginBarApiSyncQueued: false Component { id: pluginBarApiComponent @@ -138,20 +142,20 @@ Item { root.syncPluginBarApiObjects(api) } - function syncPluginBarApiObjects(api) { + // Each api keeps its own detached copy of the layout. A flush serialises + // the layout once and passes the string in so it is not re-serialised for + // every plugin. + function syncPluginBarApiObjects(api, layoutSnapshot) { if (!api) return api.activePopout = root.pluginOwnsBarObject(api.pluginId, root.activePopout) ? root.activePopout : (root.activePopout ? api.foreignPopoutMarker : null) api.clickTargets = root.pluginClickTargets(api.pluginId) - api.layoutConfig = root.publicLayoutConfig() + api.layoutConfig = layoutSnapshot !== undefined + ? JSON.parse(layoutSnapshot) : root.publicLayoutConfig() } function pluginObjectRecord(target) { - for (var i = 0; i < pluginObjectOwners.length; i++) { - var record = pluginObjectOwners[i] - if (record && record.target === target) return record - } - return null + return target ? (pluginObjectOwners.get(target) || null) : null } function markPluginObject(pluginId, target, role) { @@ -159,31 +163,20 @@ Item { if (!key || !target) return false var record = root.pluginObjectRecord(target) if (record && record.pluginId !== key) return false - var next = [] - for (var i = 0; i < pluginObjectOwners.length; i++) { - var existing = pluginObjectOwners[i] - if (!existing || existing.target !== target) next.push(existing) + if (!record) { + record = { target: target, pluginId: key, clickTarget: false, popout: false } + pluginObjectOwners.set(target, record) } - var updated = record || { target: target, pluginId: key, clickTarget: false, popout: false } - updated[role] = true - next.push(updated) - pluginObjectOwners = next + record[role] = true return true } function unmarkPluginObject(pluginId, target, role) { var key = String(pluginId || "") - var next = [] - for (var i = 0; i < pluginObjectOwners.length; i++) { - var record = pluginObjectOwners[i] - if (!record || record.target !== target || record.pluginId !== key) { - next.push(record) - continue - } - record[role] = false - if (record.clickTarget || record.popout) next.push(record) - } - pluginObjectOwners = next + var record = root.pluginObjectRecord(target) + if (!record || record.pluginId !== key) return + record[role] = false + if (!record.clickTarget && !record.popout) pluginObjectOwners.delete(target) } function pluginOwnsBarObject(pluginId, target) { @@ -200,8 +193,25 @@ Item { return out } + // Every click target registration, popout change and layout change used to + // resync every plugin api synchronously. Startup alone is hundreds of + // registrations, each walking every api and every target, so coalesce them + // into one resync per event-loop turn (as onModuleSlotsChanged already does + // for prunePluginBarApis). bindPluginBarApi still syncs a brand-new api + // directly so a widget never sees an empty api on its first read. + function schedulePluginBarApiSync() { + if (pluginBarApiSyncQueued) return + pluginBarApiSyncQueued = true + Qt.callLater(root.syncAllPluginBarApiObjects) + } + function syncAllPluginBarApiObjects() { - for (var id in pluginBarApis) root.syncPluginBarApiObjects(pluginBarApis[id]) + pluginBarApiSyncQueued = false + var layoutSnapshot = JSON.stringify(root.layoutConfig || {}) + for (var id in pluginBarApis) { + var api = pluginBarApis[id] + if (api) root.syncPluginBarApiObjects(api, layoutSnapshot) + } } function registerPluginClickTarget(pluginId, target) { @@ -215,15 +225,20 @@ Item { root.unmarkPluginObject(pluginId, target, "clickTarget") } + // A plugin may read api.activePopout right after asking for its popout, so + // its own api is brought up to date inline; everyone else waits for the + // coalesced resync. function requestPluginPopout(pluginId, owner) { if (!root.markPluginObject(pluginId, owner, "popout")) return root.requestPopout(owner) + root.syncPluginBarApiObjects(pluginBarApis[String(pluginId || "")]) } function releasePluginPopout(pluginId, owner) { if (!root.pluginOwnsBarObject(pluginId, owner)) return root.releasePopout(owner) root.unmarkPluginObject(pluginId, owner, "popout") + root.syncPluginBarApiObjects(pluginBarApis[String(pluginId || "")]) } function pluginBarApiFor(pluginId, moduleName, registered) { @@ -287,16 +302,14 @@ Item { } function releasePluginObjects(pluginId) { - var owned = pluginObjectOwners.slice() + var owned = Array.from(pluginObjectOwners.values()) for (var i = 0; i < owned.length; i++) { var record = owned[i] if (!record || record.pluginId !== pluginId) continue if (record.clickTarget) root.unregisterClickTarget(record.target) if (record.popout && root.activePopout === record.target) root.releasePopout(record.target) + pluginObjectOwners.delete(record.target) } - pluginObjectOwners = pluginObjectOwners.filter(function(record) { - return record && record.pluginId !== pluginId - }) } function prunePluginBarApis() { @@ -313,9 +326,9 @@ Item { pluginBarApis = next } - onActivePopoutChanged: syncAllPluginBarApiObjects() - onClickTargetsChanged: syncAllPluginBarApiObjects() - onLayoutConfigChanged: syncAllPluginBarApiObjects() + onActivePopoutChanged: schedulePluginBarApiSync() + onClickTargetsChanged: schedulePluginBarApiSync() + onLayoutConfigChanged: schedulePluginBarApiSync() onModuleSlotsChanged: Qt.callLater(prunePluginBarApis) Component.onDestruction: { @@ -1914,7 +1927,9 @@ Item { acceptedButtons: Qt.LeftButton enabled: slot.visible && slot.width > 0 && slot.height > 0 propagateComposedEvents: true - cursorShape: root.moduleClickTargetAt(slot, mouseX, mouseY) ? Qt.PointingHandCursor : Qt.ArrowCursor + // Only the hovered slot needs the hit test; without the guard every + // slot on every monitor re-ran it on each click target change. + cursorShape: moduleHover.hovered && root.moduleClickTargetAt(slot, mouseX, mouseY) ? Qt.PointingHandCursor : Qt.ArrowCursor // Do not assign drag.target here: ModuleSlot is owned by Row/Column // positioners, and mutating slot.x/slot.y can leave stale offsets that // make neighboring modules overlap after a small aborted drag. diff --git a/shell/plugins/bar/widgets/Indicators.qml b/shell/plugins/bar/widgets/Indicators.qml index 00417eed..c4fd9224 100644 --- a/shell/plugins/bar/widgets/Indicators.qml +++ b/shell/plugins/bar/widgets/Indicators.qml @@ -158,12 +158,12 @@ BarWidget { onIndicatorEntriesChanged: syncActiveIndicatorOrder() - implicitWidth: root.vertical - ? Math.max(activeVerticalBlock.implicitWidth, inactiveVerticalArea.implicitWidth) - : activeHorizontalBlock.implicitWidth + inactiveHorizontalArea.implicitWidth - implicitHeight: root.vertical - ? activeVerticalBlock.implicitHeight + inactiveVerticalArea.implicitHeight - : Math.max(activeHorizontalBlock.implicitHeight, inactiveHorizontalArea.implicitHeight) + // Only the tree for the current orientation exists (see orientationLoader + // below); its wrapper keeps the stock sizing expressions, so the root's + // implicit size still follows the blocks synchronously rather than waiting + // on a positioner's polish. + implicitWidth: orientationLoader.item ? orientationLoader.item.implicitWidth : 0 + implicitHeight: orientationLoader.item ? orientationLoader.item.implicitHeight : 0 ShellIpc { target: "omarchy.indicators" @@ -184,89 +184,117 @@ BarWidget { Component.onCompleted: root.refreshRequested() - Row { - id: horizontalIndicators + // Instantiating both orientation trees and toggling `visible` doubled every + // indicator per bar (and every process an indicator spawns, once per + // monitor). Load only the tree that matches the bar orientation instead. + Loader { + id: orientationLoader + sourceComponent: root.vertical ? verticalIndicatorsTree : horizontalIndicatorsTree + } - visible: !root.vertical - spacing: 0 - - HoverHandler { - onHoveredChanged: root.setIndicatorAreaHovered(hovered) - } + Component { + id: horizontalIndicatorsTree Item { - id: inactiveHorizontalArea - - implicitWidth: root.revealInactiveIndicators ? inactiveHorizontalBlock.implicitWidth : 0 - implicitHeight: Math.max(inactiveHorizontalBlock.implicitHeight, root.barSize) + implicitWidth: activeHorizontalBlock.implicitWidth + inactiveHorizontalArea.implicitWidth + implicitHeight: Math.max(activeHorizontalBlock.implicitHeight, inactiveHorizontalArea.implicitHeight) width: implicitWidth height: implicitHeight - clip: true - IndicatorBlock { - id: inactiveHorizontalBlock - anchors.verticalCenter: parent.verticalCenter - indicatorsModule: root - indicatorEntries: root.indicatorEntries - indicatorBlock: "inactive" - horizontal: true - reportActiveState: !root.vertical + Row { + id: horizontalIndicators + + spacing: 0 + + HoverHandler { + onHoveredChanged: root.setIndicatorAreaHovered(hovered) + } + + Item { + id: inactiveHorizontalArea + + implicitWidth: root.revealInactiveIndicators ? inactiveHorizontalBlock.implicitWidth : 0 + implicitHeight: Math.max(inactiveHorizontalBlock.implicitHeight, root.barSize) + width: implicitWidth + height: implicitHeight + clip: true + + IndicatorBlock { + id: inactiveHorizontalBlock + anchors.verticalCenter: parent.verticalCenter + indicatorsModule: root + indicatorEntries: root.indicatorEntries + indicatorBlock: "inactive" + horizontal: true + reportActiveState: !root.vertical + } + + HoverHandler { + onHoveredChanged: root.setIndicatorAreaHovered(hovered) + } + } + + ActiveIndicatorBlock { + id: activeHorizontalBlock + indicatorsModule: root + indicatorModel: activeIndicatorModel + horizontal: true + reportActiveState: !root.vertical + } } - - HoverHandler { - onHoveredChanged: root.setIndicatorAreaHovered(hovered) - } - } - - ActiveIndicatorBlock { - id: activeHorizontalBlock - indicatorsModule: root - indicatorModel: activeIndicatorModel - horizontal: true - reportActiveState: !root.vertical } } - Column { - id: verticalIndicators - - visible: root.vertical - spacing: 0 - - HoverHandler { - onHoveredChanged: root.setIndicatorAreaHovered(hovered) - } + Component { + id: verticalIndicatorsTree Item { - id: inactiveVerticalArea - - implicitWidth: Math.max(inactiveVerticalBlock.implicitWidth, root.barSize) - implicitHeight: root.revealInactiveIndicators ? inactiveVerticalBlock.implicitHeight : 0 + implicitWidth: Math.max(activeVerticalBlock.implicitWidth, inactiveVerticalArea.implicitWidth) + implicitHeight: activeVerticalBlock.implicitHeight + inactiveVerticalArea.implicitHeight width: implicitWidth height: implicitHeight - clip: true - IndicatorBlock { - id: inactiveVerticalBlock - anchors.horizontalCenter: parent.horizontalCenter - indicatorsModule: root - indicatorEntries: root.indicatorEntries - indicatorBlock: "inactive" - horizontal: false - reportActiveState: root.vertical + Column { + id: verticalIndicators + + spacing: 0 + + HoverHandler { + onHoveredChanged: root.setIndicatorAreaHovered(hovered) + } + + Item { + id: inactiveVerticalArea + + implicitWidth: Math.max(inactiveVerticalBlock.implicitWidth, root.barSize) + implicitHeight: root.revealInactiveIndicators ? inactiveVerticalBlock.implicitHeight : 0 + width: implicitWidth + height: implicitHeight + clip: true + + IndicatorBlock { + id: inactiveVerticalBlock + anchors.horizontalCenter: parent.horizontalCenter + indicatorsModule: root + indicatorEntries: root.indicatorEntries + indicatorBlock: "inactive" + horizontal: false + reportActiveState: root.vertical + } + + HoverHandler { + onHoveredChanged: root.setIndicatorAreaHovered(hovered) + } + } + + ActiveIndicatorBlock { + id: activeVerticalBlock + indicatorsModule: root + indicatorModel: activeIndicatorModel + horizontal: false + reportActiveState: root.vertical + } } - - HoverHandler { - onHoveredChanged: root.setIndicatorAreaHovered(hovered) - } - } - - ActiveIndicatorBlock { - id: activeVerticalBlock - indicatorsModule: root - indicatorModel: activeIndicatorModel - horizontal: false - reportActiveState: root.vertical } } diff --git a/test/shell.d/bar-test.sh b/test/shell.d/bar-test.sh index 0b06258a..7820fcca 100644 --- a/test/shell.d/bar-test.sh +++ b/test/shell.d/bar-test.sh @@ -30,6 +30,29 @@ if ! perl -0ne 'exit(/onPressAndHold:\s*function[^{]*\{[^}]*?\bpressed\b[^}]*?\b fi pass "bar move ignores a press-and-hold propagated from a widget above" +# Every click target registration used to resync every plugin api inline. With +# six monitors' worth of widgets that is hundreds of full walks at startup, so +# the change handlers coalesce into one deferred resync and ownership lookups +# are keyed by target instead of scanning an array. +if ! rg -q 'onClickTargetsChanged: schedulePluginBarApiSync\(\)' "$ROOT/shell/plugins/bar/Bar.qml"; then + fail "bar coalesces plugin api resyncs instead of running one per click target change" +fi +if ! rg -q 'Qt\.callLater\(root\.syncAllPluginBarApiObjects\)' "$ROOT/shell/plugins/bar/Bar.qml"; then + fail "bar defers the coalesced plugin api resync to the event loop" +fi +if ! rg -q 'pluginObjectOwners\.get\(target\)' "$ROOT/shell/plugins/bar/Bar.qml"; then + fail "bar looks plugin object ownership up by target instead of scanning" +fi +pass "bar coalesces plugin api resyncs and looks ownership up by target" + +# Both bar orientations used to be instantiated and toggled with `visible`, +# doubling every indicator (and every process an indicator spawns) per bar. +if ! rg -q 'sourceComponent: root\.vertical \? verticalIndicatorsTree : horizontalIndicatorsTree' \ + "$ROOT/shell/plugins/bar/widgets/Indicators.qml"; then + fail "indicators instantiate only the tree for the current bar orientation" +fi +pass "indicators instantiate only the tree for the current bar orientation" + run_node_test <<'JS' const fs = require('fs') const bar = requireFromRoot('shell/plugins/bar/BarModel.js')