From 237b401f41ad1d02424282ddcc1a1c76a47359c0 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 18 May 2025 00:16:59 +0100 Subject: [PATCH 1/7] fix bubble anim overshooting in the timeline when the SR hides --- .../View/Style/TimelineItemBubbledStylerView.swift | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift index 344c9bd55e..472fd1807f 100644 --- a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift @@ -45,7 +45,10 @@ struct TimelineItemBubbledStylerView: View { .zIndex(1) } - VStack(alignment: alignment, spacing: 0) { + VStack(alignment: alignment, spacing: -4) { + // -4 spacing to compensate for the Spacer we have to add to stop + // animation oversteer - see below. + HStack(spacing: 0) { if timelineItem.isOutgoing { Spacer() @@ -64,6 +67,11 @@ struct TimelineItemBubbledStylerView: View { .padding(.top, 8) .padding(.bottom, 3) } + + // we need a Spacer here to top-align the bubble within the VStack, so that + // when the the animation doesn't drift around and overshoot when the SR is hidden + // see https://github.com/element-hq/element-x-ios/issues/4127 + Spacer() } .padding(.horizontal, bubbleHorizontalPadding) .padding(.leading, bubbleAvatarPadding) From 3c25d6a0eb603bab73616bc57109950a6afc0490 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 18 May 2025 00:17:24 +0100 Subject: [PATCH 2/7] typo --- .../Sources/Screens/Timeline/TimelineTableViewController.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ElementX/Sources/Screens/Timeline/TimelineTableViewController.swift b/ElementX/Sources/Screens/Timeline/TimelineTableViewController.swift index 17910355cf..4eb45d38a0 100644 --- a/ElementX/Sources/Screens/Timeline/TimelineTableViewController.swift +++ b/ElementX/Sources/Screens/Timeline/TimelineTableViewController.swift @@ -292,7 +292,7 @@ class TimelineTableViewController: UIViewController { } } - // We only animate when there's a new last message, so its safe + // We only animate when there's a new last message, so it's safe // to animate from the bottom (which is the top as we're flipped). dataSource?.defaultRowAnimation = (UIAccessibility.isReduceMotionEnabled ? .none : .top) tableView.delegate = self From 3b2f42721e359b67e3e8cd9c44a0ef20e2a0c1a9 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 19 May 2025 01:09:36 +0200 Subject: [PATCH 3/7] fix drifting animations when loading scrollback by hoisting messageBubbleTopPadding to the body of TimelineItemBubbledStylerView and disabling animation when messageBubbleTopPadding changes. This fixes the issue described in https://github.com/element-hq/element-x-ios/issues/4127#issuecomment-2889224682 It also should fix one of the main causes of scroll jumps from https://github.com/element-hq/element-x-ios/issues/2877 For now, this includes some commented out background colours which are very helpful for visualising these failure modes --- .../Style/TimelineItemBubbledStylerView.swift | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift index 472fd1807f..8f1bb09b76 100644 --- a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift @@ -48,6 +48,8 @@ struct TimelineItemBubbledStylerView: View { VStack(alignment: alignment, spacing: -4) { // -4 spacing to compensate for the Spacer we have to add to stop // animation oversteer - see below. + // XXX: does this squidge the bubble & the SR too close together now? + // it looks like it should, but in practice it doesn't seem to. HStack(spacing: 0) { if timelineItem.isOutgoing { @@ -62,22 +64,36 @@ struct TimelineItemBubbledStylerView: View { if !timelineItem.isOutgoing { Spacer() } - TimelineItemStatusView(timelineItem: timelineItem, adjustedDeliveryStatus: adjustedDeliveryStatus) + TimelineItemStatusView(timelineItem: timelineItem, adjustedDeliveryStatus: adjustedDeliveryStatus, context: context) .environmentObject(context) .padding(.top, 8) .padding(.bottom, 3) } + // .background(Color.purple) // we need a Spacer here to top-align the bubble within the VStack, so that // when the the animation doesn't drift around and overshoot when the SR is hidden // see https://github.com/element-hq/element-x-ios/issues/4127 Spacer() + // .background { Color.yellow.frame(minWidth: 10) } } .padding(.horizontal, bubbleHorizontalPadding) .padding(.leading, bubbleAvatarPadding) } } + // .background { + // Int(timelineItem.id.uniqueID.value).unsafelyUnwrapped % 4 == 0 ? Color.green : + // Int(timelineItem.id.uniqueID.value).unsafelyUnwrapped % 4 == 1 ? Color.cyan : + // Int(timelineItem.id.uniqueID.value).unsafelyUnwrapped % 4 == 2 ? Color.brown : + // Color.indigo + // } .padding(EdgeInsets(top: 1, leading: 8, bottom: 1, trailing: 8)) + // hoist the unanimated messageBubbleTopPadding to the topmost view, as otherwise the + // UITableView row pops when this View changes layout seemingly. + // N.B. we can't combine two paddings together without triggering popping. + .padding(.top, messageBubbleTopPadding) + // don't reanimate bubble layouts if we're just changing the messageBubbleTopPadding + .animation(nil, value: messageBubbleTopPadding) .highlightedTimelineItem(isFocussed) } @@ -171,7 +187,6 @@ struct TimelineItemBubbledStylerView: View { } } .pinnedIndicator(isPinned: isPinned, isOutgoing: timelineItem.isOutgoing) - .padding(.top, messageBubbleTopPadding) } var messageBubble: some View { From 550f59f08c64e13d81efbbbc311e056c3dd6ff80 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 19 May 2025 01:10:06 +0200 Subject: [PATCH 4/7] add commented out debug colour for now --- .../View/TimelineItemViews/SeparatorRoomTimelineView.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/ElementX/Sources/Screens/Timeline/View/TimelineItemViews/SeparatorRoomTimelineView.swift b/ElementX/Sources/Screens/Timeline/View/TimelineItemViews/SeparatorRoomTimelineView.swift index 2535a8c20c..b40e7c9f98 100644 --- a/ElementX/Sources/Screens/Timeline/View/TimelineItemViews/SeparatorRoomTimelineView.swift +++ b/ElementX/Sources/Screens/Timeline/View/TimelineItemViews/SeparatorRoomTimelineView.swift @@ -18,6 +18,7 @@ struct SeparatorRoomTimelineView: View { .multilineTextAlignment(.center) .padding(.horizontal, 36.0) .padding(.vertical, 8.0) + // .background(Color.red) } } From 9df170ee38ec7d46c7ba9c537f48afc1adfef50b Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 19 May 2025 01:15:23 +0200 Subject: [PATCH 5/7] fix intermittent glitches when sending messages. When UITableView inserts a new row at precisely the same time as us hiding the Send Receipts on a bubble (due to moving them to the new row), there's a race which can cause UITableView to miscalculate y-offsets due to the height of the bubble suddenly changing. To break the race, we hide the SR shortly after the row is inserted. Fixes the problem shown in https://github.com/element-hq/element-x-ios/issues/4127#issuecomment-2889131459 --- .../TimelineItemStatusView.swift | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift b/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift index 08c3aadf8f..49482733d3 100644 --- a/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift @@ -12,13 +12,35 @@ struct TimelineItemStatusView: View { let timelineItem: EventBasedTimelineItemProtocol let adjustedDeliveryStatus: TimelineItemDeliveryStatus? @EnvironmentObject private var context: TimelineViewModel.Context + @State private var isSendReceiptVisible: Bool private var isLastOutgoingMessage: Bool { timelineItem.isOutgoing && context.viewState.timelineState.uniqueIDs.last == timelineItem.id.uniqueID } + init(timelineItem: EventBasedTimelineItemProtocol, adjustedDeliveryStatus: TimelineItemDeliveryStatus?, context: TimelineViewModel.Context, isSendReceiptVisible: Bool = false) { + self.timelineItem = timelineItem + self.adjustedDeliveryStatus = adjustedDeliveryStatus + // Ugly - we can't call isLastOutgoingMessage here as the real `context` hasn't loaded yet + // so instead we manually pass in context to init() and duplicate isLastOutgoingMessage here. + self.isSendReceiptVisible = timelineItem.isOutgoing && context.viewState.timelineState.uniqueIDs.last == timelineItem.id.uniqueID + } + var body: some View { mainContent + .onChange(of: context.viewState.timelineState.uniqueIDs.last) { _, _ in + if isLastOutgoingMessage { + isSendReceiptVisible = true + } else if isSendReceiptVisible { + // we were the last msg in the timeline, but not any more + // so remove the SR after a short delay to avoid racing with the new msg animation + DispatchQueue.main.asyncAfter(deadline: .now() + 0.01) { + withAnimation { + isSendReceiptVisible = false + } + } + } + } } @ViewBuilder @@ -39,7 +61,7 @@ struct TimelineItemStatusView: View { case .sending: TimelineDeliveryStatusView(deliveryStatus: .sending) case .sent, .none: - if isLastOutgoingMessage { + if isSendReceiptVisible { // We only display the sent icon for the latest outgoing message TimelineDeliveryStatusView(deliveryStatus: .sent) } From ad17c136644542c1713082b3237764cb027ca29f Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 19 May 2025 01:38:20 +0200 Subject: [PATCH 6/7] restore animations during loading scrollback, as it's breaking stuff --- .../Timeline/View/Style/TimelineItemBubbledStylerView.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift index 8f1bb09b76..7b314073df 100644 --- a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift @@ -93,7 +93,7 @@ struct TimelineItemBubbledStylerView: View { // N.B. we can't combine two paddings together without triggering popping. .padding(.top, messageBubbleTopPadding) // don't reanimate bubble layouts if we're just changing the messageBubbleTopPadding - .animation(nil, value: messageBubbleTopPadding) + // .animation(nil, value: messageBubbleTopPadding) .highlightedTimelineItem(isFocussed) } From ecc995338d2cb78841b9c4f4f21dda195674c08e Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 19 May 2025 18:00:44 +0200 Subject: [PATCH 7/7] increase the delay in hiding SR to not clash with UITableView's anim and reintroduce skipping animations on bubble layouts if we're just changing the messageBubbleTopPadding, as per https://github.com/element-hq/element-x-ios/issues/4127#issuecomment-2891537849 --- .../Timeline/View/Style/TimelineItemBubbledStylerView.swift | 2 +- .../Timeline/View/Supplementary/TimelineItemStatusView.swift | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift index 7b314073df..8f1bb09b76 100644 --- a/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Style/TimelineItemBubbledStylerView.swift @@ -93,7 +93,7 @@ struct TimelineItemBubbledStylerView: View { // N.B. we can't combine two paddings together without triggering popping. .padding(.top, messageBubbleTopPadding) // don't reanimate bubble layouts if we're just changing the messageBubbleTopPadding - // .animation(nil, value: messageBubbleTopPadding) + .animation(nil, value: messageBubbleTopPadding) .highlightedTimelineItem(isFocussed) } diff --git a/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift b/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift index 49482733d3..0f5c6330ed 100644 --- a/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift +++ b/ElementX/Sources/Screens/Timeline/View/Supplementary/TimelineItemStatusView.swift @@ -34,7 +34,7 @@ struct TimelineItemStatusView: View { } else if isSendReceiptVisible { // we were the last msg in the timeline, but not any more // so remove the SR after a short delay to avoid racing with the new msg animation - DispatchQueue.main.asyncAfter(deadline: .now() + 0.01) { + DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { withAnimation { isSendReceiptVisible = false } @@ -64,6 +64,7 @@ struct TimelineItemStatusView: View { if isSendReceiptVisible { // We only display the sent icon for the latest outgoing message TimelineDeliveryStatusView(deliveryStatus: .sent) + // .transition(.identity) // makes the SR disappear rapidly to avoid ugly z-index flickering } case .sendingFailed: // Bubbles handle the case internally