Conversation
gammons
left a comment
There was a problem hiding this comment.
Overall
Solid, well-scoped change. The core insight — that nil (not lipgloss.NoColor{}) is the correct sentinel because NoColor.RGBA() reports opaque black — is correct and clearly documented. Backwards compatibility is preserved (guarded if overrides.X != "" calls, no built-in theme uses the keywords), and the tests cover case-insensitivity plus both nil directions in mixColors.
I verified the PR's central claim that every hand-rolled color conversion is guarded by grepping all .RGBA() call sites: the only theme-color consumers are tint.go, render.go, and blockkit/color.go (the halfblock.go one operates on image pixels, not theme colors), and all three are now nil-guarded. Coverage is complete. Build and the styles/messages/blockkit suites pass locally.
Issues worth addressing
1. (Design) Selection highlight and compose-insert background silently vanish on transparent themes
When Background is transparent, mixColors returns nil for any derived tint:
SelectionTintColor(tint.go:39,44) → the selected-message row gets no background → the "which row is selected" affordance disappears.ComposeInsertBG(styles.go:382) → insert-mode compose box loses its background cue.
This may be an acceptable tradeoff, but it's a real UX regression for exactly the users this feature targets. Consider whether these derived indicators should fall back to a solid color (e.g. Accent) rather than going transparent, so selection stays visible. At minimum this should be a conscious, documented decision. This is the one item I'd want an answer on before merge.
2. (Low) nil sentinel collision in SelectionTintColor
selectionBgFocused/selectionBgUnfocused use nil to mean "not computed yet" (tint.go:38,43). With a transparent Background, mixColors legitimately returns nil, so the cache never populates and mixColors re-runs on every call. Harmless today (the bg == nil branch returns early), but it defeats the caching and is a latent footgun. A separate computed bool flag would be cleaner.
3. (Low) "default" as a synonym for transparent is ambiguous
transparent/none clearly signal "no color," but a user reading background = "default" in TOML could reasonably read it as "the theme's default color" rather than "the terminal's default background." Consider dropping "default" or documenting it explicitly.
4. (Low, docs) wiki/Configuration.md isn't updated
The theme section documents color values but doesn't mention the new transparent/none/default keywords, so the feature is undiscoverable outside the PR body. Worth a one-line addition. (There's no color validation in config.go, so nothing to update there.)
5. (Nit) Magic constant in colorString
The #333333 fallback (blockkit/color.go:50) duplicates the default Border value. A named const (e.g. subduedAttachmentBar) would document intent.
Verdict
Approvable. None of the above are blockers, but #1 is worth a quick answer before merge — it's the difference between "transparent works" and "transparent works but you can't see what row you've selected."
|
hi @brettbash see my comments. the first issue is really the one that should be addressed before this can be merged. |
26938c2 to
02860d7
Compare
Theme colors now accept "transparent", "none", or "default", letting terminal emulators with background opacity (Ghostty, kitty, etc.) show the wallpaper through slk's panels. Transparent values resolve to a nil color, which lipgloss renders as "no color". The hand-rolled color conversions are guarded so nothing paints literal black for nil: message-pane background fills emit the ANSI default-background reset (SGR 49), foregrounds emit SGR 39, derived tints (compose insert bg, selection rows) stay transparent when mixed against a transparent background, and attachment color bars fall back to a neutral gray. Fully backwards compatible: existing hex/ANSI theme values are unchanged, and no built-in theme uses the new keys.
02860d7 to
7d46f75
Compare
|
Sorry this sat so long — it's the oldest PR in the queue and that's on me. CI had also never been allowed to run (first-time contributor); I've approved it, and the The core mechanism is right, and you got the hard part right. Using But the feature doesn't actually work as documented, and that's what's blocking. I applied
I also confirmed my earlier concern about selection, plus two more: The last two are readability regressions, not just aesthetics — on a light-fg terminal that's white on orange. What I'd like:
To be clear on the product question: I want this feature. 59 themes plus drop-in customs means theming is first-class here, it's opt-in, it's zero-cost for existing users, and terminal transparency is a reasonable thing to want. It just needs to actually work on more than 2 themes before it ships. |
|
#171 has landed and I've re-run CI here — the Note that #171 also enabled My review above still stands — that's what's needed to move this forward. |
Summary
"transparent","none", or"default"so terminal emulators with background opacity (Ghostty, kitty, alacritty, etc.) can show the wallpaper through slk's UI instead of painting opaque panels.NoColor.RGBA()reports opaque black). Those are guarded:Usage
Compatibility
Fully backwards compatible — existing hex/ANSI values parse exactly as before, and no built-in theme uses the new keywords. Covered by new tests in
styles_test.goandtint_test.go; full suite passes with-race.