Conversation
|
Thanks for the thoughtful implementation and the screenshots. I like this direction, and I am reopening the PR. It should fit into a broader theming model rather than make Omarchy configuration the Fastpotify theme API. The cleanest shape may be a generic Fastpotify theme file that Omarchy writes. Please hold off on reshaping the implementation until we settle which side should own that integration. |
There was a problem hiding this comment.
🟡 Changes recommended
The PR introduces user-facing help/error strings that can be misleading or inaccurate in real failure modes, and these should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
User-visible UI impact: Adds an opt-in Omarchy theme choice on Linux that follows Omarchy’s resolved desktop palette and selected interface font (including live updates). The built-in Winamp skin is recoloured to match the active app palette in non-default palettes.
Changes:
- Adds an
OmarchyThemeChoice, wires it through settings UI, serialization, and runtime theme selection. - Introduces Linux-only Omarchy integration: palette parsing, font resolution via Fontconfig, and a polling watcher for live updates.
- Recolours the built-in Winamp skin to the active
Palette(while keeping user-provided.wszskins unchanged), plus regression tests and docs updates.
File summaries
| File | Description |
|---|---|
| src/ui/settings.rs | Conditionally shows the Omarchy theme option and adds explanatory text in the Appearance section. |
| src/theme.rs | Adds interface-font plumbing and support for installing/applying an Omarchy-provided font. |
| src/skin/mod.rs | Implements palette-aware recolouring for the built-in Winamp skin and adds tests for recolouring behavior. |
| src/settings.rs | Adds ThemeChoice::Omarchy, updates labels/ALL list, and adds a serialization round-trip test. |
| src/omarchy.rs | New Linux Omarchy integration: palette parsing, font resolving, and a polling watcher with tests. |
| src/omarchy_stub.rs | New non-Linux stubs for the Omarchy module APIs. |
| src/lib.rs | Adds platform-gated omarchy module wiring (real vs stub). |
| src/app.rs | Applies Omarchy palette/font at runtime, refreshes built-in skin on palette changes, and adds regression tests. |
| README.md | Documents Omarchy theme support and built-in skin palette-following behavior. |
| docs/_reference/settings-and-files.md | Documents theme=omarchy and the Omarchy file paths/behavior. |
| docs/_guide/winamp.md | Documents built-in skin palette-following and preserved classic skins. |
| docs/_guide/what-is-fastpotify.md | Updates high-level feature description to include Omarchy theme support on Linux. |
Review details
Suppressed comments (1)
src/omarchy.rs:51
- Same as load_palette(): if BaseDirs cannot be created, this error currently claims Omarchy fonts are unavailable on Linux, which is misleading. Use a home-directory resolution error instead so users understand what failed.
let path =
fontconfig_path().ok_or_else(|| "Omarchy fonts are only available on Linux".to_string())?;
let source = std::fs::read_to_string(&path)
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let omarchy_face = |face: &FontFace, weight: f32| { | ||
| let mut data = FontData::from_owned(face.bytes.to_vec()); | ||
| data.index = face.index; | ||
| data.tweak.coords = VariationCoords::new([(b"wght", weight)]); | ||
| Arc::new(data) | ||
| }; |
| let path = | ||
| theme_path().ok_or_else(|| "Omarchy themes are only available on Linux".to_string())?; | ||
| let source = std::fs::read_to_string(&path) |
| if omarchy_theme_visible { | ||
| "Omarchy follows the active desktop palette." | ||
| } else { | ||
| "" | ||
| }, |
|
Im a bit confused as to why this was reopened if the goal is still to have generic theming and not direct compatibility with one specific system, as this PR seems to introduce the latter. |
|
It was reopened to keep the theming discussion open. The integration approach is still undecided, and this implementation is not approved for merging. Please hold off on further changes until I settle that. |
Wouldn't it make more sense to move to an issue? |
|
The theming decision is now made: Fastpotify will own a generic JSON palette format, starting with #392, and Omarchy integration can produce that format separately. I’m closing this direct colors/fonts integration so we have one implementation path. Thank you for the work and screenshots; the palette conversion work may still be useful on the Omarchy side. |
|
I see you added the theming support now. Awesome 😎 |
Visible changes
.wszskins remain unchanged.Why
Fastpotify can match an Omarchy desktop without maintaining a separate app theme or installing a theme-change hook.
This overlaps with the desktop-theming use case in #171. That PR proposes general custom schemes; this one reads Omarchy's existing palette and font selection directly.
What changed
Verification
Tested on Linux. Both test configurations pass 305 library tests and 3 executable tests. Windows and macOS were not tested locally.
All images below use the repository's demo fixtures, not a real account. The captures contain no textual or EXIF metadata and are hosted on a separate review-assets branch, outside this source diff.
Main window: before and after, dark and light
Mini-player: original skin, Tokyo Night, and Flexoki Light