Follow-ups to the icon handling change in #16115
The change in #16115 fixed three things about the icons of the app launchers: a URL that answers with an error body no longer installs that body as the icon, a bare icon name that resolves to nothing is reported, and removing a launcher now takes its icon away in the formats the icon theme uses.
Several things were left out, and the review of that change named all of them. This issue tracks them, so #16115 can land without growing further.
1. An icon installed in another format survives its removal
omarchy-tui-install copies a local icon under the extension that file
already has:
omarchy-tui-install T3 true tile ./icon.svg # installs t3.svgThe removal scripts name the formats the theme uses, png, svg and xpm,
so an icon installed in any other format is left behind in the icon
directory when its launcher is removed.
The formats the installers can produce are not bounded by the theme's formats, so either the removal should match the icon by its name in every format, or the installers should only accept the formats the theme has.
2. An interrupted download leaves a file in the indexed icon directory
The answer is downloaded to a temporary file beside the icon, so that an install that is interrupted between the download and the move leaves a stray file inside the icon theme directory the desktop indexes:
omarchy-tui-install T1 true tile https://example.com/slow-icon.png # ^CA temporary file outside the indexed directory, or a cleanup that also runs when the install is interrupted, would leave nothing behind.
3. The download deadline is new for the TUI path
omarchy-tui-install downloaded the icon with no limit at all before, so
an icon URL that was slow but reachable used to install. The change gives
the download ten seconds, the same limit omarchy-webapp-install already
had, and a slow but valid icon now fails the whole install with
Error: Failed to download icon.Whether the TUI installer wants that limit is a policy question, and the answer may be different from the web app's.
4. The rule is written twice, and the copies already differ
The download helper now exists in omarchy-tui-install and in
omarchy-webapp-install, and the copies are not interchangeable: the TUI
one keeps the downloader's own error output, the web app one discards it
because it falls back to another source quietly. The icon removal rule is
likewise written out in both removal scripts, and the sanitizer each one
uses to name the icon is a private copy of the installer's.
A shared helper would give the rule one owner. The two installers are siblings by design, and the rest of their icon handling is already copied between them, so this is the part where the copying is starting to cost.
5. The search is bounded to a depth the theme does not have to obey
The theme lookup searches to a fixed depth of four levels below each theme root, which is where the standard layout ends. The desktop resolves a name at any depth inside a theme and through theme inheritance, so a theme that nests one level deeper is reported as missing, a warning the baseline never produced. The number is not explained anywhere either.
6. The system data directories are never searched by the test
The test points XDG_DATA_DIRS at a sandbox directory for the whole file,
so the branch the search takes when the variable is unset — the one that
covers the package-owned icons under the system data directories — is never
exercised. A wrong default list would make the new warning fire on every
install that names a bundled icon, and the suite would still pass.
Two of the cases in that suite also capture the installer's exit status and never assert it, so they would pass if the installer failed before it reached the icon check at all.
7. The web app installer's new download behaviour has no test
The suite exercises the TUI installer, the TUI removal and the web app
removal, but never runs omarchy-webapp-install, so the guarantee that a
failed web app download leaves the installed icon alone is unproven. The
web app case in the suite installs through the TUI installer and writes the
launcher file by hand, which reads as a round trip but is not one.
Also worth covering
The mode the icon is installed with, the depth the search reaches and the download deadline are three bare literals in the code, and the mode is a third literal in the test. Naming them would give the rules one owner.
What is already handled
- A URL that answers with an error body, or with something that is not an image, fails the install and leaves no icon behind.
- A failed reinstall keeps the icon that was installed before it.
- A bare name that resolves to nothing is reported, and the launcher is still written.
- A name the user typed cannot reach another app's icon, and a name that sanitizes to nothing is removed by name.
- The icon the theme provides as a symbolic link is found, which matters because most of the installed themes are made of links.
Activity
3
statick8822h agoThe review of #16127 found one thing in it that was a defect of the change itself: the new test of the system data directories took the first icon the host's directory order offered and, when that name did not survive the launcher's round trip, took the first one again with the previous name excluded, which is the same file it had just rejected. On a host whose first two icons both carry a name the launcher resolves differently, the search never ended and the suite hung instead of failing.
The search now asks for the shape the launcher resolves unchanged: a stem of lower-case letters, digits and dashes. One search, no loop.
The pull request was reviewed again over the corrected tree and approved.
statick8821h agoThe maintainers' reviewer found one thing in #16127 that was a defect of the change itself: the removal matched the icon's name with a wildcard, so removing one launcher could take another app's icon with it. An icon whose name carries a second part belongs to another app, because the launcher resolves "foo.bar" to "foo-bar", and the lookup already told those two apart.
The removal now asks the same question the lookup asks: the name of the icon is the name of the file without its extension. The suite covers it.
The reviewer also named two things in the suite: a search that failed ended the test file before the skip it was written for, and the check of the signal handler ended without saying which part had failed. Both are fixed.
All seven points are addressed in #16127, which is stacked on #16115.
install/helpers/icon.sh, which the four commands source; the copies that differed are now the caller's choiceThe review of that pull request also found five things in it, and each one is now fixed with a test: the icon theme cache was no longer refreshed after a download, the removal took files that were not icons, the web app installer sourced the shared helper twice, the removal scripts needed the helper's environment to list the launchers, and two of the new tests could pass without proving what they claim.
Leaving this open until the pull request is merged.