From ff276a48212b770af8da9c054068149f825c20a2 Mon Sep 17 00:00:00 2001 From: David Langley Date: Wed, 12 Jul 2023 21:51:18 +0100 Subject: [PATCH 1/2] Sort timetamps on timeline and show the date on the summary view - Sort reaction aggregations on the timeline by count and then by most recent timestamp ascending(tagging new reactions on the end) - Show the timestamp on the summary view and sort descending(newest at the top) --- .../Mocks/AggregatedReactionMock.swift | 50 +++++++++++-------- .../Supplementary/ReactionsSummaryView.swift | 14 ++++-- .../Fixtures/RoomTimelineItemFixtures.swift | 12 +++-- .../AggregratedReaction.swift | 12 ++++- .../RoomTimelineItemFactory.swift | 19 +++++-- .../Sources/RoomScreenViewModelTests.swift | 2 +- 6 files changed, 72 insertions(+), 37 deletions(-) diff --git a/ElementX/Sources/Mocks/AggregatedReactionMock.swift b/ElementX/Sources/Mocks/AggregatedReactionMock.swift index bf8a56880e..e4371c5678 100644 --- a/ElementX/Sources/Mocks/AggregatedReactionMock.swift +++ b/ElementX/Sources/Mocks/AggregatedReactionMock.swift @@ -23,42 +23,50 @@ extension AggregatedReaction { } } + private static func mockReaction(key: String, senderIDs: [String]) -> AggregatedReaction { + let senders = senderIDs + .map { id in + ReactionSender(senderId: id, timestamp: Date()) + } + return AggregatedReaction(accountOwnerID: alice, key: key, senders: senders) + } + private static var alice: String { RoomMemberProxyMock.mockAlice.userID } static var mockThumbsUpHighlighted: AggregatedReaction { - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿ‘", senders: [alice] + mockIds(4)) + mockReaction(key: "๐Ÿ‘", senderIDs: [alice] + mockIds(4)) } static var mockClap: AggregatedReaction { - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿ‘", senders: mockIds(1)) + mockReaction(key: "๐Ÿ‘", senderIDs: mockIds(1)) } static var mockParty: AggregatedReaction { - AggregatedReaction(accountOwnerID: alice, key: "๐ŸŽ‰", senders: mockIds(20)) + mockReaction(key: "๐ŸŽ‰", senderIDs: mockIds(20)) } static var mockReactions: [AggregatedReaction] { [ - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿ˜…", senders: [alice]), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿคทโ€โ™‚๏ธ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐ŸŽจ", senders: [alice] + mockIds(5)), - AggregatedReaction(accountOwnerID: alice, key: "๐ŸŽ‰", senders: mockIds(8)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿคฏ", senders: [alice] + mockIds(14)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿซฃ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿš€", senders: [alice] + mockIds(3)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿ˜‡", senders: mockIds(2)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿคญ", senders: [alice] + mockIds(8)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿซค", senders: mockIds(10)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿถ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿฑ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿญ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿน", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿฐ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐ŸฆŠ", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿป", senders: mockIds(1)), - AggregatedReaction(accountOwnerID: alice, key: "๐Ÿผ", senders: mockIds(1)) + mockReaction(key: "๐Ÿ˜…", senderIDs: [alice]), + mockReaction(key: "๐Ÿคทโ€โ™‚๏ธ", senderIDs: mockIds(1)), + mockReaction(key: "๐ŸŽจ", senderIDs: [alice] + mockIds(5)), + mockReaction(key: "๐ŸŽ‰", senderIDs: mockIds(8)), + mockReaction(key: "๐Ÿคฏ", senderIDs: [alice] + mockIds(14)), + mockReaction(key: "๐Ÿซฃ", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿš€", senderIDs: [alice] + mockIds(3)), + mockReaction(key: "๐Ÿ˜‡", senderIDs: mockIds(2)), + mockReaction(key: "๐Ÿคญ", senderIDs: [alice] + mockIds(8)), + mockReaction(key: "๐Ÿซค", senderIDs: mockIds(10)), + mockReaction(key: "๐Ÿถ", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿฑ", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿญ", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿน", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿฐ", senderIDs: mockIds(1)), + mockReaction(key: "๐ŸฆŠ", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿป", senderIDs: mockIds(1)), + mockReaction(key: "๐Ÿผ", senderIDs: mockIds(1)) ] } } diff --git a/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift b/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift index 8192ee68da..8c174f65d9 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift @@ -67,7 +67,7 @@ struct ReactionsSummaryView: View { ScrollView { VStack(alignment: .leading) { ForEach(reaction.senders, id: \.self) { sender in - ReactionSummarySenderView(sender: sender, member: members[sender], imageProvider: imageProvider) + ReactionSummarySenderView(sender: sender, member: members[sender.senderId], imageProvider: imageProvider) .padding(.horizontal, 16) } } @@ -107,28 +107,32 @@ private struct ReactionSummaryButton: View { } private struct ReactionSummarySenderView: View { - var sender: String + var sender: ReactionSender var member: RoomMemberState? let imageProvider: ImageProviderProtocol? var displayName: String { - member?.displayName ?? sender + member?.displayName ?? sender.senderId } var body: some View { HStack { LoadableAvatarImage(url: member?.avatarURL, name: displayName, - contentID: sender, + contentID: sender.senderId, avatarSize: .user(on: .timeline), imageProvider: imageProvider) VStack(alignment: .leading) { Text(displayName) .font(.compound.bodyMDSemibold) - Text(sender) + Text(sender.senderId) .font(.compound.bodySM) .foregroundColor(.compound.textSecondary) } + .frame(maxWidth: .infinity, alignment: .leading) + Text(sender.timestamp.formattedMinimal()) + .font(.compound.bodyXS) + .foregroundColor(.compound.textSecondary) } .frame(maxWidth: .infinity, alignment: .leading) .padding(.vertical, 8) diff --git a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift index 981b233158..6cd742a241 100644 --- a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift +++ b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift @@ -34,7 +34,7 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Helena"), content: .init(body: "Letโ€™s get lunch soon! New salad place opened up ๐Ÿฅ—. When are yโ€™all free? ๐Ÿค—"), properties: RoomTimelineItemProperties(reactions: [ - AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: ["me"]) + AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: [ReactionSender(senderId: "me", timestamp: Date())]) ])), TextRoomTimelineItem(id: .init(timelineID: UUID().uuidString), timestamp: "10:11 AM", @@ -43,8 +43,14 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Helena"), content: .init(body: "I can be around on Wednesday. How about some ๐ŸŒฎ instead? Like https://www.tortilla.co.uk/"), properties: RoomTimelineItemProperties(reactions: [ - AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™", senders: ["helena"]), - AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: ["me", "helena", "jacob"]) + AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™", senders: [ReactionSender(senderId: "helena", timestamp: Date())]), + AggregatedReaction(accountOwnerID: "me", + key: "๐Ÿ™Œ", + senders: [ + ReactionSender(senderId: "me", timestamp: Date()), + ReactionSender(senderId: "helena", timestamp: Date()), + ReactionSender(senderId: "jacob", timestamp: Date()) + ]) ])), SeparatorRoomTimelineItem(id: .init(timelineID: "Today"), text: "Today"), TextRoomTimelineItem(id: .init(timelineID: UUID().uuidString), diff --git a/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift b/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift index 108ac9aa32..3069a7f768 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift @@ -23,7 +23,15 @@ struct AggregatedReaction: Hashable { /// The reaction that was sent. let key: String /// The user ids of those who sent the reactions - let senders: [String] + let senders: [ReactionSender] +} + +/// Details of who sent the reaction +struct ReactionSender: Hashable { + /// The id of the user who sent the reaction + let senderId: String + /// The time that the reaction was received on the original homeserver + let timestamp: Date } extension AggregatedReaction { @@ -34,6 +42,6 @@ extension AggregatedReaction { /// Whether to highlight the reaction, indicating that the current user sent this reaction. var isHighlighted: Bool { - senders.contains(where: { $0 == accountOwnerID }) + senders.contains(where: { $0.senderId == accountOwnerID }) } } diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift index 649ef93d47..20bcb419c9 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift @@ -313,13 +313,22 @@ struct RoomTimelineItemFactory: RoomTimelineItemFactoryProtocol { private func aggregateReactions(_ reactions: [Reaction]) -> [AggregatedReaction] { reactions.map { reaction in - AggregatedReaction(accountOwnerID: userID, key: reaction.key, senders: reaction.senders.map(\.senderId)) + let senders = reaction.senders + .map { senderData in + ReactionSender(senderId: senderData.senderId, timestamp: Date(timeIntervalSince1970: TimeInterval(senderData.timestamp / 1000))) + } + .sorted { a, b in + // Sort reactions most recent first + a.timestamp > b.timestamp + } + return AggregatedReaction(accountOwnerID: userID, key: reaction.key, senders: senders) } .sorted { a, b in - // Sort by count and then by key for a consistence experience. - // Otherwise emojis can switch around. We can replace - // with timestamp as a secondary sort when it is available. - (a.count, a.key) > (b.count, b.key) + // Sort aggregated reactions by count and then by most recent reaction + if a.count == b.count { + return a.senders[0].timestamp < b.senders[0].timestamp + } + return a.count > b.count } } diff --git a/UnitTests/Sources/RoomScreenViewModelTests.swift b/UnitTests/Sources/RoomScreenViewModelTests.swift index 978d52b4b1..9d39097404 100644 --- a/UnitTests/Sources/RoomScreenViewModelTests.swift +++ b/UnitTests/Sources/RoomScreenViewModelTests.swift @@ -375,7 +375,7 @@ class RoomScreenViewModelTests: XCTestCase { private extension TextRoomTimelineItem { init(text: String, sender: String, addReactions: Bool = false) { - let reactions = addReactions ? [AggregatedReaction(accountOwnerID: "bob", key: "๐Ÿฆ„", senders: [sender])] : [] + let reactions = addReactions ? [AggregatedReaction(accountOwnerID: "bob", key: "๐Ÿฆ„", senders: [ReactionSender(senderId: sender, timestamp: Date())])] : [] self.init(id: .init(timelineID: UUID().uuidString), timestamp: "10:47 am", isOutgoing: sender == "bob", From 84c7b13ce6ce84fc79c1ed40e27aa6e947610bdc Mon Sep 17 00:00:00 2001 From: David Langley Date: Fri, 14 Jul 2023 14:44:48 +0100 Subject: [PATCH 2/2] Address comments. - Fix ID case. - Improve readability of components in body. - Improve the comments that describe the sorting. --- ElementX/Sources/Mocks/AggregatedReactionMock.swift | 2 +- .../View/Supplementary/ReactionsSummaryView.swift | 10 ++++++---- .../Timeline/Fixtures/RoomTimelineItemFixtures.swift | 10 +++++----- .../TimelineItemContent/AggregratedReaction.swift | 4 ++-- .../TimelineItems/RoomTimelineItemFactory.swift | 12 +++++++++--- UnitTests/Sources/RoomScreenViewModelTests.swift | 2 +- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/ElementX/Sources/Mocks/AggregatedReactionMock.swift b/ElementX/Sources/Mocks/AggregatedReactionMock.swift index e4371c5678..bf9581ebbd 100644 --- a/ElementX/Sources/Mocks/AggregatedReactionMock.swift +++ b/ElementX/Sources/Mocks/AggregatedReactionMock.swift @@ -26,7 +26,7 @@ extension AggregatedReaction { private static func mockReaction(key: String, senderIDs: [String]) -> AggregatedReaction { let senders = senderIDs .map { id in - ReactionSender(senderId: id, timestamp: Date()) + ReactionSender(senderID: id, timestamp: Date()) } return AggregatedReaction(accountOwnerID: alice, key: key, senders: senders) } diff --git a/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift b/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift index 8c174f65d9..92ac2c75d2 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift @@ -67,7 +67,7 @@ struct ReactionsSummaryView: View { ScrollView { VStack(alignment: .leading) { ForEach(reaction.senders, id: \.self) { sender in - ReactionSummarySenderView(sender: sender, member: members[sender.senderId], imageProvider: imageProvider) + ReactionSummarySenderView(sender: sender, member: members[sender.senderID], imageProvider: imageProvider) .padding(.horizontal, 16) } } @@ -112,24 +112,26 @@ private struct ReactionSummarySenderView: View { let imageProvider: ImageProviderProtocol? var displayName: String { - member?.displayName ?? sender.senderId + member?.displayName ?? sender.senderID } var body: some View { HStack { LoadableAvatarImage(url: member?.avatarURL, name: displayName, - contentID: sender.senderId, + contentID: sender.senderID, avatarSize: .user(on: .timeline), imageProvider: imageProvider) + VStack(alignment: .leading) { Text(displayName) .font(.compound.bodyMDSemibold) - Text(sender.senderId) + Text(sender.senderID) .font(.compound.bodySM) .foregroundColor(.compound.textSecondary) } .frame(maxWidth: .infinity, alignment: .leading) + Text(sender.timestamp.formattedMinimal()) .font(.compound.bodyXS) .foregroundColor(.compound.textSecondary) diff --git a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift index 6cd742a241..200643374e 100644 --- a/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift +++ b/ElementX/Sources/Services/Timeline/Fixtures/RoomTimelineItemFixtures.swift @@ -34,7 +34,7 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Helena"), content: .init(body: "Letโ€™s get lunch soon! New salad place opened up ๐Ÿฅ—. When are yโ€™all free? ๐Ÿค—"), properties: RoomTimelineItemProperties(reactions: [ - AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: [ReactionSender(senderId: "me", timestamp: Date())]) + AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: [ReactionSender(senderID: "me", timestamp: Date())]) ])), TextRoomTimelineItem(id: .init(timelineID: UUID().uuidString), timestamp: "10:11 AM", @@ -43,13 +43,13 @@ enum RoomTimelineItemFixtures { sender: .init(id: "", displayName: "Helena"), content: .init(body: "I can be around on Wednesday. How about some ๐ŸŒฎ instead? Like https://www.tortilla.co.uk/"), properties: RoomTimelineItemProperties(reactions: [ - AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™", senders: [ReactionSender(senderId: "helena", timestamp: Date())]), + AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™", senders: [ReactionSender(senderID: "helena", timestamp: Date())]), AggregatedReaction(accountOwnerID: "me", key: "๐Ÿ™Œ", senders: [ - ReactionSender(senderId: "me", timestamp: Date()), - ReactionSender(senderId: "helena", timestamp: Date()), - ReactionSender(senderId: "jacob", timestamp: Date()) + ReactionSender(senderID: "me", timestamp: Date()), + ReactionSender(senderID: "helena", timestamp: Date()), + ReactionSender(senderID: "jacob", timestamp: Date()) ]) ])), SeparatorRoomTimelineItem(id: .init(timelineID: "Today"), text: "Today"), diff --git a/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift b/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift index 3069a7f768..2b9d0ec2ab 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItemContent/AggregratedReaction.swift @@ -29,7 +29,7 @@ struct AggregatedReaction: Hashable { /// Details of who sent the reaction struct ReactionSender: Hashable { /// The id of the user who sent the reaction - let senderId: String + let senderID: String /// The time that the reaction was received on the original homeserver let timestamp: Date } @@ -42,6 +42,6 @@ extension AggregatedReaction { /// Whether to highlight the reaction, indicating that the current user sent this reaction. var isHighlighted: Bool { - senders.contains(where: { $0.senderId == accountOwnerID }) + senders.contains(where: { $0.senderID == accountOwnerID }) } } diff --git a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift index 20bcb419c9..be3c0e3331 100644 --- a/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift +++ b/ElementX/Sources/Services/Timeline/TimelineItems/RoomTimelineItemFactory.swift @@ -315,16 +315,22 @@ struct RoomTimelineItemFactory: RoomTimelineItemFactoryProtocol { reactions.map { reaction in let senders = reaction.senders .map { senderData in - ReactionSender(senderId: senderData.senderId, timestamp: Date(timeIntervalSince1970: TimeInterval(senderData.timestamp / 1000))) + ReactionSender(senderID: senderData.senderId, timestamp: Date(timeIntervalSince1970: TimeInterval(senderData.timestamp / 1000))) } .sorted { a, b in - // Sort reactions most recent first + // Sort reactions within an aggregation by timestamp descending. + // This puts the most recent at the top, useful in cases like the + // reaction summary view. a.timestamp > b.timestamp } return AggregatedReaction(accountOwnerID: userID, key: reaction.key, senders: senders) } .sorted { a, b in - // Sort aggregated reactions by count and then by most recent reaction + // Sort aggregated reactions by count and then timestamp ascending, using + // the most recent reaction in the aggregation(hence index 0). + // This appends new aggregations on the end of the reaction layout + // and the deterministic sort avoids reactions jumping around if the reactions timeline + // view reloads. if a.count == b.count { return a.senders[0].timestamp < b.senders[0].timestamp } diff --git a/UnitTests/Sources/RoomScreenViewModelTests.swift b/UnitTests/Sources/RoomScreenViewModelTests.swift index 9d39097404..2b443a5089 100644 --- a/UnitTests/Sources/RoomScreenViewModelTests.swift +++ b/UnitTests/Sources/RoomScreenViewModelTests.swift @@ -375,7 +375,7 @@ class RoomScreenViewModelTests: XCTestCase { private extension TextRoomTimelineItem { init(text: String, sender: String, addReactions: Bool = false) { - let reactions = addReactions ? [AggregatedReaction(accountOwnerID: "bob", key: "๐Ÿฆ„", senders: [ReactionSender(senderId: sender, timestamp: Date())])] : [] + let reactions = addReactions ? [AggregatedReaction(accountOwnerID: "bob", key: "๐Ÿฆ„", senders: [ReactionSender(senderID: sender, timestamp: Date())])] : [] self.init(id: .init(timelineID: UUID().uuidString), timestamp: "10:47 am", isOutgoing: sender == "bob",