Skip to content

Enlarge the S in the glass app icon - #234

Merged
delano merged 1 commit into
mainfrom
codex/enlarge-glass-icon
Oct 2, 2026
Merged

delano merged 1 commit into
mainfrom
codex/enlarge-glass-icon

Conversation

@delano

@delano delano commented Oct 2, 2026

Copy link
Copy Markdown
Member

Summary

The glass icon’s S occupied about 51% of the canvas height and looked undersized in the Dock and Command-Tab. Render the Composer layers against the full canvas and enlarge the logo to 70.4%, with regenerated foreground, cast shadow, and preview assets.

Add scripts/build-icons.sh --update-glass-artwork to regenerate the saved logo layers and preview through Icon Composer’s renderer. Document this workflow in the README.

Review guide

Compare the updated artwork/OnetimePad-Glass-CastShadow.png preview with the previous version. The sizing adjustment is in writeComposerLayer; the saved icon.json material settings are unchanged.

Validation

  • All 8 icon assembly tests passed (python3 -m unittest scripts.tests.test_app_icons).
  • Ran --update-glass-artwork and compiled the saved composition with --glass.
  • Completed local packaging and install.sh --allow-capture; verified the installed fallback contains the enlarged S.
  • Shell syntax and diff whitespace checks passed.

Render Composer layers against the full canvas and increase the logo placement to 70.4% of its height. Regenerate the saved foreground, cast shadow, and glass preview.
@greptile-apps

greptile-apps Bot commented Oct 2, 2026

Copy link
Copy Markdown

Required label not found on this PR.

@claude

claude Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Claude finished @delano's task —— View job


Code Review: Enlarge the S in the glass app icon

  • Read repo conventions (AGENTS.md, script headers, icon docs)
  • Review scripts/render-icon.swift changes
  • Review scripts/build-icons.sh changes
  • Review README / docs consistency
  • Inspect the regenerated preview asset
  • Run scripts/tests/test_app_icons.py — not possible here (see Validation note)

Overall this is a clean, well-commented change and the regenerated preview reads well at 1024. Nothing looks unsafe or broken; the notes below are mostly about keeping the renderer's constants coherent now that the composer layers no longer share the standalone renderer's geometry.

Renderer (scripts/render-icon.swift)

1. Magic string + magic number for the scale (render-icon.swift:753)

let foregroundScale: CGFloat = mark.name == "logo" ? 0.80 : markScale

mark.name == "logo" is the only place in the file that branches on a mark's name (the registry at render-icon.swift:322-326 is otherwise data-driven). Consider either a composerScale field on Mark (render-icon.swift:191) or a named constant next to logoFill (render-icon.swift:298). The // … makes this 70.4% of the canvas height comment hardcodes the product of two constants, so it rots silently if logoFill ever moves; deriving it the other way keeps the intent authoritative:

/// The share of the Composer canvas the logo's ink should span.
let composerLogoCoverage: CGFloat = 0.704
… mark.name == "logo" ? composerLogoCoverage / logoFill : markScale

2. The maruhi grows too, silently (render-icon.swift:751-754)
The markScale branch looks unchanged, but tile went from 824 to 1024, so --mark maruhi --composer now renders the glyph ~1.24× larger than before. If that is intended, the comment should say the inset fix applies to both marks; if the maruhi was meant to be untouched, it needs markScale * 824 / 1024. No saved maruhi assets exist, so nothing in-tree regresses — it is just an undocumented behaviour change in a supported mode.

3. The cast shadow geometry stayed absolute while the mark grew (render-icon.swift:768-779, blur at :797)
The sweep (0...420), the ramp end (420/sqrt(2) from the canvas centre), and the 7-tap blur are all in canvas pixels, so after a 1.38× mark they describe a relatively ~28% shorter, relatively tighter shadow. By my arithmetic on the fitted box (≈721px on the long side vs ≈522px before), the mark's SE extremity now projects ≈467px along the shadow axis from the centre, past the ramp's zero at 420 — previously ≈338px, comfortably inside it. The practical effect is that part of the S's lower-right now sits in the zero-alpha region and the extrusion's terminating edge lands somewhere new. The repo already has history here (docs/spec/icon/about-longshadow.md:69, the un-shadowed crescent). Suggest deriving the run and the ramp from foregroundScale (e.g. 420 * foregroundScale / markScale) so the composition is scale-invariant, and eyeballing the preview's lower-right/bottom-left for a hard cut.

4. Design check (non-blocking). On the regenerated preview the S spans roughly y≈150–880 of 1024, i.e. about 72% of the macOS shape's height with ~14% margin top and bottom. That is hotter than most stock macOS icons, so it is worth a side-by-side in the Dock at 16/32pt and a look at how the enlarged silhouette interacts with the Composer mask's specular edge. Purely a design call — the 1024 preview itself looks good.

Build script (scripts/build-icons.sh)

5. No output validation, unlike its sibling (build-icons.sh:151-171). build_glass_icon carefully checks every artefact is nonempty (:206-220). update_glass_artwork copies two PNGs without checking the renderer produced them, and never checks that ictool wrote a nonempty artwork/OnetimePad-Glass-CastShadow.png. A tool that exits 0 while writing nothing would leave a silently truncated tracked asset. Suggest [[ -s … ]] guards for the three destinations.

6. Xcode version gate is not applied here (build-icons.sh:159-162 vs :182-188). --glass requires Xcode 27+ because of refractivity; the preview exported by ictool is supposed to represent that same composition, so the ≥27 check (or a shared helper) belongs in this path too — otherwise an older Icon Composer can export a preview that does not match what actool compiles.

7. ictool is located by path arithmetic (build-icons.sh:158). dirname "$DEVELOPER_DIR" → Xcode.app/Contents is right for the standard layout but silently wrong elsewhere; the error message mentions DEVELOPER_DIR but not the path that was tried. Printing $composer_tool in the failure message would save a debugging round trip.

8. The function mutates tracked artwork without saying which files (build-icons.sh:166-170). Every other mode echoes its output path. Echoing the three destinations makes the subsequent git diff of binaries reviewable.

9. Staleness has a gap. build_icon_if_stale restales on render-icon.swift edits (build-icons.sh:139-149), but nothing ties artwork/OnetimePad-Glass.icon/Assets/*.png to the renderer. This PR is exactly the case that drift would hide: change foregroundScale, forget --update-glass-artwork, and --glass happily compiles stale art. A warning in build_glass_icon when scripts/render-icon.swift -nt artwork/OnetimePad-Glass.icon/Assets/01-logo-foreground.png would close it cheaply.

10. The usage string is now duplicated five times (:287, :300, :318, :323, plus the earlier copy). This PR had to edit all five identically — a good moment to extract a usage() function.

Docs

11. Stale cross-references. docs/spec/icon/about-longshadow.md:12-14 still states the export "does not update the saved composition", and the script header at build-icons.sh:21-25 still says to import the PNGs by hand. Both should point at --update-glass-artwork now that it exists.

12. README wording (README.md:109): "the saved S foreground" — elsewhere the repo consistently calls it the logo mark; "the saved logo foreground" would match build-icons.sh:3-9.

Validation

I could not reproduce the PR's validation in this environment: the runner is Linux and build-icons.sh exits early off Darwin (:57-60), and python3 -m unittest scripts.tests.test_app_icons was not in my allowed tool set. The test file does not currently assert anything about layer geometry, so the 8 passing tests do not cover the sizing change — the preview image is the only regression surface, which is an argument for the staleness check in item 9.

Per AGENTS.md, I have treated the PR description's percentages as the author's measurements rather than as established project claims; the 70.4% figure is consistent with 0.80 × logoFill (0.88) as written in the code.
• branch codex/enlarge-glass-icon

@delano
delano merged commit 6a4f3cf into main Oct 2, 2026
@delano
delano deleted the codex/enlarge-glass-icon branch October 2, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant