🐛 fix(tab-groups): inherit window appearance in group menu and name dialog - #727
Draft
kud wants to merge 2 commits into
Draft
🐛 fix(tab-groups): inherit window appearance in group menu and name dialog#727kud wants to merge 2 commits into
kud wants to merge 2 commits into
Conversation
- Compute isDark from the minimal tab style's background color (or window's effective appearance otherwise) and force the rename-tab alert's window appearance to match, so it isn't mismatched against a colored minimal tab bar - Apply the same logic to the tab group context menu, deriving appearance from it_terminalWindowDecorationBackgroundColor's perceived brightness when minimal style is active, falling back to the window's own appearance otherwise
- Appends a Bug Fixes entry to docs/notes-3.7.txt - Documents that the tab group context menu and the New Tab Group / Rename Tab Group dialogs now match the window's light or dark appearance, including under the Minimal theme
Owner
|
This looks good to me. Both hunks are faithful copies of the precedents you cited (the per-tab menu and the tab-title dialog), every symbol they use is already in scope in that file, and neither touches a return contract or call site. It closes a real gap: the tab-group menu and name dialog genuinely don't inherit the window appearance on master today. Do you have any reservations before marking it ready? In particular, were you able to confirm the build and take a look at the color swatches under the dark appearance that you flagged? |
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.
Note
Draft on purpose: I haven't compiled this locally yet, so treat it as a proposal on shape rather than something to pull. I'll build and confirm the behaviour before marking it ready for review.
📄 Description
The tab group UI added in the recent tab-groups work doesn't inherit the window's appearance, so on a dark window the group context menu and the group name dialog both render in the light system appearance. The per-tab equivalents already handle this.
Two omissions, each fixed by applying the precedent that already exists a few hundred lines away in the same file:
-tabView:menuForTabGroup:-tabView:menuForTabViewItem:(PseudoTerminal.m:7975)-promptForTabGroupName:title:-openEditTabTitleWindow(PseudoTerminal.m:8375)Two notes on why it's a copy rather than a shared helper:
it_terminalWindowUseMinimalStyleand the decoration background's perceived brightness; the alert path keys offkPreferenceKeyTabStyle == TAB_STYLE_MINIMALandminimalTabStyleBackgroundColor.isDark. Same intent, different predicates, and swapping them would change behaviour under Compact and Automatic. Each site copies its own matching precedent.-promptForTabGroupName:title:is shared by both "New Tab Group" (-addTabToNewGroup:) and "Rename Tab Group" (-renameTabGroupWithID:), so the single insertion covers both dialogs.The colour swatch view (
ColorsMenuItemView) inserted into the group menu is a custom-drawn subview, so setting the menu's appearance should cover it without a separate change. That's the one part I want to confirm visually rather than assert.🔍 Scope
Out of scope, flagged rather than fixed:
WindowArrangements.m:296— the replace-confirmation alert sets no appearance either, but it predates tab groups and is shared with "Save Window as Arrangement".-closeTabs:confirmWith:skippingPinned:is fine already; it routes throughiTermWarningwithwindow:self.window, so it's a sheet on the window.✅ How to Validate
🛠️ Developer Checklist