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