From 7f0f5771135f60fcf5199c1c45b8a6ae6d47b83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20D=E2=80=99Aquino?= Date: Mon, 9 Sep 2024 18:01:38 -0700 Subject: [PATCH] Save scroll position in timeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit implements a way to save the scroll position within a timeline. It does that by assigning each note in the timeline an ID based on the NoteID, and adding a `scrollPosition` modifier along with SceneStorage to keep track of and persist the scroll position along the timeline view throughout a single app session. Notes: - Scroll position is not persisted across app restarts. When the user completely quits the app, scroll position is lost. - This works on home feed and universe view. However, due to how Universe view is dynamically loaded, performance may not be as good as on the home feed - This only works on iOS 17 and higher, since the necessary scroll position reading mechanism is only available in those versions. On older versions things should work as before this change. Testing ------- PASS Damus: This commit iOS: 17.6.1 Device: iPhone 13 mini Steps: 1. Scroll down home feed to a note with a memorable image 2. Switch to the notifications tab 3. Switch back to the home tab. Ensure scroll position is at the memorable image (or close). PASS 4. Navigate into another profile from the home feed 5. Go back to the home feed by clicking the "back" button on the top left. Ensure scroll position is preserved. PASS 6. Navigate into another profile from the home feed again. 7. Go back to the home feed by clicking the home button at the bottom tab bar. Ensure scroll position is preserved. PASS 8. Click on the home button at the bottom tab bar while at the home feed. You should be taken to the top. PASS iOS 16 regression testing ------------------------- PASS Damus: This commit iOS: 16.4 Device: iPhone SE simulator Steps: 1. Navigate through the home feed, navigate between tabs 2. Ensure there are no visible regressions on navigation. PASS Changelog-Fixed: Fixed situations where scroll position would be lost (iOS 17 only) Closes: https://github.com/damus-io/damus/issues/751 Signed-off-by: Daniel D’Aquino --- damus/Views/Timeline/InnerTimelineView.swift | 51 ++++++++++++- damus/Views/TimelineView.swift | 78 +++++++++++++------- 2 files changed, 99 insertions(+), 30 deletions(-) diff --git a/damus/Views/Timeline/InnerTimelineView.swift b/damus/Views/Timeline/InnerTimelineView.swift index 0e572f8fd..3ecf2a269 100644 --- a/damus/Views/Timeline/InnerTimelineView.swift +++ b/damus/Views/Timeline/InnerTimelineView.swift @@ -27,7 +27,7 @@ struct InnerTimelineView: View { return [.wide] } - var body: some View { + var main_content: some View { LazyVStack(spacing: 0) { let events = self.events.events if events.isEmpty { @@ -37,6 +37,12 @@ struct InnerTimelineView: View { let indexed = Array(zip(evs, 0...)) ForEach(indexed, id: \.0.id) { tup in let ev = tup.0 + // Since NoteId is a struct (therefore a value type, not a reference type), + // assigning the id to a variable in Swift will cause the memory contents to be copied over, + // therefore ensuring we will *own* this piece of memory, reducing the risk of being rugged by Ndb today and in future as the codebase evolves. + // This is a 32-byte copy operation without any parsing, so it should in theory not regress performance significantly. + // Thanks for coming to my TED talk about this one line of code. + let ev_id = ev.id let ind = tup.1 EventView(damus: state, event: ev, options: event_options) .onTapGesture { @@ -45,6 +51,7 @@ struct InnerTimelineView: View { state.nav.push(route: Route.Thread(thread: thread)) } .padding(.top, 7) + .id(BlockID.note(ev_id)) .onAppear { let to_preload = Array([indexed[safe: ind+1]?.0, @@ -62,8 +69,48 @@ struct InnerTimelineView: View { } } } - //.padding(.horizontal) + } + + var body: some View { + if #available(iOS 17.0, *) { + self.main_content + .scrollTargetLayout() // This helps us keep track of the scroll position by telling SwiftUI which VStack we should use for scroll position ids + } else { + // Fallback on earlier versions + self.main_content + } + } + + enum BlockID: RawRepresentable, Hashable, Codable { + case top + case note(NoteId) + + // MARK: - Custom RawRepresentable implementation + // Note: String RawRepresentable implementation is needed to be used with SceneStorage + // Note: Declaring enum as a `String` for synthesized protocol conformance does not work as this is an enum with associated types + + typealias RawValue = String + + var rawValue: String { + switch self { + case .top: + return "top" + case .note(let note_id): + return "note:\(note_id.hex())" + } + } + init?(rawValue: String) { + let components = rawValue.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false) + if components.count == 2 && components[0] == "note" { + let second_component = String(components[1]) + guard let note_id = NoteId.init(hex: second_component) else { return nil } + self = .note(note_id) + } else if components[0] == "top" { + self = .top + } + return nil + } } } diff --git a/damus/Views/TimelineView.swift b/damus/Views/TimelineView.swift index b1466c513..c8ed5afc8 100644 --- a/damus/Views/TimelineView.swift +++ b/damus/Views/TimelineView.swift @@ -16,6 +16,8 @@ struct TimelineView: View { let filter: (NostrEvent) -> Bool let content: Content? let apply_mute_rules: Bool + // Note: SceneStorage persists through a session. If user completely quits the app, scroll position is not persisted. + @SceneStorage("scroll_position") var scroll_position: InnerTimelineView.BlockID = .top init(events: EventHolder, loading: Binding, damus: DamusState, show_friend_icon: Bool, filter: @escaping (NostrEvent) -> Bool, apply_mute_rules: Bool = true, content: (() -> Content)? = nil) { self.events = events @@ -28,39 +30,59 @@ struct TimelineView: View { } var body: some View { - MainContent + ScrollViewReader { scroller in + self.MainContent(scroller: scroller) + } + .onAppear { + events.flush() + } } - var MainContent: some View { - ScrollViewReader { scroller in - ScrollView { - if let content { - content - } - - Color.white.opacity(0) - .id("startblock") - .frame(height: 1) - - InnerTimelineView(events: events, damus: damus, filter: loading ? { _ in true } : filter, apply_mute_rules: self.apply_mute_rules) - .redacted(reason: loading ? .placeholder : []) - .shimmer(loading) - .disabled(loading) - .background(GeometryReader { proxy -> Color in - handle_scroll_queue(proxy, queue: self.events) - return Color.clear - }) - } - //.buttonStyle(BorderlessButtonStyle()) - .coordinateSpace(name: "scroll") - .onReceive(handle_notify(.scroll_to_top)) { () in - events.flush() - self.events.should_queue = false - scroll_to_event(scroller: scroller, id: "startblock", delay: 0.0, animate: true, anchor: .top) + func MainContent(scroller: ScrollViewProxy) -> some View { + if #available(iOS 17.0, *) { + return self.MainScrollView(scroller: scroller) + .scrollPosition(id: + // A custom Binding is needed to reconciliate incompatible types between this call and @SceneStorage + Binding( + get: { + return self.scroll_position as InnerTimelineView.BlockID? + }, + set: { newValue in + let newValueToSet = newValue ?? .top + self.scroll_position = newValueToSet + } + ), anchor: .top) + } else { + return self.MainScrollView(scroller: scroller) + } + } + + func MainScrollView(scroller: ScrollViewProxy) -> some View { + ScrollView { + if let content { + content } + + Color.white.opacity(0) + .id(InnerTimelineView.BlockID.top) + .frame(height: 1) + + InnerTimelineView(events: events, damus: damus, filter: loading ? { _ in true } : filter, apply_mute_rules: self.apply_mute_rules) + .redacted(reason: loading ? .placeholder : []) + .shimmer(loading) + .disabled(loading) + .background(GeometryReader { proxy -> Color in + handle_scroll_queue(proxy, queue: self.events) + return Color.clear + }) } - .onAppear { + .coordinateSpace(name: "scroll") + .onReceive(handle_notify(.scroll_to_top)) { () in events.flush() + self.events.should_queue = false + withAnimation { + self.scroll_position = .top + } } } }