Merge pull request #14117 from keylimesoda/fix/image-picker-large-collections

Keep the image picker responsive with large theme collections

test/shell.d/image-picker-test.sh

@@ -42,7 +42,83 @@ assertEqual(picker.filteredPosition(images, 2, 'dark'), 1, 'image picker compute
 assertEqual(picker.selectedFilteredPosition(images, 2, 'dark'), 0, 'image picker selected filtered position falls back when selected is hidden')
 assertEqual(picker.nextSelectedIndexForFilter(images, 0, 'dark'), 1, 'image picker moves selection to first match when filter hides current item')
 
+class WindowModel {
+  constructor() { this.items = []; this.insertions = 0 }
+  get count() { return this.items.length }
+  get(i) { return this.items[i] }
+  remove(i) { this.items.splice(i, 1) }
+  insert(i, item) { this.items.splice(i, 0, { ...item }); this.insertions++ }
+  move(from, to) { this.items.splice(to, 0, this.items.splice(from, 1)[0]) }
+  setProperty(i, key, value) { this.items[i][key] = value }
+}
+
+for (const count of [0, 1, 200, 530, 10000]) {
+  const collection = Array.from({ length: count }, (_, i) => ({ filePath: `/themes/theme-${i}.png` }))
+  const indices = picker.matchingIndices(collection, '')
+  const model = new WindowModel()
+  for (const selected of [0, Math.min(1, count - 1), Math.floor(count / 2), count - 1]) {
+    const window = picker.visibleWindow(indices, selected, 16)
+    picker.syncWindow(model, window)
+    assertDeepEqual(model.items, window, `carousel window reconciles ${count} images at ${selected}`)
+    assert(model.count <= 33, `carousel bounds delegates for ${count} images`)
+    if (count) assert(window.some(item => item.imageIndex === selected && item.relativeIndex === 0), `carousel includes selection in ${count} images`)
+  }
+  const matches = picker.matchingIndices(collection, 'theme-19')
+  picker.syncWindow(model, picker.visibleWindow(matches, matches[0], 16))
+  assert(model.items.every(item => collection[item.imageIndex].filePath.includes('theme-19')), `carousel filters ${count} images`)
+  picker.syncWindow(model, picker.visibleWindow([], 0, 16))
+  assertEqual(model.count, 0, `carousel clears ${count} images when there are no matches`)
+}
+
+const windowModel = new WindowModel()
+const allIndices = Array.from({ length: 200 }, (_, i) => i)
+picker.syncWindow(windowModel, picker.visibleWindow(allIndices, 100, 16))
+const retained = windowModel.get(17)
+picker.syncWindow(windowModel, picker.visibleWindow(allIndices, 101, 16))
+assertEqual(windowModel.get(16), retained, 'carousel retains overlapping delegates when navigating')
+assertEqual(windowModel.insertions, 34, 'carousel creates only one new delegate for an adjacent selection')
+for (const radius of [1, 8, 16]) {
+  assertEqual(picker.visibleWindow(allIndices, 100, radius).length, radius * 2 + 1, `carousel scales its window to radius ${radius}`)
+}
+
 const imagePickerQml = fs.readFileSync(path.join(root, 'shell/plugins/image-picker/ImagePicker.qml'), 'utf8')
+// Exercise the actual QML refresh handler: model-only filtering tests cannot
+// catch a row refresh selecting an image outside the active filter.
+const refreshHandler = imagePickerQml.match(/function loadRows\(rows, reveal\) \{[\s\S]*?\n  \}/)[0]
+const refreshRoot = {
+  filterText: 'dark',
+  selectedImage: '/themes/removed-dark.png',
+  requestSerial: 1,
+  indexForSelectedImage(images) { return picker.indexForSelectedImage(images, this.selectedImage) },
+  enableNeighborsWhenReady() {},
+  revealWhenSettled() {}
+}
+const refreshContext = {
+  root: refreshRoot,
+  ImagePickerModel: picker,
+  Qt: { callLater(callback) { callback() } }
+}
+require('vm').runInNewContext(`${refreshHandler}; loadRows`, refreshContext)
+const refreshRows = '/themes/light.png\n/themes/remaining-dark.png\n/themes/other-dark.png'
+refreshContext.loadRows(refreshRows, false)
+assertEqual(refreshRoot.selectedIndex, 1, 'filtered refresh selects a visible row after the selected theme is removed')
+assert(picker.visibleWindow(picker.matchingIndices(refreshRoot.imageArray, 'dark'), refreshRoot.selectedIndex, 8)
+  .some(item => item.imageIndex === refreshRoot.selectedIndex), 'filtered refresh has a selected delegate to start preview loading')
+refreshRoot.selectedImage = '/themes/other-dark.png'
+refreshContext.loadRows(refreshRows, false)
+assertEqual(refreshRoot.selectedIndex, 2, 'filtered refresh preserves a selected theme that still matches')
+refreshRoot.selectedImage = '/themes/light.png'
+refreshContext.loadRows(refreshRows, false)
+assertEqual(refreshRoot.selectedIndex, 1, 'filtered refresh moves a hidden selection to the first match')
+refreshRoot.filterText = 'missing'
+refreshContext.loadRows(refreshRows, false)
+assertEqual(refreshRoot.selectedIndex, -1, 'filtered refresh leaves no selection when nothing matches')
+refreshRoot.filterText = ''
+refreshContext.loadRows(refreshRows, false)
+assertEqual(refreshRoot.selectedIndex, 0, 'unfiltered refresh retains its first-row fallback')
+refreshContext.loadRows('', false)
+assertEqual(refreshRoot.selectedIndex, -1, 'empty refresh has no selected image')
+
 assert(
   /function preloadRows[\s\S]*if \(opened \|\| requestActive\) return/.test(imagePickerQml),
   'image picker ignores cache preloads while a request is visible'
@@ -74,7 +150,14 @@ assert(
   'image picker parks on OverlayWindow and takes the keyboard once images load'
 )
 assert(
-  /source: item\.sourceActivated && item\.thumbnailPath \? Util\.fileUrl\(item\.thumbnailPath\) : ""[\s\S]*asynchronous: false/.test(imagePickerQml),
-  'image picker loads activated thumbnails synchronously to avoid carousel flicker'
+  /model: visibleImages/.test(imagePickerQml) &&
+    /sourceSize: Qt\.size\(root\.expandedWidth, root\.expandedHeight\)/.test(imagePickerQml) &&
+    /asynchronous: true\s*cache: false/.test(imagePickerQml),
+  'image picker renders its window with bounded asynchronous decoding'
+)
+assert(
+  imagePickerQml.includes('(item.selected || root.neighborImagesEnabled)') &&
+    /onStatusChanged: if \(item.selected && \(status === Image.Ready \|\| status === Image.Error\)\) root.neighborImagesEnabled = true/.test(imagePickerQml),
+  'image picker prioritizes the selected preview and releases neighbors on success or failure'
 )
 JS

test/shell.d/menu-images-test.sh

@@ -5,9 +5,34 @@ set -euo pipefail
 source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh"
 
 require_command flock
+require_command setsid
 
 tmp=$(mktemp -d)
-trap 'rm -rf "$tmp"' EXIT
+lazy_groups=()
+cleanup() {
+  local status=$? gate group running attempt
+  trap - EXIT
+  # Release every fixture gate, including on an assertion failure. Each lazy
+  # invocation owns a private process group, so its detached pool is tracked.
+  for gate in "$tmp"/lazy-state*/gate; do
+    [[ ! -d ${gate%/*} ]] || touch "$gate"
+  done
+  for attempt in {1..500}; do
+    running=false
+    for group in "${lazy_groups[@]}"; do
+      if kill -0 -- "-$group" 2>/dev/null; then running=true; fi
+    done
+    [[ $running == "true" ]] || break
+    sleep 0.02
+  done
+  for group in "${lazy_groups[@]}"; do
+    kill -TERM -- "-$group" 2>/dev/null || true
+    wait "$group" 2>/dev/null || true
+  done
+  rm -rf "$tmp"
+  exit "$status"
+}
+trap cleanup EXIT
 
 cache_home="$tmp/cache"
 images="$tmp/images"
@@ -151,3 +176,141 @@ while IFS=$'\t' read -r row_image row_thumbnail; do
     fail "image menu prints each image with its generated thumbnail"
 done <<<"$rows"
 pass "image menu prints its rows for the shell to hold"
+
+# Block converters behind a gate: printing lazy rows must neither await them
+# nor start one process per image. Repeated refreshes share one worker pool.
+lazy_images="$tmp/lazy-images"
+lazy_state="$tmp/lazy-state"
+mkdir -p "$lazy_images" "$lazy_state"
+for (( i = 0; i < 40; i++ )); do
+  printf 'image' >"$lazy_images/$i.png"
+done
+printf '0\n' >"$lazy_state/active"
+printf '0\n' >"$lazy_state/peak"
+cat >"$stub_bin/nproc" <<'EOF'
+#!/bin/bash
+echo "${FAKE_CORES:-2}"
+EOF
+cat >"$stub_bin/vipsthumbnail" <<'EOF'
+#!/bin/bash
+while (( $# > 0 )); do
+  if [[ $1 == "--path" ]]; then output=${2%%\[*}; break; fi
+  shift
+done
+exec 9>"$LAZY_STATE/lock"
+priority=$(ps -o ni= -p "$$")
+(( priority >= 10 )) || : >"$LAZY_STATE/priority-failed"
+[[ $(ionice -p "$$") == "idle" ]] || : >"$LAZY_STATE/priority-failed"
+flock 9
+active=$(<"$LAZY_STATE/active")
+active=$((active + 1))
+printf '%s\n' "$active" >"$LAZY_STATE/active"
+(( active <= $(<"$LAZY_STATE/peak") )) || printf '%s\n' "$active" >"$LAZY_STATE/peak"
+flock -u 9
+while [[ ! -f $LAZY_STATE/gate ]]; do sleep 0.02; done
+printf 'thumbnail' >"$output"
+flock 9
+active=$(<"$LAZY_STATE/active")
+printf '%s\n' "$((active - 1))" >"$LAZY_STATE/active"
+echo done >>"$LAZY_STATE/completed"
+EOF
+chmod +x "$stub_bin/nproc" "$stub_bin/vipsthumbnail"
+
+lazy_rows() {
+  setsid env PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$tmp/lazy-cache-$cores" LAZY_STATE="$lazy_state" FAKE_CORES="$cores" \
+    timeout --foreground 10 "$ROOT/bin/omarchy-menu-images" --lazy-thumbnails --print-rows "$lazy_images" >"$lazy_state/rows" &
+  local group=$!
+  lazy_groups+=("$group")
+  wait "$group" || fail "lazy image menu returns rows without waiting for its pool"
+  rows=$(<"$lazy_state/rows")
+}
+
+for cores in 1 2 8; do
+  rm -f "$lazy_images/new.png"
+  printf 'image' >"$lazy_images/0.png"
+  lazy_state="$tmp/lazy-state-$cores"
+  mkdir -p "$lazy_state"
+  printf '0\n' >"$lazy_state/active"
+  printf '0\n' >"$lazy_state/peak"
+  expected_workers=1
+  (( cores < 4 )) || expected_workers=2
+  for run in 1 2; do
+    lazy_rows
+    (( $(wc -l <<<"$rows") == 40 )) || fail "lazy image menu returns all rows before conversion"
+  done
+  for attempt in {1..100}; do
+    (( $(<"$lazy_state/active") == expected_workers )) && break
+    sleep 0.02
+  done
+  (( $(<"$lazy_state/active") == expected_workers && $(<"$lazy_state/peak") == expected_workers )) ||
+    fail "lazy image menu bounds repeated refreshes to $expected_workers workers on $cores cores"
+  [[ ! -e $lazy_state/completed ]] || fail "lazy image menu does not wait for conversion"
+  [[ ! -e $lazy_state/priority-failed ]] || fail "lazy image menu reserves CPU and I/O priority for the UI"
+  pass "lazy image menu opens with at most $expected_workers workers on $cores cores"
+
+  # This job did not exist in the pool's first batch. A contended refresh must
+  # retain it, along with a replacement for an image changed during conversion.
+  printf 'new-image' >"$lazy_images/new.png"
+  printf 'changed-image-with-new-size' >"$lazy_images/0.png"
+  lazy_rows
+  (( $(wc -l <<<"$rows") == 41 )) || fail "contended refresh returns the added image"
+  for changed in new 0; do
+    signature=$(stat -Lc '%s:%Y' "$lazy_images/$changed.png")
+    hash=$(printf '%s\t%s' "$lazy_images/$changed.png" "$signature" | md5sum | cut -d ' ' -f 1)
+    [[ ! -e $tmp/lazy-cache-$cores/omarchy/image-selector/$hash.jpg ]] ||
+      fail "contended refresh does not start another converter pool"
+  done
+
+  touch "$lazy_state/gate"
+  for attempt in {1..500}; do
+    if [[ -f $lazy_state/completed ]] && (( $(wc -l <"$lazy_state/completed") == 42 )); then break; fi
+    sleep 0.02
+  done
+  (( $(wc -l <"$lazy_state/completed") == 42 && $(<"$lazy_state/peak") == expected_workers )) ||
+    fail "lazy image menu completes the queue after its parent and queue path are gone"
+  for changed in new 0; do
+    signature=$(stat -Lc '%s:%Y' "$lazy_images/$changed.png")
+    hash=$(printf '%s\t%s' "$lazy_images/$changed.png" "$signature" | md5sum | cut -d ' ' -f 1)
+    [[ -f $tmp/lazy-cache-$cores/omarchy/image-selector/$hash.jpg ]] ||
+      fail "lazy image menu retains new and changed jobs while its pool is busy"
+  done
+  pass "lazy image menu workers finish every queued thumbnail after the caller exits"
+done
+
+# Exercise the same cleanup handler on both successful and failing exits,
+# with a detached, gated fixture still running when the EXIT trap fires.
+run_node_test <<'JS'
+const fs = require('fs')
+const { spawnSync } = require('child_process')
+const script = fs.readFileSync(path.join(root, 'test/shell.d/menu-images-test.sh'), 'utf8')
+const cleanupHandler = script.match(/cleanup\(\) \{[\s\S]*?\n\}/)[0]
+for (const status of [0, 31]) {
+  const result = spawnSync('bash', ['-c', `
+set -euo pipefail
+tmp=$(mktemp -d)
+lazy_groups=()
+${cleanupHandler}
+trap cleanup EXIT
+mkdir -p "$tmp/lazy-state-probe"
+setsid bash -c ': >"$1/ready"; while [[ ! -e $1/gate ]]; do sleep 0.02; done; sleep 0.05' _ "$tmp/lazy-state-probe" &
+lazy_groups+=("$!")
+while [[ ! -e $tmp/lazy-state-probe/ready ]]; do sleep 0.02; done
+printf '%s\\n%s\\n' "$tmp" "$!"
+exit ${status}
+`], { encoding: 'utf8', timeout: 15000 })
+  const [directory, group] = result.stdout.trim().split('\n')
+  const removed = !fs.existsSync(directory)
+  let alive = false
+  try {
+    try { process.kill(-Number(group), 0); alive = true } catch (error) {
+      if (error.code !== 'ESRCH') throw error
+    }
+  } finally {
+    if (group) { try { process.kill(-Number(group), 'SIGKILL') } catch (_) {} }
+    if (directory) fs.rmSync(directory, { recursive: true, force: true })
+  }
+  assertEqual(result.status, status, `fixture cleanup preserves exit status ${status}`)
+  assert(removed, `fixture cleanup removes its directory on exit ${status}`)
+  assert(!alive, `fixture cleanup finishes its detached workers on exit ${status}`)
+}
+JS