From d634ed433764411b1c274d8b0032060f161382d1 Mon Sep 17 00:00:00 2001 From: Alex Grebenyuk Date: Thu, 19 Dec 2024 17:17:54 -0500 Subject: [PATCH] Fix off-by-one error in post likes (#23912) * Fix formatting in getLikesFor * Refactor ReaderDetailLikesView * Simplify ReaderDetailLikesView further * Update release notes * Fix compilation in tests --- RELEASE-NOTES.txt | 1 + .../Detail/ReaderDetailCoordinator.swift | 95 +++++---- .../Detail/ReaderDetailViewController.swift | 42 +--- .../Detail/Views/ReaderDetailLikesView.swift | 192 ++++++------------ .../Detail/Views/ReaderDetailToolbar.swift | 2 - .../ReaderDetailCoordinatorTests.swift | 4 +- 6 files changed, 109 insertions(+), 227 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 9af4c2d44136..63410a79a5b0 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -19,6 +19,7 @@ * [*] Reader: The post cover now uses the standard aspect ratio for covers, so there is no jumping. There are also a few minor improvements to the layout and animations of the cover and the header [#23897, #23909] * [*] Reader: Move the "Reading Preferences" button to the "More" menu [#23897] * [*] Reader: Hide post toolbar when reading an article and fix like button animations [#23909] +* [*] Reader: Fix off-by-one error in post details like counter when post is liked by you [#23912] 25.5 ----- diff --git a/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailCoordinator.swift b/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailCoordinator.swift index 82fabfab60e0..4aa343b2e725 100644 --- a/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailCoordinator.swift +++ b/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailCoordinator.swift @@ -1,5 +1,6 @@ import Foundation import WordPressShared +import Combine class ReaderDetailCoordinator { @@ -43,6 +44,8 @@ class ReaderDetailCoordinator { /// Called if the view controller's post fails to load var postLoadFailureBlock: (() -> Void)? = nil + private var likesAvatarURLs: [String]? + /// An authenticator to ensure any request made to WP sites is properly authenticated lazy var authenticator: RequestAuthenticator? = { guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: coreDataStack.mainContext) else { @@ -102,10 +105,7 @@ class ReaderDetailCoordinator { } private var followCommentsService: FollowCommentsService? - - /// The total number of Likes for the post. - /// Passed to ReaderDetailLikesListController to display in the view title. - private var totalLikes = 0 + private var likesObserver: AnyCancellable? /// Initialize the Reader Detail Coordinator /// @@ -156,41 +156,50 @@ class ReaderDetailCoordinator { } } - /// Fetch Likes for the current post. - /// Returns `ReaderDetailLikesView.maxAvatarsDisplayed` number of Likes. - /// func fetchLikes(for post: ReaderPost) { - guard let postID = post.postID else { - return - } + guard let postID = post.postID else { return } // Fetch a full page of Likes but only return the `maxAvatarsDisplayed` number. // That way the first page will already be cached if the user displays the full Likes list. - postService.getLikesFor(postID: postID, - siteID: post.siteID, - success: { [weak self] users, totalLikes, _ in - var filteredUsers = users - var currentLikeUser: LikeUser? = nil - let totalLikesExcludingSelf = totalLikes - (post.isLiked ? 1 : 0) - - // Split off current user's like from the list. - // Likes from self will always be placed in the last position, regardless of the when the post was liked. - if let userID = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.userID.int64Value, - let userIndex = filteredUsers.firstIndex(where: { $0.userID == userID }) { - currentLikeUser = filteredUsers.remove(at: userIndex) - } - - self?.totalLikes = totalLikes - self?.view?.updateLikes(with: filteredUsers.prefix(ReaderDetailLikesView.maxAvatarsDisplayed).map { $0.avatarUrl }, - totalLikes: totalLikesExcludingSelf) - // Only pass current user's avatar when we know *for sure* that the post is liked. - // This is to work around a possible race condition that causes an unliked post to have current user's LikeUser, which - // would cause a display bug in ReaderDetailLikesView. The race condition issue will be investigated separately. - self?.view?.updateSelfLike(with: post.isLiked ? currentLikeUser?.avatarUrl : nil) - }, failure: { [weak self] error in - self?.view?.updateLikes(with: [String](), totalLikes: 0) - DDLogError("Error fetching Likes for post detail: \(String(describing: error?.localizedDescription))") - }) + postService.getLikesFor(postID: postID, siteID: post.siteID, success: { [weak self] users, totalLikes, _ in + guard let self else { return } + + var filteredUsers = users + if let userID = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.userID.int64Value, + let userIndex = filteredUsers.firstIndex(where: { $0.userID == userID }) { + filteredUsers.remove(at: userIndex) + } + + self.likesAvatarURLs = filteredUsers.prefix(ReaderDetailLikesView.maxAvatarsDisplayed).map(\.avatarUrl) + self.updateLikesView() + self.startObservingLikes() + }, failure: { error in + DDLogError("Error fetching Likes for post detail: \(String(describing: error?.localizedDescription))") + }) + } + + private func startObservingLikes() { + guard let post else { + return wpAssertionFailure("post missing") + } + + likesObserver = Publishers.CombineLatest( + post.publisher(for: \.likeCount, options: [.new]).removeDuplicates(), + post.publisher(for: \.isLiked, options: [.new]).removeDuplicates() + ).sink { [weak self] _, _ in + self?.updateLikesView() + } + } + + private func updateLikesView() { + guard let post, let likesAvatarURLs else { return } + + let viewModel = ReaderDetailLikesViewModel( + likeCount: post.likeCount.intValue, + avatarURLs: likesAvatarURLs, + selfLikeAvatarURL: post.isLiked ? try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.avatarURL : nil + ) + view?.updateLikesView(with: viewModel) } /// Fetch Comments for the current post. @@ -590,8 +599,7 @@ class ReaderDetailCoordinator { guard let post else { return } - - let controller = ReaderDetailLikesListController(post: post, totalLikes: totalLikes) + let controller = ReaderDetailLikesListController(post: post, totalLikes: post.likeCount.intValue) viewController?.navigationController?.pushViewController(controller, animated: true) } @@ -712,30 +720,19 @@ extension ReaderDetailCoordinator: ReaderDetailHeaderViewDelegate { } } -// MARK: - ReaderDetailFeaturedImageViewDelegate extension ReaderDetailCoordinator: ReaderDetailFeaturedImageViewDelegate { func didTapFeaturedImage(_ sender: AsyncImageView) { showFeaturedImage(sender) } } -// MARK: - ReaderDetailLikesViewDelegate extension ReaderDetailCoordinator: ReaderDetailLikesViewDelegate { func didTapLikesView() { showLikesList() } } -// MARK: - ReaderDetailToolbarDelegate -extension ReaderDetailCoordinator: ReaderDetailToolbarDelegate { - func didTapLikeButton(isLiked: Bool) { - guard let userAvatarURL = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext)?.avatarURL else { - return - } - - self.view?.updateSelfLike(with: isLiked ? userAvatarURL : nil) - } -} +extension ReaderDetailCoordinator: ReaderDetailToolbarDelegate {} // MARK: - Private Definitions diff --git a/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailViewController.swift b/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailViewController.swift index c671a3a5e456..8ed9af3308fd 100644 --- a/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailViewController.swift +++ b/WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailViewController.swift @@ -12,20 +12,7 @@ protocol ReaderDetailView: AnyObject { func showErrorWithWebAction() func scroll(to: String) func updateHeader() - - /// Shows likes view containing avatars of users that liked the post. - /// The number of avatars displayed is limited to `ReaderDetailView.maxAvatarDisplayed` plus the current user's avatar. - /// Note that the current user's avatar is displayed through a different method. - /// - /// - Seealso: `updateSelfLike(with avatarURLString: String?)` - /// - Parameters: - /// - avatarURLStrings: A list of URL strings for the liking users' avatars. - /// - totalLikes: The total number of likes for this post. - func updateLikes(with avatarURLStrings: [String], totalLikes: Int) - - /// Updates the likes view to append an additional avatar for the current user, indicating that the post is liked by current user. - /// - Parameter avatarURLString: The URL string for the current user's avatar. Optional. - func updateSelfLike(with avatarURLString: String?) + func updateLikesView(with viewModel: ReaderDetailLikesViewModel) /// Updates comments table to display the post's comments. /// - Parameters: @@ -374,38 +361,17 @@ class ReaderDetailViewController: UIViewController, ReaderDetailView { header.refreshFollowButton() } - func updateLikes(with avatarURLStrings: [String], totalLikes: Int) { - // always configure likes summary view first regardless of totalLikes, since it can affected by self likes. - likesSummary.configure(with: avatarURLStrings, totalLikes: totalLikes) - - guard totalLikes > 0 else { + func updateLikesView(with viewModel: ReaderDetailLikesViewModel) { + guard viewModel.likeCount > 0 else { hideLikesView() return } - if likesSummary.superview == nil { configureLikesSummary() } - scrollView.layoutIfNeeded() - } - - func updateSelfLike(with avatarURLString: String?) { - // only animate changes when the view is visible. - let shouldAnimate = isVisibleInScrollView(likesSummary) - guard let someURLString = avatarURLString else { - likesSummary.removeSelfAvatar(animated: shouldAnimate) - if likesSummary.totalLikesForDisplay == 0 { - hideLikesView() - } - return - } - - if likesSummary.superview == nil { - configureLikesSummary() - } - likesSummary.addSelfAvatar(with: someURLString, animated: shouldAnimate) + likesSummary.configure(with: viewModel) } func updateComments(_ comments: [Comment], totalComments: Int) { diff --git a/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailLikesView.swift b/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailLikesView.swift index f92a6bd4301d..50c654170bcf 100644 --- a/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailLikesView.swift +++ b/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailLikesView.swift @@ -4,17 +4,17 @@ protocol ReaderDetailLikesViewDelegate: AnyObject { func didTapLikesView() } -class ReaderDetailLikesView: UIView, NibLoadable { - - @IBOutlet weak var avatarStackView: UIStackView! - @IBOutlet weak var summaryLabel: UILabel! - - // Theming support +final class ReaderDetailLikesView: UIView, NibLoadable { + @IBOutlet private weak var avatarStackView: UIStackView! + @IBOutlet private weak var summaryLabel: UILabel! + @IBOutlet private weak var selfAvatarImageView: CircularImageView! var displaySetting: ReaderDisplaySetting = .standard { didSet { applyStyles() - updateSummaryLabel() + if let viewModel { + configure(with: viewModel) + } } } @@ -22,114 +22,55 @@ class ReaderDetailLikesView: UIView, NibLoadable { displaySetting.color == .system ? .systemBackground : displaySetting.color.background } - /// The UIImageView used to display the current user's avatar image. This view is hidden by default. - @IBOutlet private weak var selfAvatarImageView: CircularImageView! - static let maxAvatarsDisplayed = 5 - weak var delegate: ReaderDetailLikesViewDelegate? - - /// Stores the number of total likes _without_ adding the like from self. - private var totalLikes: Int = 0 - /// Convenience property that adds up the total likes and self like for display purposes. - var totalLikesForDisplay: Int { - return displaysSelfAvatar ? totalLikes + 1 : totalLikes - } + weak var delegate: ReaderDetailLikesViewDelegate? - /// Convenience property that checks whether or not the self avatar image view is being displayed. - private var displaysSelfAvatar: Bool { - !selfAvatarImageView.isHidden - } + private var viewModel: ReaderDetailLikesViewModel? override func awakeFromNib() { super.awakeFromNib() - applyStyles() - } - func configure(with avatarURLStrings: [String], totalLikes: Int) { - self.totalLikes = totalLikes - updateSummaryLabel() - updateAvatars(with: avatarURLStrings) + applyStyles() addTapGesture() } - func addSelfAvatar(with urlString: String, animated: Bool = false) { - downloadGravatar(for: selfAvatarImageView, withURL: urlString) - - // pre-animation state - // set initial position from the left in LTR, or from the right in RTL. - selfAvatarImageView.alpha = 0 - let directionalMultiplier: CGFloat = userInterfaceLayoutDirection() == .leftToRight ? -1.0 : 1.0 - selfAvatarImageView.transform = CGAffineTransform(translationX: Constants.animationDeltaX * directionalMultiplier, y: 0) + func configure(with viewModel: ReaderDetailLikesViewModel) { + self.viewModel = viewModel - UIView.animate(withDuration: animated ? Constants.animationDuration : 0) { - // post-animation state - self.selfAvatarImageView.alpha = 1 - self.selfAvatarImageView.isHidden = false - self.selfAvatarImageView.transform = .identity - } + summaryLabel.attributedText = makeHighlightedText(Strings.formattedLikeCount(viewModel.likeCount), displaySetting: displaySetting) - updateSummaryLabel() - } + updateAvatars(with: viewModel.avatarURLs) - func removeSelfAvatar(animated: Bool = false) { - // pre-animation state - selfAvatarImageView.alpha = 1 - self.selfAvatarImageView.transform = .identity - - UIView.animate(withDuration: animated ? Constants.animationDuration : 0) { - // post-animation state - // moves to the left in LTR, or to the right in RTL. - self.selfAvatarImageView.alpha = 0 - self.selfAvatarImageView.isHidden = true - let directionalMultiplier: CGFloat = self.userInterfaceLayoutDirection() == .leftToRight ? -1.0 : 1.0 - self.selfAvatarImageView.transform = CGAffineTransform(translationX: Constants.animationDeltaX * directionalMultiplier, y: 0) + selfAvatarImageView.isHidden = viewModel.selfLikeAvatarURL == nil + if let avatarURL = viewModel.selfLikeAvatarURL { + downloadGravatar(for: selfAvatarImageView, withURL: avatarURL) } - - updateSummaryLabel() } override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) { super.traitCollectionDidChange(previousTraitCollection) applyStyles() } - } private extension ReaderDetailLikesView { - @MainActor func applyStyles() { - // Set border on all the avatar views for subView in avatarStackView.subviews { subView.layer.borderWidth = 1 subView.layer.borderColor = preferredBorderColor.cgColor } } - func updateSummaryLabel() { - switch (displaysSelfAvatar, totalLikes) { - case (true, 0): - summaryLabel.attributedText = NSAttributedString(string: SummaryLabelFormats.onlySelf) - case (true, 1): - summaryLabel.attributedText = highlightedText(String(format: SummaryLabelFormats.plural, 2)) - case (false, 1): - summaryLabel.attributedText = highlightedText(SummaryLabelFormats.singular) - default: - summaryLabel.attributedText = highlightedText(String(format: SummaryLabelFormats.plural, totalLikes)) - } - } - func updateAvatars(with urlStrings: [String]) { for (index, subView) in avatarStackView.subviews.enumerated() { guard let avatarImageView = subView as? UIImageView else { return } - if avatarImageView == selfAvatarImageView { continue } - if let urlString = urlStrings[safe: index] { downloadGravatar(for: avatarImageView, withURL: urlString) } else { @@ -139,83 +80,64 @@ private extension ReaderDetailLikesView { } func downloadGravatar(for avatarImageView: UIImageView, withURL url: String?) { - // Always reset gravatar - avatarImageView.cancelImageDownload() + avatarImageView.wp.prepareForReuse() avatarImageView.image = .gravatarPlaceholderImage - - guard let url, - let gravatarURL = URL(string: url) else { - return + if let url, let gravatarURL = URL(string: url) { + avatarImageView.wp.setImage(with: gravatarURL) } - - avatarImageView.downloadImage(from: gravatarURL, placeholderImage: .gravatarPlaceholderImage) } func addTapGesture() { - addGestureRecognizer(UITapGestureRecognizer(target: self, action: #selector(didTapView(_:)))) + addGestureRecognizer(UITapGestureRecognizer(target: self, action: #selector(didTapView))) } @objc func didTapView(_ gesture: UITapGestureRecognizer) { - guard gesture.state == .ended else { - return - } - if displaysSelfAvatar && totalLikes == 0 { - // Only the current user liked the post - return - } - delegate?.didTapLikesView() } +} - struct Constants { - static let animationDuration: TimeInterval = 0.3 - static let animationDeltaX: CGFloat = 16.0 - } +private func makeHighlightedText(_ text: String, displaySetting: ReaderDisplaySetting) -> NSAttributedString { + let labelParts = text.components(separatedBy: "_") - struct SummaryLabelFormats { - static let onlySelf = NSLocalizedString( - "reader.detail.likes.self", - value: "You like this.", - comment: "Describes that the current user is the only one liking a post." - ) - static let singular = NSLocalizedString( - "reader.detail.likes.single", - value: "_1 like_", - comment: "Describes that only one user likes a post. " - + " The underscores denote underline and is not displayed." - ) - static let plural = NSLocalizedString( - "reader.detail.likes.plural", - value: "_%1$d likes_", - comment: "Plural format string for displaying the number of post likes." - + " %1$d is the number of likes. The underscores denote underline and is not displayed." - ) - } + let firstPart = labelParts.first ?? "" + let countPart = labelParts[safe: 1] ?? "" + let lastPart = labelParts.last ?? "" - func highlightedText(_ text: String) -> NSAttributedString { - let labelParts = text.components(separatedBy: "_") + let foregroundColor = displaySetting.color.secondaryForeground + let highlightedColor = displaySetting.color == .system ? UIAppColor.primary : displaySetting.color.foreground - let firstPart = labelParts.first ?? "" - let countPart = labelParts[safe: 1] ?? "" - let lastPart = labelParts.last ?? "" + let foregroundAttributes: [NSAttributedString.Key: Any] = [.foregroundColor: foregroundColor] + var highlightedAttributes: [NSAttributedString.Key: Any] = [.foregroundColor: highlightedColor] - let foregroundColor = displaySetting.color.secondaryForeground - let highlightedColor = displaySetting.color == .system ? UIAppColor.primary : displaySetting.color.foreground + if displaySetting.color != .system { + // apply underline and semibold weight for color themes other than `.system`. + highlightedAttributes[.font] = displaySetting.font(with: .footnote, weight: .semibold) + highlightedAttributes[.underlineStyle] = NSUnderlineStyle.single.rawValue + } - let foregroundAttributes: [NSAttributedString.Key: Any] = [.foregroundColor: foregroundColor] - var highlightedAttributes: [NSAttributedString.Key: Any] = [.foregroundColor: highlightedColor] + let attributedString = NSMutableAttributedString(string: firstPart, attributes: foregroundAttributes) + attributedString.append(NSAttributedString(string: countPart, attributes: highlightedAttributes)) + attributedString.append(NSAttributedString(string: lastPart, attributes: foregroundAttributes)) - if displaySetting.color != .system { - // apply underline and semibold weight for color themes other than `.system`. - highlightedAttributes[.font] = displaySetting.font(with: .footnote, weight: .semibold) - highlightedAttributes[.underlineStyle] = NSUnderlineStyle.single.rawValue - } + return attributedString +} - let attributedString = NSMutableAttributedString(string: firstPart, attributes: foregroundAttributes) - attributedString.append(NSAttributedString(string: countPart, attributes: highlightedAttributes)) - attributedString.append(NSAttributedString(string: lastPart, attributes: foregroundAttributes)) +struct ReaderDetailLikesViewModel { + /// A total like count, including your likes. + var likeCount: Int + /// Avatar URLs excluding self-like view. + var avatarURLs: [String] + var selfLikeAvatarURL: String? +} - return attributedString - } +private enum Strings { + static let likeCountSingular = NSLocalizedString("reader.detail.likes.single", value: "_1 like_", comment: "Describes that only one user likes a post. The underscores denote underline and is not displayed.") + static let likeCountPlural = NSLocalizedString("reader.detail.likes.plural", value: "_%1$d likes_", comment: "Plural format string for displaying the number of post likes. %1$d is the number of likes. The underscores denote underline and is not displayed.") + static func formattedLikeCount(_ likeCount: Int) -> String { + switch likeCount { + case 1: Strings.likeCountSingular + default: String(format: Strings.likeCountPlural, likeCount) + } + } } diff --git a/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailToolbar.swift b/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailToolbar.swift index 1d6d048cd8c0..1d2d814e6c6e 100644 --- a/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailToolbar.swift +++ b/WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailToolbar.swift @@ -2,7 +2,6 @@ import UIKit import WordPressUI protocol ReaderDetailToolbarDelegate: AnyObject { - func didTapLikeButton(isLiked: Bool) var notificationID: String? { get } } @@ -460,7 +459,6 @@ private extension ReaderDetailToolbar { } self?.configureLikeActionButton(true) - self?.delegate?.didTapLikeButton(isLiked: updatedPost.isLiked) } commentCountObserver = post?.observe(\.commentCount, options: [.old, .new]) { [weak self] _, change in diff --git a/WordPress/WordPressTest/ReaderDetailCoordinatorTests.swift b/WordPress/WordPressTest/ReaderDetailCoordinatorTests.swift index 3237639075bd..ea25945dabb2 100644 --- a/WordPress/WordPressTest/ReaderDetailCoordinatorTests.swift +++ b/WordPress/WordPressTest/ReaderDetailCoordinatorTests.swift @@ -343,9 +343,7 @@ private class ReaderDetailViewMock: UIViewController, ReaderDetailView { func updateHeader() { } - func updateLikes(with avatarURLStrings: [String], totalLikes: Int) { } - - func updateSelfLike(with avatarURLString: String?) { } + func updateLikesView(with viewModel: ReaderDetailLikesViewModel) {} func updateComments(_ comments: [Comment], totalComments: Int) { }