From 7cadcf1e862b3580702f096c1694ce56e5bf1417 Mon Sep 17 00:00:00 2001 From: Shadowfacts Date: Sat, 4 Feb 2023 14:54:39 -0500 Subject: [PATCH] Reuse conversation tree where possible when selecting a status in a conversation --- ...ConversationCollectionViewController.swift | 111 ++++++++++----- .../Conversation/ConversationTree.swift | 57 +++++--- .../ConversationViewController.swift | 128 ++++++++++++------ 3 files changed, 200 insertions(+), 96 deletions(-) diff --git a/Tusker/Screens/Conversation/ConversationCollectionViewController.swift b/Tusker/Screens/Conversation/ConversationCollectionViewController.swift index 42f24724..9ad00420 100644 --- a/Tusker/Screens/Conversation/ConversationCollectionViewController.swift +++ b/Tusker/Screens/Conversation/ConversationCollectionViewController.swift @@ -45,11 +45,15 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont (collectionView.cellForItem(at: $0) as? TimelineStatusCollectionViewCell)?.trailingSwipeActions() } config.itemSeparatorHandler = { [unowned self] indexPath, sectionConfig in - let rowsInSection = self.collectionView.numberOfItems(inSection: indexPath.section) - let lastInSection = indexPath.row == rowsInSection - 1 var config = sectionConfig config.topSeparatorVisibility = .hidden - config.bottomSeparatorVisibility = lastInSection ? .visible : .hidden + if case .ancestors = self.dataSource.sectionIdentifier(for: indexPath.section) { + config.bottomSeparatorVisibility = .hidden + } else if indexPath.row == self.collectionView.numberOfItems(inSection: indexPath.section) - 1 { + config.bottomSeparatorVisibility = .visible + } else { + config.bottomSeparatorVisibility = .hidden + } config.bottomSeparatorInsets = TimelineStatusCollectionViewCell.separatorInsets return config } @@ -89,7 +93,7 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont } return UICollectionViewDiffableDataSource(collectionView: collectionView) { [unowned self] collectionView, indexPath, itemIdentifier in switch itemIdentifier { - case let .status(id: id, state: state, prevLink: prevLink, nextLink: nextLink): + case let .status(id: id, node: _, state: state, prevLink: prevLink, nextLink: nextLink): if id == self.mainStatusID { return collectionView.dequeueConfiguredReusableCell(using: mainStatusCell, for: indexPath, item: (id, state, prevLink)) } else { @@ -113,36 +117,28 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont loadViewIfNeeded() var snapshot = NSDiffableDataSourceSnapshot() - snapshot.appendSections([.statuses]) + snapshot.appendSections([.ancestors, .mainStatus]) if status.inReplyToID != nil { - snapshot.appendItems([.loadingIndicator], toSection: .statuses) + snapshot.appendItems([.loadingIndicator], toSection: .ancestors) } - let mainStatusItem = Item.status(id: mainStatusID, state: mainStatusState, prevLink: status.inReplyToID != nil, nextLink: false) - snapshot.appendItems([mainStatusItem], toSection: .statuses) + // this will be replace with the actual node in the tree once it's loaded + let tempMainNode = ConversationNode(status: status) + let mainStatusItem = Item.status(id: mainStatusID, node: tempMainNode, state: mainStatusState, prevLink: status.inReplyToID != nil, nextLink: false) + snapshot.appendItems([mainStatusItem], toSection: .mainStatus) dataSource.apply(snapshot, animatingDifferences: false) } - func addContext(_ context: ConversationContext, for mainStatus: StatusMO) async { - await mastodonController.persistentContainer.addAll(statuses: context.ancestors + context.descendants) - - // fetch all descendant status managed objects - let descendantIDs = context.descendants.map(\.id) - let request = StatusMO.fetchRequest() - request.predicate = NSPredicate(format: "id IN %@", descendantIDs) - let descendants = try? mastodonController.persistentContainer.viewContext.fetch(request) - - let tree = ConversationTree.build(from: context, mainStatus: mainStatus, descendants: descendants ?? []) - + func addTree(_ tree: ConversationTree, mainStatus: StatusMO) { var snapshot = dataSource.snapshot() snapshot.deleteItems([.loadingIndicator]) - let mainStatusItem = Item.status(id: mainStatusID, state: mainStatusState, prevLink: mainStatus.inReplyToID != nil, nextLink: false) - let parentItems = tree.ancestors.enumerated().map { index, id in - Item.status(id: id, state: .unknown, prevLink: index > 0, nextLink: true) + let mainStatusItem = Item.status(id: mainStatusID, node: tree.mainStatus, state: mainStatusState, prevLink: mainStatus.inReplyToID != nil, nextLink: false) + let parentItems = tree.ancestors.enumerated().map { index, node in + Item.status(id: node.status.id, node: node, state: .unknown, prevLink: index > 0, nextLink: true) } - snapshot.insertItems(parentItems, beforeItem: mainStatusItem) + snapshot.appendItems(parentItems, toSection: .ancestors) snapshot.reloadItems([mainStatusItem]) // convert sub-threads into items for section and add to snapshot @@ -156,7 +152,7 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont position = .centeredVertically } else { item = snapshot.itemIdentifiers.first { - if case .status(id: self.statusIDToScrollToOnLoad, _, _, _) = $0 { + if case .status(id: self.statusIDToScrollToOnLoad, _, _, _, _) = $0 { return true } else { return false @@ -185,7 +181,7 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont for node in childThreads { let section = Section.childThread(firstStatusID: node.status.id) snapshot.appendSections([section]) - snapshot.appendItems([.status(id: node.status.id, state: .unknown, prevLink: false, nextLink: !node.children.isEmpty)], toSection: section) + snapshot.appendItems([.status(id: node.status.id, node: node, state: .unknown, prevLink: false, nextLink: !node.children.isEmpty)], toSection: section) var currentNode = node while true { @@ -208,7 +204,7 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont } currentNode = next - snapshot.appendItems([.status(id: next.status.id, state: .unknown, prevLink: true, nextLink: !next.children.isEmpty)], toSection: section) + snapshot.appendItems([.status(id: next.status.id, node: next, state: .unknown, prevLink: true, nextLink: !next.children.isEmpty)], toSection: section) } } } @@ -217,7 +213,7 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont var snapshot = dataSource.snapshot() var cellsToMask: [StatusCollectionViewCell] = [] for item in snapshot.itemIdentifiers { - guard case .status(id: _, state: let state, prevLink: _, nextLink: _) = item, + guard case .status(id: _, node: _, state: let state, prevLink: _, nextLink: _) = item, state.collapsible == true else { continue } @@ -248,17 +244,18 @@ class ConversationCollectionViewController: UIViewController, CollectionViewCont extension ConversationCollectionViewController { enum Section: Hashable { - case statuses + case ancestors + case mainStatus case childThread(firstStatusID: String) } enum Item: Hashable { - case status(id: String, state: CollapseState, prevLink: Bool, nextLink: Bool) + case status(id: String, node: ConversationNode, state: CollapseState, prevLink: Bool, nextLink: Bool) case expandThread(childThreads: [ConversationNode], inline: Bool) case loadingIndicator static func ==(lhs: Item, rhs: Item) -> Bool { switch (lhs, rhs) { - case let (.status(id: a, state: _, prevLink: aPrev, nextLink: aNext), .status(id: b, state: _, prevLink: bPrev, nextLink: bNext)): + case let (.status(id: a, node: _, state: _, prevLink: aPrev, nextLink: aNext), .status(id: b, node: _, state: _, prevLink: bPrev, nextLink: bNext)): return a == b && aPrev == bPrev && aNext == bNext case let (.expandThread(childThreads: a, inline: aInline), .expandThread(childThreads: b, inline: bInline)): return a.count == b.count && zip(a, b).allSatisfy { $0.status.id == $1.status.id } && aInline == bInline @@ -271,7 +268,7 @@ extension ConversationCollectionViewController { func hash(into hasher: inout Hasher) { switch self { - case let .status(id: id, state: _, prevLink: prevLink, nextLink: nextLink): + case let .status(id: id, node: _, state: _, prevLink: prevLink, nextLink: nextLink): hasher.combine(0) hasher.combine(id) hasher.combine(prevLink) @@ -292,7 +289,7 @@ extension ConversationCollectionViewController { extension ConversationCollectionViewController: UICollectionViewDelegate { func collectionView(_ collectionView: UICollectionView, shouldSelectItemAt indexPath: IndexPath) -> Bool { switch dataSource.itemIdentifier(for: indexPath) { - case .status(id: let id, state: _, prevLink: _, nextLink: _): + case .status(id: let id, node: _, state: _, prevLink: _, nextLink: _): return id != mainStatusID case .expandThread(childThreads: _, inline: _): return true @@ -307,12 +304,24 @@ extension ConversationCollectionViewController: UICollectionViewDelegate { break case .loadingIndicator: break - case .status(id: let id, state: let state, _, _): - selected(status: id, state: state.copy()) + case .status(id: let id, node: let node, state: let state, _, _): + // we can only take the fast path if the user tapped on a descendant status. + // if the current main status is C, or one of its descendants, and the user taps A, then B won't be loaded: + // A + // / \ + // B C + if case .childThread(_) = dataSource.sectionIdentifier(for: indexPath.section) { + let tree = ConversationTree(ancestors: buildNewAncestors(above: indexPath), mainStatus: node) + let conv = ConversationViewController(preloadedTree: tree, state: state.copy(), mastodonController: mastodonController) + conv.showStatusesAutomatically = showStatusesAutomatically + show(conv) + } else { + selected(status: id, state: state.copy()) + } case .expandThread(childThreads: let childThreads, inline: _): - if case .status(id: let id, state: let state, _, _) = dataSource.itemIdentifier(for: IndexPath(row: indexPath.row - 1, section: indexPath.section)) { - // todo: it would be nice to avoid re-fetching the context here, since we should have all the necessary information already - let conv = ConversationViewController(for: id, state: state.copy(), mastodonController: mastodonController) + if case .status(id: _, node: let node, state: let state, _, _) = dataSource.itemIdentifier(for: IndexPath(row: indexPath.row - 1, section: indexPath.section)) { + let tree = ConversationTree(ancestors: buildNewAncestors(above: indexPath), mainStatus: node) + let conv = ConversationViewController(preloadedTree: tree, state: state.copy(), mastodonController: mastodonController) conv.statusIDToScrollToOnLoad = childThreads.first!.status.id conv.showStatusesAutomatically = showStatusesAutomatically show(conv) @@ -320,6 +329,34 @@ extension ConversationCollectionViewController: UICollectionViewDelegate { } } + // ConversationNode doesn't know about its parent, so we reconstruct that info from the data source + private func buildNewAncestors(above indexPath: IndexPath) -> [ConversationNode] { + let snapshot = dataSource.snapshot() + let currentAncestors = snapshot.itemIdentifiers(inSection: .ancestors).compactMap { + if case .status(id: _, node: let node, _, _, _) = $0 { + return node + } else { + return nil + } + } + let currentMainStatus = snapshot.itemIdentifiers(inSection: .mainStatus).compactMap { + if case .status(id: _, node: let node, _, _, _) = $0 { + return node + } else { + return nil + } + } + let parentsInCurrentSection = snapshot.itemIdentifiers(inSection: dataSource.sectionIdentifier(for: indexPath.section)!)[0.. UIContextMenuConfiguration? { return (collectionView.cellForItem(at: indexPath) as? TimelineStatusCollectionViewCell)?.contextMenuConfiguration() } diff --git a/Tusker/Screens/Conversation/ConversationTree.swift b/Tusker/Screens/Conversation/ConversationTree.swift index 0f132df3..19c47ba3 100644 --- a/Tusker/Screens/Conversation/ConversationTree.swift +++ b/Tusker/Screens/Conversation/ConversationTree.swift @@ -19,32 +19,56 @@ class ConversationNode { } } struct ConversationTree { - let ancestors: [String] - let descendants: [ConversationNode] - - static func build(from context: ConversationContext, mainStatus: StatusMO, descendants: [StatusMO]) -> ConversationTree { - let ancestors = getDirectParents(inReplyTo: mainStatus.inReplyToID, from: context) - let childThreads = getDescendantThreads(mainStatus: mainStatus, descendants: descendants) - return ConversationTree(ancestors: ancestors, descendants: childThreads) + let ancestors: [ConversationNode] + let mainStatus: ConversationNode + var descendants: [ConversationNode] { + mainStatus.children } - private static func getDirectParents(inReplyTo inReplyToID: String?, from context: ConversationContext) -> [String] { - var statuses = context.ancestors - var parents = [String]() + init(ancestors: [ConversationNode], mainStatus: ConversationNode) { + self.ancestors = ancestors + self.mainStatus = mainStatus + } + + static func build(for mainStatus: StatusMO, ancestors: [StatusMO], descendants: [StatusMO]) -> ConversationTree { + let mainStatusNode = ConversationNode(status: mainStatus) + let ancestors = buildAncestorNodes(mainStatusNode: mainStatusNode, ancestors: ancestors) + buildDescendantNodes(mainStatusNode: mainStatusNode, descendants: descendants) + return ConversationTree(ancestors: ancestors, mainStatus: mainStatusNode) + } + + private static func buildAncestorNodes(mainStatusNode: ConversationNode, ancestors: [StatusMO]) -> [ConversationNode] { + var statuses = ancestors + var parents = [ConversationNode]() - var parentID: String? = inReplyToID + var parentID: String? = mainStatusNode.status.inReplyToID while let currentParentID = parentID, let parentIndex = statuses.firstIndex(where: { $0.id == currentParentID }) { let parentStatus = statuses.remove(at: parentIndex) - parents.insert(parentStatus.id, at: 0) + + let node = ConversationNode(status: parentStatus) + parents.insert(node, at: 0) + parentID = parentStatus.inReplyToID } + // once the parents list is built and in-order, then we walk through and set each node's children + for (index, node) in parents.enumerated() { + if index == parents.count - 1 { + // the last parent is the direct parent of the main status + node.children = [mainStatusNode] + } else { + // otherwise, it's the parent of the status that comes immediately after it in the parents list + node.children = [parents[index + 1]] + } + } + return parents } - private static func getDescendantThreads(mainStatus: StatusMO, descendants: [StatusMO]) -> [ConversationNode] { + // doesn't return anything, since we're modifying the main status node in-place + private static func buildDescendantNodes(mainStatusNode: ConversationNode, descendants: [StatusMO]) { var descendants = descendants func removeAllInReplyTo(id: String) -> [StatusMO] { @@ -53,12 +77,11 @@ struct ConversationTree { return statuses } - let mainStatusNode = ConversationNode(status: mainStatus) var nodes: [String: ConversationNode] = [ - mainStatus.id: mainStatusNode + mainStatusNode.status.id: mainStatusNode ] - var idsToCheck = [mainStatus.id] + var idsToCheck = [mainStatusNode.status.id] while !idsToCheck.isEmpty { let inReplyToID = idsToCheck.removeFirst() @@ -74,7 +97,5 @@ struct ConversationTree { nodeForID.children.append(replyNode) } } - - return mainStatusNode.children } } diff --git a/Tusker/Screens/Conversation/ConversationViewController.swift b/Tusker/Screens/Conversation/ConversationViewController.swift index 167b6e7a..7254572c 100644 --- a/Tusker/Screens/Conversation/ConversationViewController.swift +++ b/Tusker/Screens/Conversation/ConversationViewController.swift @@ -77,6 +77,14 @@ class ConversationViewController: UIViewController { super.init(nibName: nil, bundle: nil) } + init(preloadedTree: ConversationTree, state mainStatusState: CollapseState, mastodonController: MastodonController) { + self.mode = .preloaded(preloadedTree) + self.mainStatusState = mainStatusState + self.mastodonController = mastodonController + + super.init(nibName: nil, bundle: nil) + } + required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") } @@ -115,9 +123,15 @@ class ConversationViewController: UIViewController { override func viewWillAppear(_ animated: Bool) { super.viewWillAppear(animated) - Task { - if case .unloaded = state { - await loadMainStatus() + if case .unloaded = state { + if case .preloaded(let tree) = mode { + // when everything is preloaded, we're on the fast path and want to avoid any async work + // just kicking off a MainActor task causes a delay before the content appears, even if the task doesn't suspend + mainStatusLoaded(tree.mainStatus.status) + } else { + Task { @MainActor in + await loadMainStatus() + } } } } @@ -142,9 +156,20 @@ class ConversationViewController: UIViewController { // MARK: Loading + @MainActor private func loadMainStatus() async { - guard let mainStatusID = await resolveStatusIfNecessary() else { - return + let mainStatusID: String + switch mode { + case .localID(let id): + mainStatusID = id + case .resolve(let url): + if let id = await resolveStatus(url: url) { + mainStatusID = id + } else { + return + } + case .preloaded(_): + fatalError("unreachable") } @MainActor @@ -166,7 +191,7 @@ class ConversationViewController: UIViewController { Task { await doLoadMainStatus() } - await mainStatusLoaded(cached) + mainStatusLoaded(cached) } else { // otherwise, show a loading indicator while loading the main status let indicator = UIActivityIndicatorView(style: .medium) @@ -174,74 +199,94 @@ class ConversationViewController: UIViewController { state = .loading(indicator) if let status = await doLoadMainStatus() { - await mainStatusLoaded(status) + mainStatusLoaded(status) } } } @MainActor - private func resolveStatusIfNecessary() async -> String? { - switch mode { - case .localID(let id): - return id - case .resolve(let url): - let indicator = UIActivityIndicatorView(style: .medium) - indicator.startAnimating() - state = .loading(indicator) - - let url = WebURL(url)!.serialized(excludingFragment: true) - let request = Client.search(query: url, types: [.statuses], resolve: true) - do { - let (results, _) = try await mastodonController.run(request) - guard let status = results.statuses.first(where: { $0.url?.serialized() == url }) else { - throw UnableToResolveError() - } - _ = mastodonController.persistentContainer.addOrUpdateOnViewContext(status: status) - mode = .localID(status.id) - return status.id - } catch { - state = .unableToResolve(error) - return nil + private func resolveStatus(url: URL) async -> String? { + let indicator = UIActivityIndicatorView(style: .medium) + indicator.startAnimating() + state = .loading(indicator) + + let url = WebURL(url)!.serialized(excludingFragment: true) + let request = Client.search(query: url, types: [.statuses], resolve: true) + do { + let (results, _) = try await mastodonController.run(request) + guard let status = results.statuses.first(where: { $0.url?.serialized() == url }) else { + throw UnableToResolveError() } + _ = mastodonController.persistentContainer.addOrUpdateOnViewContext(status: status) + mode = .localID(status.id) + return status.id + } catch { + state = .unableToResolve(error) + return nil } } - @MainActor - private func mainStatusLoaded(_ mainStatus: StatusMO) async { + private func mainStatusLoaded(_ mainStatus: StatusMO) { let vc = ConversationCollectionViewController(for: mainStatus.id, state: mainStatusState, mastodonController: mastodonController) vc.statusIDToScrollToOnLoad = statusIDToScrollToOnLoad ?? mainStatus.id vc.showStatusesAutomatically = showStatusesAutomatically vc.addMainStatus(mainStatus) state = .displaying(vc) - await loadContext(for: mainStatus) + if case .preloaded(let tree) = mode { + vc.addTree(tree, mainStatus: mainStatus) + } else { + Task { @MainActor in + await loadTree(for: mainStatus) + } + } } @MainActor - private func loadContext(for mainStatus: StatusMO) async { - guard case .displaying(_) = state else { + private func loadTree(for mainStatus: StatusMO) async { + guard case .displaying(_) = state, + let context = await loadContext(for: mainStatus) else { return } + await mastodonController.persistentContainer.addAll(statuses: context.ancestors + context.descendants) + + let ancestorIDs = context.ancestors.map(\.id) + let ancestorsReq = StatusMO.fetchRequest() + ancestorsReq.predicate = NSPredicate(format: "id in %@", ancestorIDs) + let ancestors = try? mastodonController.persistentContainer.viewContext.fetch(ancestorsReq) + + let descendantIDs = context.descendants.map(\.id) + let descendantsReq = StatusMO.fetchRequest() + descendantsReq.predicate = NSPredicate(format: "id IN %@", descendantIDs) + let descendants = try? mastodonController.persistentContainer.viewContext.fetch(descendantsReq) + + let tree = ConversationTree.build(for: mainStatus, ancestors: ancestors ?? [], descendants: descendants ?? []) + + guard case .displaying(let vc) = state else { + return + } + vc.addTree(tree, mainStatus: mainStatus) + } + + private func loadContext(for mainStatus: StatusMO) async -> ConversationContext? { let request = Status.getContext(mainStatus.id) do { let (context, _) = try await mastodonController.run(request) - guard case .displaying(let vc) = state else { - return - } - - await vc.addContext(context, for: mainStatus) + return context } catch { guard case .displaying(_) = state else { - return + return nil } let error = error as! Client.Error let config = ToastConfiguration(from: error, with: "Error Loading Context", in: self) { [weak self] toast in toast.dismissToast(animated: true) - await self?.loadContext(for: mainStatus) + await self?.loadTree(for: mainStatus) } self.showToast(configuration: config, animated: true) + + return nil } } @@ -341,6 +386,7 @@ extension ConversationViewController { enum Mode { case localID(String) case resolve(URL) + case preloaded(ConversationTree) } }