#16127

Give the icon rule of the app launchers one owner

Openstatick88wants to merge intofrom

This closes the follow-ups of #16115, tracked in #16120.

The icon handling of the app launcher commands was written four times: once in omarchy-tui-install, once in omarchy-webapp-install, and once in each of the two removal scripts. The copies had already started to differ, and the review of #16115 named what the differences cost. This gives the rule one owner and closes the gaps that were left.

What changed

One owner for the icon rule. The name an icon is known by, the reference-to-name rule, the local install, the guarded download, the theme lookup and the removal live in install/helpers/icon.sh, which the four commands source. The copies that already differed are now the caller's choice: the web app installer discards the downloader's error output because it falls back to another source quietly, and the TUI installer keeps it because a failed fetch has to say why it failed.

An icon is removed in the format it was installed with. The installers copy a local icon under the extension that file already has, so the formats they can produce are not bounded by the formats the theme ships. The removal matched the icon by name in png, svg and xpm, so an icon installed in any other format was left behind. It is now matched by its sanitized name in every format, and only the files that are images go with it, so a file that carries the name but is not an icon stays.

A download in flight leaves nothing in the indexed directory. The answer is downloaded to a temporary file before the icon is written. That file used to be created inside the icon directory the desktop indexes, so a download that died left a file there for the theme to pick up. The temporary file is now written outside that directory, and it is removed when the install is interrupted. The caller's own signal handlers are put back afterwards, so a command that already handles a signal does not lose it because it downloaded an icon.

The icon theme cache is refreshed. The base refreshed it after every download, and the shared helper refreshes it too, so an icon installed from a URL is registered with the cache the desktop resolves it through.

One download deadline, named. The TUI path had no limit before #16115 and the web app path had ten seconds. The shared helper holds one deadline of ten seconds, and the limit the search reaches is named next to the rule as well.

The theme lookup reaches every real entry. The bound is the icon theme's own layout: the theme, the size, the context and the name, which is four levels below the icons directory the search starts at. On a live system 68,952 entries sit at that level and none sits deeper, so the bound covers everything that is really there.

The suite covers what it did not. The web app installer's download contract is now exercised: an icon URL that answers with an image installs it, one that answers with an error body fails the install and leaves the installed icon alone. The lookup is also exercised against the system data directories, the branch the environment takes when it names no list of its own, and every case now asserts the exit status it captures.

How it was verified

  • The suite in test/shell.d/tui-icon-test.sh runs the real commands against a fake home. The new cases fail against the code before this change: an icon installed as .jpg survived its removal, and a download in flight left a file in the icon directory.
  • The full suite was run on this change and on the commit before it, one after the other, and the set of failures is the same. The failures that were already there are environmental, and the pull request lists them.
  • The change was reviewed with the native review contract, and the receipt covers the commit that is pushed.

This is stacked on #16115: the rule only exists there, so this branch is based on it and the pull request targets that branch.

Activity

22
statick8816h ago

Stacked on #16115: the rule this change gives one owner is introduced there, so this branch is based on it and the pull request targets quattro until #16115 lands. Once it does, this pull request shows only its own two commits.

The seven commits it carries now are #16115's.

greptile-apps[bot]16h ago

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge, with two non-blocking cleanup and test concerns.

Findings

  1.  Interrupted downloads leave files ▶
  2.  Signal test starts too early ▶

Summary

The PR gives the four launcher commands one shared owner for icon naming, downloads, lookup, installation, and removal.

  • TUI and web-app launchers share rules for installing icons.
  • Removing a launcher removes its matching image icons in any format.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[TUI install and remove] --> C[Shared icon helper]
  B[Web app install and remove] --> C
  C --> D[Icon names and theme lookup]
  C --> E[Checked downloads and cache refresh]
  C --> F[Local install and image removal]

Reviews (8) · Last reviewed commit: "Pin the boundary the icon rules claim" · Reviewed by Greptile

greptile-apps[bot]reviewed16h ago
greptile-apps[bot]reviewed16h ago
greptile-apps[bot]reviewed12h ago
greptile-apps[bot]reviewed11h ago