Circular sleep chart displays as if it is a clock#2
Circular sleep chart displays as if it is a clock#2Tom4259 wants to merge 11 commits intoDanielJamesTronca:mainfrom
Conversation
…ecting sleep stages
There was a problem hiding this comment.
Pull request overview
Adjusts the circular sleep chart so its start/end indicators align with clock positions (e.g., sleep starting at ~11pm appears near 11 o’clock), alongside several UI/style and API enhancements.
Changes:
- Rotates circular chart segments based on the first sample’s start time; updates iconography and default threshold hours.
- Updates legend layout to be more responsive and optionally hide durations; adds a new
.timelineNoDurationsstyle. - Improves timeline connectors (line width/opacity/offset and stage-to-stage color gradient) and adds
Codableconformance to core models; bumps minimum platform versions.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/SleepChartKit/Utils/SleepChartConstants.swift | Tweaks legend spacing and connector styling constants. |
| Sources/SleepChartKit/Services/SleepStageColorProvider.swift | Changes default unspecified-stage color and introduces a custom color provider. |
| Sources/SleepChartKit/Models/SleepStage.swift | Adds Codable conformance. |
| Sources/SleepChartKit/Models/SleepSample.swift | Adds Codable conformance. |
| Sources/SleepChartKit/Models/SleepChartStyle.swift | Adds .timelineNoDurations style. |
| Sources/SleepChartKit/Components/SleepTimelineGraph.swift | Adds stage-aware gradient connectors and adjusts connector geometry. |
| Sources/SleepChartKit/Components/SleepLegendView.swift | Switches to ViewThatFits + LazyHGrid approach; adds hideDurations. |
| Sources/SleepChartKit/Components/SleepCircularChartView.swift | Rotates start angle based on sleep start time; changes default threshold and start icon. |
| Sources/SleepChartKit/Components/SleepChartView.swift | Wires .timelineNoDurations into legend rendering. |
| Package.swift | Raises minimum supported platform versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //making the chart look like the sleep starts at it would on a clock | ||
| let sleepStart = samples.first!.startDate | ||
|
|
There was a problem hiding this comment.
In sleepSegments, samples.first! is force-unwrapped even though the method already guards against an empty array. Consider using the already-validated first sample from the guard (or guard let sleepStart = samples.first?.startDate else { ... }) to avoid unnecessary force unwraps and keep the control flow clearer.
| public var body: some View { | ||
| HStack(spacing: 4) { | ||
| // Color indicator circle |
There was a problem hiding this comment.
LegendItem uses a hard-coded HStack(spacing: 4) while overall legend spacing is now controlled by SleepChartConstants.legendItemSpacing. Consider introducing a dedicated constant for “within-item” spacing (or reusing an existing one) instead of a magic number so layout tuning remains centralized.
| from: prevRect, | ||
| to: currentRect | ||
| to: currentRect, | ||
| fromStage: previousStage, |
There was a problem hiding this comment.
In the connector rendering call, previousStage is already unwrapped into prevStage, but the code still passes previousStage (optional) into fromStage:. This makes the API/typing noisier than necessary. Consider passing prevStage and changing renderStageConnector to take non-optional SleepStage parameters (since the connector is only drawn when both stages exist).
| fromStage: previousStage, | |
| fromStage: prevStage, |
| context.stroke( | ||
| connectorPath, | ||
| with: .color(.gray.opacity(SleepChartConstants.connectorOpacity)), | ||
| with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2), |
There was a problem hiding this comment.
The linear gradient for the connector stroke uses controlPoint1/controlPoint2 as the gradient’s start/end points. That makes the gradient direction dependent on curve control points rather than the actual connector endpoints, which can look incorrect when stage durations/positions vary. Consider using startPoint and endPoint for the gradient vector so the color transition aligns with the connector’s start/end stages.
| with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2), | |
| with: .linearGradient(gradient, startPoint: startPoint, endPoint: endPoint), |
| .iOS(.v16), | ||
| .macOS(.v13), | ||
| .watchOS(.v9), | ||
| .tvOS(.v16) |
There was a problem hiding this comment.
This PR bumps the minimum deployment targets (e.g., iOS 15 → 16, macOS 12 → 13). That’s a breaking change for package consumers; please confirm it’s required (e.g., for ViewThatFits) and call it out explicitly in the PR description / release notes so downstream apps can plan accordingly.
| .iOS(.v16), | |
| .macOS(.v13), | |
| .watchOS(.v9), | |
| .tvOS(.v16) | |
| .iOS(.v15), | |
| .macOS(.v12), | |
| .watchOS(.v8), | |
| .tvOS(.v15) |
| //making the chart look like the sleep starts at it would on a clock | ||
| let sleepStart = samples.first!.startDate | ||
|
|
||
| var h: Double = Double(Calendar.current.component(.hour, from: sleepStart)) | ||
| if h >= 12 { h -= 12 } | ||
|
|
||
| let m: Double = Double(Calendar.current.component(.minute, from: sleepStart)) | ||
|
|
||
| var difference: Double = (360 / 12) * h | ||
| difference += (360 / 12) * (m / 60) | ||
|
|
||
| currentAngle += difference |
There was a problem hiding this comment.
The new “rotate chart based on sleep start time” logic changes the circular chart’s core positioning behavior, but there’s no unit coverage validating the resulting start angle (existing tests only assert the view can be created). Consider extracting the angle/segment calculation into a testable helper (internal function/type) and adding tests for representative start times (e.g., 23:00 → near 11 o’clock, 07:00 → near 7 o’clock).
| return .indigo | ||
| case .asleepUnspecified: | ||
| return .purple | ||
| return Color(UIColor.lightGray) |
There was a problem hiding this comment.
DefaultSleepStageColorProvider uses UIColor.lightGray, but this package declares macOS/watchOS support where UIColor isn’t available. This will fail to compile on non-UIKit platforms; use a cross-platform Color (e.g., .gray/.secondary) or wrap in #if canImport(UIKit)/#elseif canImport(AppKit) with UIColor/NSColor as appropriate.
| return Color(UIColor.lightGray) | |
| return .gray |
| showLabels: Bool = true, | ||
| showIcons: Bool = true, | ||
| thresholdHours: Double = 9.0 | ||
| thresholdHours: Double = 12 | ||
| ) { |
There was a problem hiding this comment.
The default thresholdHours value was changed to 12, but the stored property comment and initializer docs still say the default is 9 hours. Please update the documentation to match the new default (and/or explain why 12 is now the default).
| var awakeColour: Color | ||
| var remColour: Color | ||
| var coreColour: Color | ||
| var deepColour: Color | ||
| var unspecifiedColour: Color | ||
| var inBedColour: Color | ||
|
|
||
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | ||
|
|
||
| self.awakeColour = awake ?? .orange | ||
| self.remColour = REM ?? .cyan | ||
| self.coreColour = core ?? .blue | ||
| self.deepColour = deep ?? .indigo | ||
| self.unspecifiedColour = unspecified ?? .purple | ||
| self.inBedColour = inBed ?? .gray | ||
| } | ||
|
|
||
| public func color(for stage: SleepStage) -> Color { | ||
|
|
||
| switch stage { | ||
|
|
||
| case .awake: return awakeColour | ||
| case .asleepREM: return remColour | ||
| case .asleepCore: return coreColour | ||
| case .asleepDeep: return deepColour | ||
| case .asleepUnspecified: return unspecifiedColour | ||
| case .inBed: return inBedColour |
There was a problem hiding this comment.
CustomSleepStageColorProvider exposes a public API with inconsistent naming (awakeColour/remColour vs the rest of the codebase’s Color terminology) and a parameter label REM that breaks Swift API design guidelines (capitalized acronym as an external label is unusual). Consider renaming to awakeColor, remColor, etc., and using a rem: parameter label for consistency and discoverability.
| var awakeColour: Color | |
| var remColour: Color | |
| var coreColour: Color | |
| var deepColour: Color | |
| var unspecifiedColour: Color | |
| var inBedColour: Color | |
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.awakeColour = awake ?? .orange | |
| self.remColour = REM ?? .cyan | |
| self.coreColour = core ?? .blue | |
| self.deepColour = deep ?? .indigo | |
| self.unspecifiedColour = unspecified ?? .purple | |
| self.inBedColour = inBed ?? .gray | |
| } | |
| public func color(for stage: SleepStage) -> Color { | |
| switch stage { | |
| case .awake: return awakeColour | |
| case .asleepREM: return remColour | |
| case .asleepCore: return coreColour | |
| case .asleepDeep: return deepColour | |
| case .asleepUnspecified: return unspecifiedColour | |
| case .inBed: return inBedColour | |
| var awakeColor: Color | |
| var remColor: Color | |
| var coreColor: Color | |
| var deepColor: Color | |
| var unspecifiedColor: Color | |
| var inBedColor: Color | |
| public init(awake: Color? = nil, rem: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.awakeColor = awake ?? .orange | |
| self.remColor = rem ?? .cyan | |
| self.coreColor = core ?? .blue | |
| self.deepColor = deep ?? .indigo | |
| self.unspecifiedColor = unspecified ?? .purple | |
| self.inBedColor = inBed ?? .gray | |
| } | |
| @available(*, deprecated, message: "Use init(awake:rem:core:deep:unspecified:inBed:) instead.") | |
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.init( | |
| awake: awake, | |
| rem: REM, | |
| core: core, | |
| deep: deep, | |
| unspecified: unspecified, | |
| inBed: inBed | |
| ) | |
| } | |
| public func color(for stage: SleepStage) -> Color { | |
| switch stage { | |
| case .awake: return awakeColor | |
| case .asleepREM: return remColor | |
| case .asleepCore: return coreColor | |
| case .asleepDeep: return deepColor | |
| case .asleepUnspecified: return unspecifiedColor | |
| case .inBed: return inBedColor |
DanielJamesTronca
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I reviewed this in a clean copy and I can't merge it as-is because it breaks existing package contracts and currently fails to build.
Blocking issues:
- The PR no longer builds cleanly in a fresh review copy:
Sources/SleepChartKit/Services/SleepStageColorProvider.swiftusesColor(UIColor.lightGray), which fails withcannot find 'UIColor' in scope. That breaks the package's cross-platform promise. Package.swiftraises the minimum supported platforms from iOS 15 / macOS 12 / watchOS 8 / tvOS 15 to iOS 16 / macOS 13 / watchOS 9 / tvOS 16. That is a breaking compatibility change and it is not required for the stated PR goal.- The circular chart currently represents progress toward a threshold. This PR changes existing default behavior by rotating the chart to wall-clock positions and by changing the default
thresholdHoursfrom9.0to12. Existing adopters would get different output without opting in. .timelineNoDurationsis additive, but the naming/behavior contract is not clear enough yet. The diff also mixes legend visibility and duration visibility in a way that needs to be made explicit.
What I'd like to see instead:
- Keep current package platform minimums unchanged in this PR.
- Keep current circular-chart defaults unchanged.
- If you want clock-face rendering, add it as a new opt-in mode instead of changing existing semantics.
- Replace the
UIColorusage with a cross-platform-safeColorvalue. - Narrow the PR to non-breaking additive changes only.
Happy to re-review a revised version with those changes.
For example sleeping at 11pm will have the bed icon just to the left of the top middle, and waking up at 7am will show just to the left of the bottom middle