windows: zero config comes up correct, and stops crashing on first run - #755
Open
deblasis wants to merge 4 commits into
Open
windows: zero config comes up correct, and stops crashing on first run#755deblasis wants to merge 4 commits into
deblasis wants to merge 4 commits into
Conversation
createFileAbsolute's handle was dropped on the floor rather than closed. On POSIX that costs nothing. On Windows the create denies read sharing for as long as the handle lives, so every later read of the config in the same process fails with a sharing violation on a file nothing else has open. On a first run that is every read there is: the config does not exist, the open creates it, and the app dies with 0xC000027B before it draws.
Upstream ships no default theme: with an empty config the terminal comes up on the compile-time colours, and the surrounding chrome is left to guess. That is fine when the terminal is the whole window. It is not fine here, where the terminal sits inside chrome that has to agree with it, and a "zero configuration" install is the common case rather than the exception. wintty_theme.zig holds the pair, derived from the logo and checked by wintty_theme_test.zig: every slot the terminal actually renders text in clears WCAG AA against its background, with palette 0 exempted since it is the shadow colour and only has to stay distinguishable. It is applied through the same overlay a user theme file goes through, so precedence is unchanged: an explicit theme wins, an explicit setting wins over the theme, and nothing at all now lands on the pair instead of on the compile-time defaults. window-theme moves from auto to system at the same time, since auto derives the window theme from the background, which now follows the desktop, and would fight it. ghostty_config_set_color_scheme lets the host say which half to take before finalize. ghostty_config_builtin_theme hands the same text out so the host can read colours it needs before a surface exists.
The shell now tells the config which half of the built-in pair to take,
before finalize, and re-tells it on reload. With nothing configured the
terminal, the chrome and the splash all come from the same source and move
together when the desktop setting changes.
What this fixes, measured on a light desktop with an empty config:
window chrome glyphs 1.87:1 -> 6.56:1 and up
splash ink a fixed step in linear luminance -> a fixed step
in L*, so it reads the same in both halves
The chrome number came from Mica tinting off the desktop while the window
element theme was derived from palette luminance. With window-theme now
system, both sides read the desktop and the split is gone.
ThemeResolution gains StepLightness, which walks CIE L* rather than
multiplying luminance, so "one step lighter" means the same thing on a
light background as on a dark one. LaunchTexture uses it for the splash
ink; the old fixed contrast ratio produced a visible step in one half and
a nearly invisible one in the other.
ConfigService's ReadFlags is now guarded: it ran unguarded in the
constructor, so any throw there took the app down before it drew, which is
how the first-run file sharing violation presented. Its palette fallbacks
also move to libghostty's own defaults, since the previous ones were a
different theme's colours and only showed up when everything else failed.
IThemeProvider loses the resolved colours and the font. Nothing read them
and nothing refreshed them, so they sat at fixed values regardless of
config, waiting for a first caller to trust them.
Palette. In the light half, slot 0 and slot 15 were the same colour, and slot 7 was dark. Anything pairing black with bright white rendered invisible, and `ESC[47m` gave a dark background carrying dark text. The cause was the test: it held every slot to a contrast ratio against the background and exempted only slot 0, which is the near-background slot in a DARK theme. Clearing that rule is what pushed the light half's white slots dark. The rule is polarity-aware now: fills are slot 0 in the dark half and slots 7 and 15 in the light one, and a fill is held to the opposite pair of rules -- distinguishable from the background, and readable with foreground text on top. Slot 0 against slot 15 is pinned separately. Verified by restoring the old palette and watching it fail. The test could also pass on a theme with a line missing: the non-palette colours defaulted to undefined, and 0xAA-filled bytes clear every ratio it asserts. They are optional and required now. C API. ghostty_config_set_color_scheme returns bool and refuses after finalize instead of accepting and doing nothing. Accepting was worse than useless: the recorded scheme would then disagree with the colours already resolved, and the next real desktop change would compare equal to it, be dropped as "no change", and leave the config stuck on the wrong half until the user flipped twice. ghostty_config_builtin_theme returns static storage through the one ABI type that everywhere else means "you own this". Both the header and the managed import now say so, since freeing it hands the allocator a pointer it never owned. ghostty_config_theme_is_builtin is new, and fixes a real regression: the chrome decided whether the built-in pair applied by reading `theme` out of the top-level config file, but `theme` can be set in a file reached through `config-file`. That combination rendered the terminal in the user's theme and painted the chrome from the built-in pair. Windows. A failed ReadFlags in the constructor left the "which scheme are my caches for" flag at its default, which on a light desktop is accidentally correct, so the retry guard declined every retry for the life of the process. It now points at the scheme we do not have. Reload sampled the desktop scheme twice with a config rebuild in between; it samples once. ConfigIniFile.Load opens with FileShare.ReadWrite rather than File.ReadLines' default of FileShare.Read, since this file has writers. edit.zig's comment named the wrong mechanism. Both create and open pass FILE_SHARE_READ|WRITE|DELETE, so a leaked handle cannot block another Zig reader. What it holds is GENERIC_WRITE, which locks out a reader asking for FILE_SHARE_READ alone -- the .NET default, and the host reads this file immediately after asking for its path. Also: the perceptual-distance test asserted a symmetric window around the target, but the step is a whole number of counts and near black one count is worth over half a unit of L*. It now asserts at-or-past and no more than one count past, with a near-black row that would have failed the old bound. The direction test was satisfied by ink equal to the background. The luminance formula had a third copy. StepLightness' doc promised a tint that never equals its input and never drifts in hue; both stop holding once a channel clamps, so it says what it does.
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
1 participant
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.
A fresh install with no config crashed before it drew, and once it drew, the
chrome did not match the terminal. Both are fixed here.
The crash
edit.zig'sopenPathcreated the config file and dropped the handle withoutclosing it. On Windows the create denies read sharing for as long as the handle
lives, so every later read in the same process fails with a sharing violation.
On a first run that is every read there is, and the app exits 0xC000027B.
ConfigService.ReadFlagsalso ran unguarded in the constructor, so the throwtook the app down rather than falling back. It is guarded now.
The colours
Upstream ships no default theme: an empty config lands on the compile-time
colours and the chrome is left to guess.
wintty_theme.zigadds a light/darkpair derived from the logo, applied through the same overlay a user theme file
goes through, so precedence is unchanged. An explicit theme still wins, an
explicit setting still wins over the theme.
window-thememoves fromautotosystemat the same time.autoderivesthe window theme from the terminal background, which now follows the desktop,
so it would fight it.
Measured on a light desktop, empty config:
The 1.87:1 came from Mica tinting off the desktop while the window element
theme came from palette luminance. Both read the desktop now.
ThemeResolution.StepLightnesswalks CIE L* instead of multiplying luminance,so "one step lighter" means the same thing against a light background as a dark
one. The splash uses it; the old fixed ratio gave a visible step in one half and
a nearly invisible one in the other.
Also
IThemeProviderloses its resolved colours and font. Nothing read them andnothing refreshed them, so they sat at fixed values from a different theme
regardless of config, waiting for a first caller to trust them.
Two new exports:
ghostty_config_set_color_scheme(which half to take, beforefinalize) and
ghostty_config_builtin_theme(the same text, for colours thehost needs before a surface exists).
Testing
zig fmt --checkclean. 2397 managed tests pass.wintty_theme_test.zigasserts WCAG AA for every slot the terminal renders text in, with palette 0
exempted as the shadow colour that only has to stay distinguishable.
Verified by screenshot in both halves and both tab orientations.
Stack
# 755zero config, this branch's base for the rest# 756folder tab and seam covers# 757seam diagnosticMerge in order. Each part is under the pr-gate size cap on its own.
Review
Three subagent reviews (ghostty core, .NET/Windows interop, WinUI shell) ran
over this stack. Their findings are fixed in the last commit, which is most
of the size below: a light-half palette collision that rendered black on
bright white invisible, a test oracle that was polarity-blind and could pass
on a theme with a line missing, an ownership hazard on the new string export,
a chrome/terminal split when
themeis set in a file reached throughconfig-file, and a failed first read that wedged the retry guard shut.Size-override: the review fixes for this work belong with the code they
correct, and splitting them out would separate every fix from what it fixes.
The remaining split is by language rather than by concern.