-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Issue 3081 workspace color left rail #3082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -203,6 +203,22 @@ struct SidebarWorkspaceRowBackgroundStyle { | |||||||||||||||||||||||||||||||||||
| static let clear = Self(color: nil, opacity: 0) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func sidebarWorkspaceRowExplicitRailNSColor( | ||||||||||||||||||||||||||||||||||||
| activeTabIndicatorStyle: SidebarActiveTabIndicatorStyle, | ||||||||||||||||||||||||||||||||||||
| customColorHex: String?, | ||||||||||||||||||||||||||||||||||||
| colorScheme: ColorScheme | ||||||||||||||||||||||||||||||||||||
| ) -> NSColor? { | ||||||||||||||||||||||||||||||||||||
| guard activeTabIndicatorStyle == .leftRail, | ||||||||||||||||||||||||||||||||||||
| let customColorHex else { | ||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return WorkspaceTabColorSettings.displayNSColor( | ||||||||||||||||||||||||||||||||||||
| hex: customColorHex, | ||||||||||||||||||||||||||||||||||||
| colorScheme: colorScheme, | ||||||||||||||||||||||||||||||||||||
| forceBright: true | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func sidebarWorkspaceRowBackgroundStyle( | ||||||||||||||||||||||||||||||||||||
| activeTabIndicatorStyle: SidebarActiveTabIndicatorStyle, | ||||||||||||||||||||||||||||||||||||
| isActive: Bool, | ||||||||||||||||||||||||||||||||||||
|
|
@@ -223,24 +239,15 @@ func sidebarWorkspaceRowBackgroundStyle( | |||||||||||||||||||||||||||||||||||
| forceBright: activeTabIndicatorStyle == .leftRail | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| let selectedCustomBackground = customColorHex.flatMap { | ||||||||||||||||||||||||||||||||||||
| sidebarSelectedWorkspaceCustomBackgroundNSColor(hex: $0, colorScheme: colorScheme) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| switch activeTabIndicatorStyle { | ||||||||||||||||||||||||||||||||||||
| case .leftRail: | ||||||||||||||||||||||||||||||||||||
| if isActive { | ||||||||||||||||||||||||||||||||||||
| return SidebarWorkspaceRowBackgroundStyle( | ||||||||||||||||||||||||||||||||||||
| color: selectedCustomBackground ?? selectedBackground, | ||||||||||||||||||||||||||||||||||||
| color: selectedBackground, | ||||||||||||||||||||||||||||||||||||
| opacity: 1 | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if let customBackground { | ||||||||||||||||||||||||||||||||||||
| return SidebarWorkspaceRowBackgroundStyle( | ||||||||||||||||||||||||||||||||||||
| color: customBackground, | ||||||||||||||||||||||||||||||||||||
| opacity: isMultiSelected ? 0.35 : 0.7 | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if isMultiSelected { | ||||||||||||||||||||||||||||||||||||
| return SidebarWorkspaceRowBackgroundStyle(color: accentBackground, opacity: 0.25) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
@@ -249,7 +256,7 @@ func sidebarWorkspaceRowBackgroundStyle( | |||||||||||||||||||||||||||||||||||
| case .solidFill: | ||||||||||||||||||||||||||||||||||||
| if isActive { | ||||||||||||||||||||||||||||||||||||
| return SidebarWorkspaceRowBackgroundStyle( | ||||||||||||||||||||||||||||||||||||
| color: selectedCustomBackground ?? selectedBackground, | ||||||||||||||||||||||||||||||||||||
| color: selectedBackground, | ||||||||||||||||||||||||||||||||||||
| opacity: 1 | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
@@ -13073,6 +13080,10 @@ private struct TabItemView: View, Equatable { | |||||||||||||||||||||||||||||||||||
| .semibold | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private var showsLeadingRail: Bool { | ||||||||||||||||||||||||||||||||||||
| explicitRailColor != nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private var activeBorderLineWidth: CGFloat { | ||||||||||||||||||||||||||||||||||||
| switch activeTabIndicatorStyle { | ||||||||||||||||||||||||||||||||||||
| case .leftRail: | ||||||||||||||||||||||||||||||||||||
|
|
@@ -13501,6 +13512,16 @@ private struct TabItemView: View, Equatable { | |||||||||||||||||||||||||||||||||||
| RoundedRectangle(cornerRadius: 6) | ||||||||||||||||||||||||||||||||||||
| .strokeBorder(activeBorderColor, lineWidth: activeBorderLineWidth) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| .overlay(alignment: .leading) { | ||||||||||||||||||||||||||||||||||||
| if showsLeadingRail { | ||||||||||||||||||||||||||||||||||||
| Capsule(style: .continuous) | ||||||||||||||||||||||||||||||||||||
| .fill(railColor) | ||||||||||||||||||||||||||||||||||||
| .frame(width: 3) | ||||||||||||||||||||||||||||||||||||
| .padding(.leading, 4) | ||||||||||||||||||||||||||||||||||||
| .padding(.vertical, 5) | ||||||||||||||||||||||||||||||||||||
| .offset(x: -1) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| .padding(.horizontal, 6) | ||||||||||||||||||||||||||||||||||||
| .background { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -13943,6 +13964,21 @@ private struct TabItemView: View, Equatable { | |||||||||||||||||||||||||||||||||||
| return Color(nsColor: color).opacity(style.opacity) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private var railColor: Color { | ||||||||||||||||||||||||||||||||||||
| explicitRailColor ?? .clear | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private var explicitRailColor: Color? { | ||||||||||||||||||||||||||||||||||||
| guard let railColor = sidebarWorkspaceRowExplicitRailNSColor( | ||||||||||||||||||||||||||||||||||||
| activeTabIndicatorStyle: activeTabIndicatorStyle, | ||||||||||||||||||||||||||||||||||||
| customColorHex: workspaceSnapshot.customColorHex, | ||||||||||||||||||||||||||||||||||||
| colorScheme: colorScheme | ||||||||||||||||||||||||||||||||||||
| ) else { | ||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return Color(nsColor: railColor).opacity(0.95) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13972
to
+13980
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Inside
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private func tabColorSwatchColor(for hex: String) -> NSColor { | ||||||||||||||||||||||||||||||||||||
| WorkspaceTabColorSettings.displayNSColor( | ||||||||||||||||||||||||||||||||||||
| hex: hex, | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,7 @@ final class SidebarSelectedWorkspaceColorTests: XCTestCase { | |
| } | ||
|
|
||
| @MainActor | ||
| func testSetTabColorFeedsSelectedSolidFillSidebarBackground() { | ||
| func testSolidFillKeepsSelectedBackgroundForActiveCustomColoredWorkspaceRow() { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.tabs.first else { | ||
| XCTFail("Expected TabManager to initialise with a workspace") | ||
|
|
@@ -87,13 +87,16 @@ final class SidebarSelectedWorkspaceColorTests: XCTestCase { | |
| sidebarSelectionColorHex: nil | ||
| ) | ||
|
|
||
| XCTAssertEqual(background.color?.hexString(), "#C0392B") | ||
| XCTAssertEqual( | ||
| background.color?.hexString(), | ||
| sidebarSelectedWorkspaceBackgroundNSColor(for: .light).hexString() | ||
| ) | ||
| XCTAssertEqual(background.opacity, 1.0, accuracy: 0.001) | ||
| withExtendedLifetime(cancellable) {} | ||
| } | ||
|
|
||
| @MainActor | ||
| func testSetTabColorFeedsSelectedDefaultSidebarBackground() { | ||
| func testLeftRailKeepsSelectedBackgroundForActiveCustomColoredWorkspaceRow() { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.tabs.first else { | ||
| XCTFail("Expected TabManager to initialise with a workspace") | ||
|
|
@@ -119,10 +122,79 @@ final class SidebarSelectedWorkspaceColorTests: XCTestCase { | |
| sidebarSelectionColorHex: nil | ||
| ) | ||
|
|
||
| XCTAssertEqual(background.color?.hexString(), "#C0392B") | ||
| XCTAssertEqual( | ||
| background.color?.hexString(), | ||
| sidebarSelectedWorkspaceBackgroundNSColor(for: .light).hexString() | ||
| ) | ||
| XCTAssertEqual(background.opacity, 1.0, accuracy: 0.001) | ||
| withExtendedLifetime(cancellable) {} | ||
| } | ||
|
|
||
| @MainActor | ||
| func testLeftRailLeavesInactiveCustomColoredWorkspaceRowTransparent() { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.tabs.first else { | ||
| XCTFail("Expected TabManager to initialise with a workspace") | ||
| return | ||
| } | ||
|
|
||
| manager.setTabColor(tabId: workspace.id, color: "#C0392B") | ||
|
|
||
| let background = sidebarWorkspaceRowBackgroundStyle( | ||
| activeTabIndicatorStyle: .leftRail, | ||
| isActive: false, | ||
| isMultiSelected: false, | ||
| customColorHex: workspace.customColor, | ||
| colorScheme: .light, | ||
| sidebarSelectionColorHex: nil | ||
| ) | ||
|
|
||
| XCTAssertNil(background.color) | ||
| XCTAssertEqual(background.opacity, 0, accuracy: 0.001) | ||
| } | ||
|
|
||
| @MainActor | ||
| func testLeftRailResolvesExplicitRailColorForCustomColoredWorkspaceRow() { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.tabs.first else { | ||
| XCTFail("Expected TabManager to initialise with a workspace") | ||
| return | ||
| } | ||
|
|
||
| manager.setTabColor(tabId: workspace.id, color: "#C0392B") | ||
|
|
||
| let railColor = sidebarWorkspaceRowExplicitRailNSColor( | ||
| activeTabIndicatorStyle: .leftRail, | ||
| customColorHex: workspace.customColor, | ||
| colorScheme: .light | ||
| ) | ||
|
|
||
| XCTAssertNotNil(railColor) | ||
| XCTAssertEqual(railColor?.hexString(), "#C0392B") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test expects raw hex but function applies color brighteningMedium Severity The test Additional Locations (1)Reviewed by Cursor Bugbot for commit b32086f. Configure here. |
||
| } | ||
|
|
||
| @MainActor | ||
| func testSolidFillUsesInactiveCustomWorkspaceColorAsBackground() { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.tabs.first else { | ||
| XCTFail("Expected TabManager to initialise with a workspace") | ||
| return | ||
| } | ||
|
|
||
| manager.setTabColor(tabId: workspace.id, color: "#C0392B") | ||
|
|
||
| let background = sidebarWorkspaceRowBackgroundStyle( | ||
| activeTabIndicatorStyle: .solidFill, | ||
| isActive: false, | ||
| isMultiSelected: false, | ||
| customColorHex: workspace.customColor, | ||
| colorScheme: .light, | ||
| sidebarSelectionColorHex: nil | ||
| ) | ||
|
|
||
| XCTAssertEqual(background.color?.hexString(), "#C0392B") | ||
| XCTAssertEqual(background.opacity, 0.7, accuracy: 0.001) | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused
customBackgroundcomputation in leftRail branchLow Severity
The
customBackgroundvariable is computed for allactiveTabIndicatorStylevalues but is now only consumed in thesolidFillbranch. In theleftRailbranch it's unused dead computation. Additionally, theforceBright: activeTabIndicatorStyle == .leftRailexpression is misleading — it evaluates totrueonly in the path that never readscustomBackground, and is alwaysfalsewhen the variable is actually used, making it functionally equivalent toforceBright: false.Reviewed by Cursor Bugbot for commit b32086f. Configure here.