Skip to content

Commit 54a3408

Browse files
jasonlongjasonlong
committed
Preserve context while navigating notifications
1 parent 93d01ed commit 54a3408

2 files changed

Lines changed: 193 additions & 50 deletions

File tree

‎Octodot/Views/NotificationListView.swift‎

Lines changed: 103 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import SwiftUI
22

33
struct NotificationListView: View {
4+
private static let scrollCoordinateSpace = "notification-list-scroll"
5+
static let downwardContextAnchor = UnitPoint(x: 0.5, y: 0.35)
6+
47
let notifications: [GitHubNotification]
58
let selectedNotificationID: String?
69
let checkedIDs: Set<String>
@@ -24,17 +27,15 @@ struct NotificationListView: View {
2427
}
2528
}
2629

27-
enum ScrollPlacement: Equatable {
28-
case minimal
29-
case top
30-
}
31-
3230
struct ScrollRequest: Equatable {
31+
let selectedNotificationID: String
3332
let targetID: String
34-
let placement: ScrollPlacement
3533
let visibleIDs: [String]
3634
}
3735

36+
@State private var knownRowFrames: [String: CGRect] = [:]
37+
@State private var previousScrollRequest: ScrollRequest?
38+
3839
private var listItems: [ListItem] {
3940
Self.listItems(
4041
notifications: notifications,
@@ -44,33 +45,55 @@ struct NotificationListView: View {
4445
}
4546

4647
var body: some View {
47-
ScrollViewReader { proxy in
48-
ScrollView(.vertical) {
49-
LazyVStack(spacing: 0) {
50-
ForEach(listItems, id: \.id) { item in
51-
listItemView(item)
48+
let currentScrollRequest = Self.scrollRequest(
49+
selectedNotificationID: selectedNotificationID,
50+
notifications: notifications,
51+
groupByRepo: groupByRepo
52+
)
53+
54+
GeometryReader { viewportGeometry in
55+
ScrollViewReader { proxy in
56+
ScrollView(.vertical) {
57+
LazyVStack(spacing: 0) {
58+
ForEach(listItems, id: \.id) { item in
59+
listItemView(item)
60+
}
5261
}
5362
}
54-
}
55-
.task(id: Self.scrollRequest(
56-
selectedNotificationID: selectedNotificationID,
57-
notifications: notifications,
58-
groupByRepo: groupByRepo
59-
)) {
60-
guard let scrollRequest = Self.scrollRequest(
61-
selectedNotificationID: selectedNotificationID,
62-
notifications: notifications,
63-
groupByRepo: groupByRepo
64-
) else {
65-
return
63+
.coordinateSpace(name: Self.scrollCoordinateSpace)
64+
.onPreferenceChange(NotificationListRowFramesPreferenceKey.self) { frames in
65+
knownRowFrames.merge(frames) { _, latest in latest }
6666
}
67+
.task(id: currentScrollRequest) {
68+
guard let scrollRequest = currentScrollRequest else {
69+
previousScrollRequest = nil
70+
return
71+
}
72+
73+
let priorRequest = previousScrollRequest
74+
let priorRowFrame = priorRequest.flatMap {
75+
knownRowFrames[$0.selectedNotificationID]
76+
}
77+
previousScrollRequest = scrollRequest
78+
79+
await Task.yield()
80+
let currentRowFrame = knownRowFrames[scrollRequest.selectedNotificationID]
81+
let shouldRevealContext = Self.shouldRevealDownwardContext(
82+
previous: priorRequest,
83+
current: scrollRequest,
84+
previousRowFrame: priorRowFrame,
85+
currentRowFrame: currentRowFrame,
86+
viewportHeight: viewportGeometry.size.height
87+
)
88+
if shouldRevealContext {
89+
proxy.scrollTo(
90+
scrollRequest.selectedNotificationID,
91+
anchor: Self.downwardContextAnchor
92+
)
93+
return
94+
}
6795

68-
await Task.yield()
69-
switch scrollRequest.placement {
70-
case .minimal:
7196
proxy.scrollTo(scrollRequest.targetID)
72-
case .top:
73-
proxy.scrollTo(scrollRequest.targetID, anchor: .top)
7497
}
7598
}
7699
}
@@ -95,6 +118,18 @@ struct NotificationListView: View {
95118
onToggleCheck: { onToggleCheck(notification.id) }
96119
)
97120
.id(notification.id)
121+
.background {
122+
GeometryReader { geometry in
123+
Color.clear.preference(
124+
key: NotificationListRowFramesPreferenceKey.self,
125+
value: [
126+
notification.id: geometry.frame(
127+
in: .named(Self.scrollCoordinateSpace)
128+
)
129+
]
130+
)
131+
}
132+
}
98133
.onTapGesture {
99134
Self.handleRowTap(
100135
id: notification.id,
@@ -118,31 +153,48 @@ struct NotificationListView: View {
118153
static func scrollRequest(
119154
selectedNotificationID: String?,
120155
notifications: [GitHubNotification],
121-
groupByRepo: Bool
156+
groupByRepo _: Bool
122157
) -> ScrollRequest? {
123158
guard let selectedNotificationID,
124-
let selectedIndex = notifications.firstIndex(where: { $0.id == selectedNotificationID }) else {
159+
notifications.contains(where: { $0.id == selectedNotificationID }) else {
125160
return nil
126161
}
127162

128-
let targetID: String
129-
let placement: ScrollPlacement
130-
if groupByRepo,
131-
selectedIndex == 0 || notifications[selectedIndex - 1].repository != notifications[selectedIndex].repository {
132-
targetID = "repo:\(notifications[selectedIndex].repository)"
133-
placement = .top
134-
} else {
135-
targetID = selectedNotificationID
136-
placement = .minimal
137-
}
138-
139163
return ScrollRequest(
140-
targetID: targetID,
141-
placement: placement,
164+
selectedNotificationID: selectedNotificationID,
165+
targetID: selectedNotificationID,
142166
visibleIDs: notifications.map(\.id)
143167
)
144168
}
145169

170+
static func shouldRevealDownwardContext(
171+
previous: ScrollRequest?,
172+
current: ScrollRequest,
173+
previousRowFrame: CGRect?,
174+
currentRowFrame: CGRect?,
175+
viewportHeight: CGFloat
176+
) -> Bool {
177+
guard let previous,
178+
viewportHeight > 0,
179+
let previousIndex = previous.visibleIDs.firstIndex(of: previous.selectedNotificationID),
180+
let currentIndexInPreviousList = previous.visibleIDs.firstIndex(of: current.selectedNotificationID),
181+
currentIndexInPreviousList > previousIndex else {
182+
return false
183+
}
184+
185+
guard let previousRowFrame else { return false }
186+
187+
guard let currentRowFrame else {
188+
return true
189+
}
190+
if currentRowFrame.maxY > viewportHeight {
191+
return true
192+
}
193+
194+
let bottomTolerance = max(12, previousRowFrame.height * 0.5)
195+
return previousRowFrame.maxY >= viewportHeight - bottomTolerance
196+
}
197+
146198
static func listItems(
147199
notifications: [GitHubNotification],
148200
selectedNotificationID: String?,
@@ -175,3 +227,11 @@ struct NotificationListView: View {
175227
return items
176228
}
177229
}
230+
231+
private struct NotificationListRowFramesPreferenceKey: PreferenceKey {
232+
static var defaultValue: [String: CGRect] = [:]
233+
234+
static func reduce(value: inout [String: CGRect], nextValue: () -> [String: CGRect]) {
235+
value.merge(nextValue()) { _, latest in latest }
236+
}
237+
}

‎OctodotTests/AppShellTests.swift‎

Lines changed: 90 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,6 @@ struct AppShellTests {
255255
)
256256

257257
#expect(request?.targetID == "1")
258-
#expect(request?.placement == .minimal)
259258
#expect(request?.visibleIDs == ["0", "1", "2"])
260259
#expect(NotificationListView.scrollRequest(
261260
selectedNotificationID: "999",
@@ -281,7 +280,7 @@ struct AppShellTests {
281280
#expect(original != reordered)
282281
}
283282

284-
@Test func notificationListScrollRequestTargetsRepositoryHeaderForFirstRowInGroup() {
283+
@Test func notificationListScrollRequestKeepsSelectedRowTargetAcrossRepositoryBoundaries() {
285284
let notifications = [
286285
AppStateTests.makeNotification(id: 1, repo: "acme/alpha"),
287286
AppStateTests.makeNotification(id: 2, repo: "acme/alpha"),
@@ -304,12 +303,96 @@ struct AppShellTests {
304303
groupByRepo: true
305304
)
306305

307-
#expect(firstInFirstGroup?.targetID == "repo:acme/alpha")
308-
#expect(firstInFirstGroup?.placement == .top)
306+
#expect(firstInFirstGroup?.targetID == "1")
309307
#expect(secondInSameGroup?.targetID == "2")
310-
#expect(secondInSameGroup?.placement == .minimal)
311-
#expect(firstInSecondGroup?.targetID == "repo:acme/beta")
312-
#expect(firstInSecondGroup?.placement == .top)
308+
#expect(firstInSecondGroup?.targetID == "3")
309+
}
310+
311+
@Test func notificationListRevealsContextWhenMovingDownFromViewportBottom() {
312+
let notifications = AppStateTests.makeNotifications(4)
313+
let previous = NotificationListView.scrollRequest(
314+
selectedNotificationID: "1",
315+
notifications: notifications,
316+
groupByRepo: false
317+
)
318+
let current = NotificationListView.scrollRequest(
319+
selectedNotificationID: "2",
320+
notifications: notifications,
321+
groupByRepo: false
322+
)
323+
324+
#expect(NotificationListView.shouldRevealDownwardContext(
325+
previous: previous,
326+
current: current!,
327+
previousRowFrame: CGRect(x: 0, y: 356, width: 380, height: 44),
328+
currentRowFrame: CGRect(x: 0, y: 400, width: 380, height: 44),
329+
viewportHeight: 400
330+
))
331+
#expect(NotificationListView.shouldRevealDownwardContext(
332+
previous: previous,
333+
current: current!,
334+
previousRowFrame: CGRect(x: 0, y: 300, width: 380, height: 44),
335+
currentRowFrame: CGRect(x: 0, y: 400, width: 380, height: 44),
336+
viewportHeight: 400
337+
))
338+
#expect(NotificationListView.shouldRevealDownwardContext(
339+
previous: previous,
340+
current: current!,
341+
previousRowFrame: CGRect(x: 0, y: 348, width: 380, height: 44),
342+
currentRowFrame: nil,
343+
viewportHeight: 406
344+
))
345+
#expect(NotificationListView.shouldRevealDownwardContext(
346+
previous: previous,
347+
current: current!,
348+
previousRowFrame: CGRect(x: 0, y: 300, width: 380, height: 44),
349+
currentRowFrame: CGRect(x: 0, y: 344, width: 380, height: 44),
350+
viewportHeight: 400
351+
) == false)
352+
}
353+
354+
@Test func notificationListRevealsContextAfterActingOnBottomItem() {
355+
let notifications = AppStateTests.makeNotifications(4)
356+
let previous = NotificationListView.scrollRequest(
357+
selectedNotificationID: "1",
358+
notifications: notifications,
359+
groupByRepo: false
360+
)
361+
let current = NotificationListView.scrollRequest(
362+
selectedNotificationID: "2",
363+
notifications: [notifications[0], notifications[2], notifications[3]],
364+
groupByRepo: false
365+
)
366+
367+
#expect(NotificationListView.shouldRevealDownwardContext(
368+
previous: previous,
369+
current: current!,
370+
previousRowFrame: CGRect(x: 0, y: 356, width: 380, height: 44),
371+
currentRowFrame: CGRect(x: 0, y: 356, width: 380, height: 44),
372+
viewportHeight: 400
373+
))
374+
}
375+
376+
@Test func notificationListDoesNotRevealDownwardContextWhenMovingUp() {
377+
let notifications = AppStateTests.makeNotifications(4)
378+
let previous = NotificationListView.scrollRequest(
379+
selectedNotificationID: "2",
380+
notifications: notifications,
381+
groupByRepo: false
382+
)
383+
let current = NotificationListView.scrollRequest(
384+
selectedNotificationID: "1",
385+
notifications: notifications,
386+
groupByRepo: false
387+
)
388+
389+
#expect(NotificationListView.shouldRevealDownwardContext(
390+
previous: previous,
391+
current: current!,
392+
previousRowFrame: CGRect(x: 0, y: 356, width: 380, height: 44),
393+
currentRowFrame: CGRect(x: 0, y: 312, width: 380, height: 44),
394+
viewportHeight: 400
395+
) == false)
313396
}
314397

315398
@Test func notificationListBuildsRepositoryHeadersOnlyAtBoundaries() {

0 commit comments

Comments
 (0)