fix: adapt popups and panels to session theme for dark mode consistency#641
fix: adapt popups and panels to session theme for dark mode consistency#641kud wants to merge 6 commits into
Conversation
• Adjust glass effect tintColor values for better theme contrast in iTermOpenQuicklyView. • Apply current theme appearance to Quick Open window in iTermOpenQuicklyWindowController via it_appearanceForCurrentTheme. These improvements produce more consistent and visually pleasing Quick Open window theming across light and dark modes.
Adapt toast, global search, instant replay, and popup windows to inherit their dark/light appearance from the active session/theme, using background perceived brightness and improved theme helpers. - Added background/theme brightness detection for popups (ToastWindowController, iTermGlobalSearchWindowController, iTermInstantReplayWindowController, iTermPopupWindowController). - Imported NSAppearance+iTerm and NSColor+iTerm for centralised theme checks. - Ensured window material/appearance matches session, responding dynamically to dark/light modes. Reasons: - Improves visual consistency across secondary windows - Introduces session-aware theme logic via PTYSession and NSColor+iTerm - Refactors appearance logic to avoid hardcoded dark mode - files: sources/ToastWindowController.m, sources/iTermGlobalSearchWindowController.m, sources/iTermInstantReplayWindowController.m, sources/iTermPopupWindowController.m
gnachman
left a comment
There was a problem hiding this comment.
Thanks for the PR — this addresses a real issue with hardcoded dark appearance on popups.
A few things to address:
Toast text will be invisible in light mode
ToastWindowController.m:62 hardcodes [textField setTextColor:[NSColor whiteColor]] with a black drop shadow. With your change, light terminals now get NSVisualEffectMaterialSheet + NSAppearanceNameVibrantLight, which produces a light translucent background. White text on that will be unreadable. The text color and shadow need to adapt along with the appearance.
Toast fallback should match GlobalSearch/InstantReplay
The toast falls back to hardcoded YES when there's no session:
BOOL isDark = bgColor ? bgColor.perceivedBrightness < 0.5 : YES;But GlobalSearch and InstantReplay fall back to the theme preference:
BOOL isDark = bgColor ? bgColor.perceivedBrightness < 0.5 : [NSAppearance it_appearanceForCurrentTheme].it_isDark;Please make the toast use the same fallback for consistency.
Please revert the Open Quickly tint changes
The iTermOpenQuicklyView.m tint value changes are unrelated to the dark mode popup fix and modify values I just tweaked in 2bf7bbe27. Please drop those from this PR.
Ensure popup and toast windows inherit the correct dark/light appearance based on the session's effective background color, improving theme consistency during modal interactions and Sparkle update alerts. Refactor window appearance assignment to use session background color and perceived brightness. Add popupWindowBackgroundColor API to better coordinate colour queries between controllers and delegate objects. Update related logic in iTermPopupWindowController, ToastWindowController, iTermOpenQuicklyWindowController, and iTermApplicationDelegate. Addresses inconsistent UI theming when system theme and session differ.
|
Shall be good |
gnachman
left a comment
There was a problem hiding this comment.
Thanks for working on this — the core idea of using session background color to determine darkness is the right approach for the minimal theme case, and matches what PseudoTerminal already does for its own alert sheets.
A few things I'd like to see addressed:
Extract the repeated detection pattern
The same 4-line pattern is copy-pasted in 6 places:
PTYSession *session = [[iTermController sharedInstance] currentTerminal].currentSession;
NSColor *bgColor = session.effectiveUnprocessedBackgroundColor;
BOOL isDark = bgColor ? bgColor.perceivedBrightness < 0.5 : [NSAppearance it_appearanceForCurrentTheme].it_isDark;
self.window.appearance = [NSAppearance appearanceNamed:isDark ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua];This should be extracted into a helper — e.g., a class method on NSAppearance+iTerm like +it_appearanceForCurrentSession — so there's a single source of truth.
Drop the Sparkle theming
Sparkle windows are system-level dialogs with no meaningful relationship to a terminal session. Grabbing whatever session happens to be current is arbitrary, and the flag-based notification dance is fragile (any unrelated window becoming key between the delegate callback and the Sparkle alert appearing would get its appearance overridden instead). The system appearance / it_appearanceForCurrentTheme is the right choice for standalone panels like these — it's what users expect from a standard macOS dialog.
Open Quickly is similar
Open Quickly is a global navigation panel (like Spotlight) — not tied to any particular session. The "current" session is arbitrary since you might be invoking it precisely to switch away. I'd use it_appearanceForCurrentTheme here too, same as before this PR.
Make popupWindowBackgroundColor @optional
The method is added to PopupDelegate without @optional. Since the popup controller already nil-checks the result, making it @optional with a respondsToSelector: guard would be more defensive and consistent with Cocoa conventions.
Minor: inconsistent isDark check in popup controller
iTermPopupWindowController.m uses bgColor.isDark while everywhere else uses bgColor.perceivedBrightness < 0.5. They're equivalent, but the fallback also differs (owningWindow.effectiveAppearance.it_isDark vs it_appearanceForCurrentTheme). Extracting the helper (point 1) would resolve this naturally.
The popup/toast/instant replay/global search changes all look good conceptually — just needs the cleanup above.
|
@gnachman I start wondering if we should have a Minimal Dark and Minimal Light theme to ease the conditions instead of checking the background to understand the Minimal theme. Because on the theme, there is also a proper Dark and Light theme. |
|
I'm not that tempted to make this any more complicated than it already is, and the current minimal looks pretty cool IMO so I don't want to change it to a straight light/dark. That said, light vs dark could be decoupled from theme. That would benefit Compact, which does not have light and dark variants as Regular does, and would let us have an app-wide theme separate from the system light/dark theme for Minimal. I haven't pursued that becuase this is already so complex and the benefit is small. |
|
@gnachman In this case, a helper to determine the theme thanks to the background for Compact and Minimal would work, but I do admit that even for the user, it's confusing to understand how you choose the dark theme when you select Compact and Minimal. Maybe a note in the app might be helpful. |
…e helper Extract repeated isDark detection into +it_appearanceForCurrentSessionWithBackgroundColor: on NSAppearance+iTerm. Minimal/Compact themes use session background brightness; Regular/Automatic use it_appearanceForCurrentTheme. Make popupWindowBackgroundColor optional in PopupDelegate with respondsToSelector guard. Replace 4-line copy-pasted pattern in iTermPopupWindowController, iTermInstantReplayWindowController, iTermGlobalSearchWindowController, ToastWindowController with the new helper. Revert iTermOpenQuicklyWindowController to it_appearanceForCurrentTheme (global panel, not session-tied). Revert Sparkle theming in iTermApplicationDelegate (fragile, arbitrary)—keep tab style observer.
|
All addressed in 72a4b26:
|
|
The new helper actually does a small version of this — |
gnachman
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the visual inconsistency in Minimal theme with a dark session + light system appearance is a real problem and the approach of consolidating into a helper is the right one. A few things to address:
Compact theme should not use session background color
The new helper groups TAB_STYLE_COMPACT with TAB_STYLE_MINIMAL, making both session-aware. Only Minimal theme should derive appearance from the session background color. Compact (and all other themes) should respect the global light/dark theme, matching the existing behavior in it_decorationsAreDarkWithTerminalBackgroundColorIsDark:.
Move TAB_STYLE_COMPACT to the fallthrough group:
+ (instancetype)it_appearanceForCurrentSessionWithBackgroundColor:(nullable NSColor *)backgroundColor {
iTermPreferencesTabStyle preferredStyle = [iTermPreferences intForKey:kPreferenceKeyTabStyle];
switch (preferredStyle) {
case TAB_STYLE_MINIMAL:
if (backgroundColor) {
BOOL isDark = backgroundColor.perceivedBrightness < 0.5;
return [NSAppearance appearanceNamed:isDark ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua];
}
return NSApp.effectiveAppearance;
case TAB_STYLE_COMPACT:
case TAB_STYLE_AUTOMATIC:
case TAB_STYLE_LIGHT:
case TAB_STYLE_LIGHT_HIGH_CONTRAST:
case TAB_STYLE_DARK:
case TAB_STYLE_DARK_HIGH_CONTRAST:
return [self it_appearanceForCurrentTheme];
}
}Remove redundant popupWindowBackgroundColor in iTermStatusBarLargeComposerViewController
Since the method is @optional in the protocol and the caller guards with respondsToSelector:, not implementing the method produces the identical result (bgColor = nil → global theme fallback). The explicit return nil implementation is unnecessary — just remove it and let the @optional mechanism handle it.
Minor nits
- Method naming:
it_appearanceForCurrentSessionWithBackgroundColor:takes a color, not a session. Something likeit_appearanceForSessionBackgroundColor:would be more precise. - The import removal in
iTermOpenQuicklyWindowController.mis fine — confirmed no category methods are used.
Summary
Fix dark mode appearance inconsistency across all secondary windows and popups. The root issue: many panels hardcoded
NSAppearanceNameVibrantDarkor usedit_appearanceForCurrentTheme, which only reads the macOS system appearance. In the minimal theme with a dark session background, the system appearance can still be "light" — so these windows rendered in light style even though the terminal looked dark.The fix: detect darkness from
session.effectiveUnprocessedBackgroundColor.perceivedBrightnessfirst, falling back toit_appearanceForCurrentTheme.it_isDarkonly when there is no session context. This is the same patternPseudoTerminal.malready uses for its own alert sheets.Changes
ToastWindowControllerNSAppearanceNameVibrantDarkandNSVisualEffectMaterialHUDWindowisDarkdetection before the text field setup (was computed after, causing white text on light background)YEStoit_appearanceForCurrentTheme.it_isDarkiTermGlobalSearchWindowControllerwindow.appearanceinwindowDidLoadbased on session background brightnessit_appearanceForCurrentTheme.it_isDarkiTermInstantReplayWindowControllerwindowDidLoadiTermPopupWindowController+PopupDelegateprotocol- (NSColor *)popupWindowBackgroundColortoPopupDelegate, implemented inPTYSession(returnseffectiveUnprocessedBackgroundColor) andiTermStatusBarLargeComposerViewController(returnsnil→ falls back toowningWindow.effectiveAppearance)popWithDelegate:inWindow:uses the delegate's bg colour for dark detection instead ofowningWindow.effectiveAppearance.it_isDarkiTermOpenQuicklyWindowControllerit_appearanceForCurrentThemewith the same session-brightness detectioniTermApplicationDelegate(Sparkle update window)_waitingForSparkleWindowflag and#pragma mark - SUUpdaterDelegatesectionupdater:didFindValidUpdate:andupdaterWillShowModalAlert:set the flag;updaterDidShowModalAlert:clears itwindowDidChangeKeyStatus:observer applies the correct appearance to whatever window Sparkle makes key nextTesting