From c6ad61448f0d42d89e3ccad2439820a0bbacf287 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 24 Jul 2026 19:41:48 -0700 Subject: [PATCH] Stop the launcher from listing an app more than once The apps list rebuilt its rows by writing into the maps held by the menu's items and itemOrder var properties. Writing into an object owned by a QML var property is not reliable: the same row object written into a plain JS object always lands, but written through the property it occasionally arrives with the key created and the value undefined. One write per rescan was lost, on a different app each time. A lost write left an id in itemOrder with no item behind it. The old purge only deleted app rows it could find in items, so the orphan survived the next merge, the add loop appended a second row for the same app, and the list grew by one -- permanently, and again on every later rescan. Touching a single desktop file fires around a dozen merges, because the entry model emits valuesChanged per insert and removal while it reconciles, so duplicates piled up quickly: nine YouTube rows on the reporting machine, and Alacritty doubled before that. The bookkeeping moves into MenuModel as two pure functions that build fresh maps for the caller to assign in one shot, so the fragile write disappears. They also make the merge self-healing rather than merely correct-when-nothing-is-lost: an id with no item is dropped instead of carried forward, and an id is listed once even when two desktop entries claim it -- so no single dropped write can compound into a duplicate row again. The bash-backed providers behind the font and power profile lists had the same latent bug and get the same treatment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TJQJfHXXApUk6En8EZisHg --- shell/plugins/menu/Menu.qml | 42 +++++++---------- shell/plugins/menu/MenuModel.js | 61 ++++++++++++++++++++++++ test/shell.d/menu-test.sh | 82 +++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 25 deletions(-) diff --git a/shell/plugins/menu/Menu.qml b/shell/plugins/menu/Menu.qml index ea505f9d..06861053 100644 --- a/shell/plugins/menu/Menu.qml +++ b/shell/plugins/menu/Menu.qml @@ -246,27 +246,19 @@ Item { function mergeAppRows() { if (!root.appLibrary) return - var nextOrder = [] - for (var i = 0; i < root.itemOrder.length; i++) { - var id = root.itemOrder[i] - var existing = root.items[id] - if (existing && existing.kind === "app") delete root.items[id] - else nextOrder.push(id) - } - var rows = root.appLibrary.sortedEntries("") + var appRows = [] for (var j = 0; j < rows.length; j++) { var entry = rows[j].entry var appId = String(entry.id || "") if (!appId) continue - var itemId = "apps." + appId var subtext = root.appLibrary.entrySubtext(entry) var aliases = subtext ? [subtext] : [] try { if (entry.keywords && typeof entry.keywords.join === "function") aliases = aliases.concat(entry.keywords) } catch (e) { } - root.items[itemId] = { - id: itemId, + appRows.push({ + id: "apps." + appId, parent: "apps", kind: "app", icon: "", @@ -281,12 +273,13 @@ Item { aliases: aliases, when: "", checked: "", - order: nextOrder.length - } - nextOrder.push(itemId) + order: 0 + }) } - root.itemOrder = nextOrder + var merged = MenuModel.mergeAppRows(root.items, root.itemOrder, appRows) + root.items = merged.items + root.itemOrder = merged.itemOrder if (root.opened) root.rebuildDisplay() } @@ -313,9 +306,8 @@ Item { function mergeProviderRows(rows, menuId, providerKey) { var spec = root.providers[providerKey] if (!spec) return - var changed = false var lines = String(rows || "").split("\n") - var nextOrder = root.itemOrder.slice() + var providerRows = [] for (var i = 0; i < lines.length; i++) { var line = lines[i].trim() if (!line) continue @@ -324,10 +316,8 @@ Item { var value = parts[1] || parts[0] || "" var current = parts[2] || "" if (!label) continue - var id = menuId + "." + root.slugify(value) - if (!root.items[id]) nextOrder.push(id) - root.items[id] = { - id: id, + providerRows.push({ + id: menuId + "." + root.slugify(value), parent: menuId, kind: "action", icon: (value === current) ? "✓" : (spec.icon || ""), @@ -340,11 +330,13 @@ Item { aliases: [], when: "", checked: "", - order: nextOrder.indexOf(id) - } - changed = true + order: 0 + }) } - root.itemOrder = nextOrder + var changed = providerRows.length > 0 + var merged = MenuModel.mergeRowsById(root.items, root.itemOrder, providerRows) + root.items = merged.items + root.itemOrder = merged.itemOrder if (changed && root.opened) root.rebuildDisplay() } diff --git a/shell/plugins/menu/MenuModel.js b/shell/plugins/menu/MenuModel.js index 3eb8236a..12ef3c0c 100644 --- a/shell/plugins/menu/MenuModel.js +++ b/shell/plugins/menu/MenuModel.js @@ -94,6 +94,65 @@ function mergeMenuSources(defaultItems, userItems) { } } +// Both merges below return fresh items/itemOrder objects for the caller to +// assign in one go. They must never write into the maps they are handed: those +// live in QML `var` properties, and an in-place write into such an object is +// occasionally dropped by the engine — the key lands with an undefined value. +// A lost write used to leave an id in itemOrder with no item behind it, and +// the next merge then kept that orphan and appended a second row for the same +// app, so the launcher listed it twice (and again on every later rescan). + +// Swaps every app row for the current set. Rows keep the order they arrive in; +// ids already claimed (including duplicate desktop ids) are listed once. +function mergeAppRows(items, itemOrder, appRows) { + var source = items || ({}) + var order = Array.isArray(itemOrder) ? itemOrder : [] + var rows = Array.isArray(appRows) ? appRows : [] + var nextItems = ({}) + var nextOrder = [] + + for (var i = 0; i < order.length; i++) { + var id = order[i] + var existing = source[id] + // Orphans (an id with no item) are dropped rather than carried forward, + // so a single lost write cannot compound into a duplicate row. + if (!existing || existing.kind === "app") continue + nextItems[id] = existing + nextOrder.push(id) + } + + for (var j = 0; j < rows.length; j++) { + var row = rows[j] + if (!row || !row.id || nextItems[row.id]) continue + row.order = nextOrder.length + nextItems[row.id] = row + nextOrder.push(row.id) + } + + return { items: nextItems, itemOrder: nextOrder } +} + +// Adds or replaces rows by id, leaving every other item untouched. Used by the +// bash-backed providers, which contribute rows to one submenu at a time. +function mergeRowsById(items, itemOrder, rows) { + var source = items || ({}) + var incoming = Array.isArray(rows) ? rows : [] + var nextItems = ({}) + var nextOrder = (Array.isArray(itemOrder) ? itemOrder : []).slice() + + for (var k in source) nextItems[k] = source[k] + + for (var i = 0; i < incoming.length; i++) { + var row = incoming[i] + if (!row || !row.id) continue + if (!nextItems[row.id]) nextOrder.push(row.id) + nextItems[row.id] = row + row.order = nextOrder.indexOf(row.id) + } + + return { items: nextItems, itemOrder: nextOrder } +} + function item(items, id) { return items && items[id] ? items[id] : null } @@ -282,6 +341,8 @@ if (typeof module !== "undefined") { normalizeItem: normalizeItem, parseMenuJsonc: parseMenuJsonc, mergeMenuSources: mergeMenuSources, + mergeAppRows: mergeAppRows, + mergeRowsById: mergeRowsById, item: item, slugify: slugify, depthFor: depthFor, diff --git a/test/shell.d/menu-test.sh b/test/shell.d/menu-test.sh index bf0b076d..52cedb92 100644 --- a/test/shell.d/menu-test.sh +++ b/test/shell.d/menu-test.sh @@ -206,6 +206,88 @@ assert( /function disarmPointer\(\)[\s\S]*pointerGate\.reset\(\)/.test(menuQml), 'menu resets pointer movement gate when pointer selection is disarmed' ) +// App rows are rebuilt from scratch on every desktop-entry rescan. The merge +// must be idempotent and must never carry an orphan id forward, or a single +// lost write turns into an app listed twice (and thrice, and so on). +const nonAppItems = { + root: { id: 'root', kind: 'menu', label: 'Go' }, + apps: { id: 'apps', kind: 'menu', label: 'Apps', provider: 'apps' } +} +const nonAppOrder = ['root', 'apps'] +const appRowsFor = ids => ids.map(id => ({ id: `apps.${id}`, kind: 'app', parent: 'apps', label: id, appId: id })) + +const firstMerge = menu.mergeAppRows(nonAppItems, nonAppOrder, appRowsFor(['alacritty', 'youtube'])) +assert( + firstMerge.itemOrder.join(',') === 'root,apps,apps.alacritty,apps.youtube', + 'app merge appends app rows after the static menu items' +) + +const secondMerge = menu.mergeAppRows(firstMerge.items, firstMerge.itemOrder, appRowsFor(['alacritty', 'youtube'])) +assert( + secondMerge.itemOrder.join(',') === 'root,apps,apps.alacritty,apps.youtube', + 'repeating the app merge with the same entries does not duplicate rows' +) + +assert( + menu.mergeAppRows(secondMerge.items, secondMerge.itemOrder, appRowsFor(['alacritty'])).itemOrder.join(',') + === 'root,apps,apps.alacritty', + 'app merge drops rows for entries that went away' +) + +assert( + menu.mergeAppRows(nonAppItems, nonAppOrder, appRowsFor(['youtube', 'youtube'])).itemOrder.join(',') + === 'root,apps,apps.youtube', + 'app merge lists an app once even when two desktop entries share an id' +) + +const orphanedItems = {} +for (const key in firstMerge.items) orphanedItems[key] = firstMerge.items[key] +delete orphanedItems['apps.youtube'] +const healed = menu.mergeAppRows(orphanedItems, firstMerge.itemOrder, appRowsFor(['alacritty', 'youtube'])) +assert( + healed.itemOrder.join(',') === 'root,apps,apps.alacritty,apps.youtube' + && !!healed.items['apps.youtube'], + 'app merge heals an order entry whose item went missing instead of duplicating it' +) + +assert( + !firstMerge.items['apps.youtube'].hasOwnProperty('__probe') + && (() => { + const before = Object.keys(nonAppItems).length + menu.mergeAppRows(nonAppItems, nonAppOrder, appRowsFor(['gimp'])) + return Object.keys(nonAppItems).length === before + })(), + 'app merge leaves the map it was handed untouched' +) + +const providerRowsFor = values => values.map(value => ({ id: `style.font.${value}`, kind: 'action', parent: 'style.font', label: value })) +const firstProviderMerge = menu.mergeRowsById(nonAppItems, nonAppOrder, providerRowsFor(['mono', 'serif'])) +assert( + firstProviderMerge.itemOrder.join(',') === 'root,apps,style.font.mono,style.font.serif', + 'provider merge appends its rows' +) +assert( + menu.mergeRowsById(firstProviderMerge.items, firstProviderMerge.itemOrder, providerRowsFor(['mono', 'serif'])) + .itemOrder.join(',') === 'root,apps,style.font.mono,style.font.serif', + 'repeating a provider merge does not duplicate rows' +) + +// The maps live in QML `var` properties, where an in-place write is +// occasionally dropped by the engine, so both merges must hand back fresh +// objects for the caller to assign in one shot. +assert( + /var merged = MenuModel\.mergeAppRows\(root\.items, root\.itemOrder, appRows\)\s*\n\s*root\.items = merged\.items\s*\n\s*root\.itemOrder = merged\.itemOrder/.test(menuQml), + 'menu assigns the rebuilt app item map instead of mutating it in place' +) +assert( + /var merged = MenuModel\.mergeRowsById\(root\.items, root\.itemOrder, providerRows\)\s*\n\s*root\.items = merged\.items\s*\n\s*root\.itemOrder = merged\.itemOrder/.test(menuQml), + 'menu assigns the rebuilt provider item map instead of mutating it in place' +) +assert( + !/root\.items\[[^\]]+\] =/.test(menuQml) && !/delete root\.items\[/.test(menuQml), + 'menu never writes into the item map held by the var property' +) + for (const functionName of ['openExistingMenu', 'openDmenu']) { const openMatch = menuQml.match(new RegExp(`function ${functionName}\\([^)]*\\) \\{([\\s\\S]*?)\\n \\}`)) assert(openMatch, `menu ${functionName} function exists`)