theme: make ThemeStyle and StatusColors fields constructable#2322
Draft
glani wants to merge 1 commit into
Draft
Conversation
Both ThemeStyle (the value type stored in SyntaxColors::keyword,
.string, .comment, etc.) and StatusColors::error keep their fields
private with no constructor, no public builder, and (for ThemeStyle)
no Default impl. The container type HighlightThemeStyle exposes its
syntax: SyntaxColors and status: StatusColors as pub fields, and
SyntaxColors's per-category fields are pub Option<ThemeStyle>, so a
downstream consumer can read theme.highlight_theme.style.syntax.keyword
and assign a Some(_) to it, but cannot construct the ThemeStyle value
to put inside the Some. The struct is reachable only via
Serialize/Deserialize round-trip through JSON.
Two consistency points argue for widening privacy:
1. The wasm_stub.rs counterpart already declares ThemeStyle with all
pub fields (highlighter/wasm_stub.rs:118-123). Native registry.rs
has been the outlier.
2. The encapsulation isn't load-bearing: there's no validation,
normalization, or invariant being protected. The fields are plain
Option<...> config slots.
This change widens privacy on the four fields a downstream theme
bridge needs to write programmatically: ThemeStyle.{color, font_style,
font_weight} and StatusColors.error. No semantic change, no signature
change, no test change. The remaining StatusColors fields (warning,
info, success, hint and their .background/.border variants) stay
private for now; widen as needed in a future PR.
Concrete downstream use case: a theme-bridge that maps an app-defined
htheme format's syntax.* block onto upstream's SyntaxColors:
let mut highlight = HighlightTheme::clone(&theme.highlight_theme);
let syntax = &mut highlight.style.syntax;
if let Some(s) = htheme.get("keyword") {
syntax.keyword = Some(ThemeStyle {
color: Some(s.fg),
font_weight: s.bold.then_some(FontWeightContent::Bold),
font_style: s.italic.then_some(FontStyle::Italic),
});
}
highlight.style.status.error = Some(htheme.error_color);
theme.highlight_theme = Arc::new(highlight);
Without this change the same code requires a JSON round-trip through
serde, which is brittle (no compile-time check on field names) and
pulls a serde_json dep into the consumer for what is structurally a
trivial value-construction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Widens four field-level privacies in
crates/ui/src/highlighter/registry.rs:ThemeStyle.color,ThemeStyle.font_style,ThemeStyle.font_weight→pubStatusColors.error→pubNo semantic change. No signature change. No test change. 4 lines.
Why
HighlightThemeStyleexposes itssyntax: SyntaxColorsandstatus: StatusColorsaspubfields, andSyntaxColors's per-category fields arepub Option<ThemeStyle>. A downstream consumer can readtheme.highlight_theme.style.syntax.keywordand even assign aSome(_)to it — but cannot construct theThemeStylevalue to put inside theSome. The fields are private, no constructor exists, noDefaultimpl exists, onlySerialize/Deserializederives.The result: programmatic construction of a
ThemeStylefrom outside the crate is impossible without a JSON round-trip through serde. That's brittle (no compile-time check on field names — a future field rename silently breaks downstream at runtime) and pulls aserde_jsondep into consumers who otherwise wouldn't need it.Two consistency arguments for widening:
ThemeStylewith allpubfields (crates/ui/src/highlighter/wasm_stub.rs:118-123). Nativeregistry.rsis the outlier — same logical type, different visibility surface depending on target.Option<...>config slots that get populated by deserialization and read byFrom<ThemeStyle> for HighlightStyle. Widening privacy doesn't bypass anything.Concrete downstream use case
A theme-bridge that maps an app-defined htheme format's
syntax.*block onto upstream'sSyntaxColors:Without this PR the same code path requires
serde_json::from_value(...)on a hand-built JSON object, which is a workable workaround but is strictly worse on every axis.Scope deliberately small
The remaining
StatusColorsfields (warning,info,success,hintand their.background/.bordervariants) stay private for now. They have the same encapsulation defect, but I've narrowed this PR to exactly the fields a real consumer needs to write today. Happy to widen the scope if you'd prefer all 15 in one go — just say the word.Status: draft
Marking draft until the downstream consumer (a heretic theme bridge in another workspace) verifies the patch enables the intended use case end-to-end. Will move to ready-for-review once that lands.