Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
280d66f
Add remote FF gravatar_quick_editor
pinarol Oct 30, 2024
2c5b3ae
Add forceRefresh option for image download methods
pinarol Oct 30, 2024
d6dee63
Force refresh on gravatar update notification
pinarol Oct 30, 2024
d25747d
Add `GravatarQuickEditorPresenter`
pinarol Oct 30, 2024
9737971
Display QuickEditor instead of the avatar upload menu
pinarol Oct 30, 2024
9fd8df8
Move listenForGravatarChanges to upper level
pinarol Oct 30, 2024
52a32cd
Add one more FF check
pinarol Oct 30, 2024
16cc08a
Adjust the obj-c call
pinarol Oct 30, 2024
2694718
Listen to changes on the MeHeaderView
pinarol Oct 30, 2024
7989b14
Await the image download to update the cached image
pinarol Oct 30, 2024
e33654f
Listen to gravatar changes on the signup epilogue
pinarol Oct 30, 2024
54bb02f
Add one more FF check
pinarol Oct 30, 2024
9c5f288
Separate the notification name for the QE
pinarol Oct 31, 2024
f3daef7
Merge branch 'trunk' into wppinar/integrate-gravatar-quick-editor
pinarol Oct 31, 2024
74a9dd7
Add a release note
pinarol Oct 31, 2024
8d191d4
Revert whitespace change
pinarol Oct 31, 2024
d4d8fd4
Merge branch 'trunk' into wppinar/integrate-gravatar-quick-editor
pinarol Oct 31, 2024
2fcba8d
Use overrideImageCache`
pinarol Oct 31, 2024
e85a655
Fix the old avatar issue
pinarol Nov 1, 2024
6b6e745
Merge branch 'trunk' into wppinar/integrate-gravatar-quick-editor
pinarol Nov 1, 2024
9247b27
Update unit test
pinarol Nov 1, 2024
896b756
Move `addObserver` to viewDidLoad
pinarol Nov 11, 2024
0ebbc55
Mode addObserver to init`
pinarol Nov 11, 2024
83e0a61
Add unit tests for `appendingGravatarCacheBusterParam` and handle the…
pinarol Nov 11, 2024
565fd9a
Merge branch 'trunk' into wppinar/integrate-gravatar-quick-editor
pinarol Nov 11, 2024
4b7b4fc
Fix "unused var" warning
pinarol Nov 15, 2024
239f45c
Merge branch 'trunk' into wppinar/integrate-gravatar-quick-editor
pinarol Nov 15, 2024
f0b3eb9
Update release notes
pinarol Nov 15, 2024
157f792
Revert "Update release notes"
pinarol Nov 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import Foundation

public enum GravatarQEAvatarUpdateNotificationKeys: String {
case email
}

public extension NSNotification.Name {
/// Gravatar Quick Editor updated the avatar
static let GravatarQEAvatarUpdateNotification = NSNotification.Name(rawValue: "GravatarQEAvatarUpdateNotification")
}

extension Foundation.Notification {
public func userInfoHasEmail(_ email: String) -> Bool {
guard let userInfo = userInfo,
let notificationEmail = userInfo[GravatarQEAvatarUpdateNotificationKeys.email.rawValue] as? String else {
return false
}
return email == notificationEmail
}
}
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
25.6
-----
* [*] [internal] Update Gravatar SDK to 3.0.0 [#23701]
* [*] Use the Gravatar Quick Editor to update the avatar [#23729]
* [*] (Hidden under a feature flag) User Management for self-hosted sites. [#23768]

25.5
Expand Down
11 changes: 11 additions & 0 deletions WordPress/Classes/Extensions/URL+Helpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,15 @@ extension URL {
components?.queryItems = queryItems
return components?.url ?? self
}

/// Gravatar doesn't support "Cache-Control: none" header. So we add a random query parameter to
/// bypass the backend cache and get the latest image.
public func appendingGravatarCacheBusterParam() -> URL {
var urlComponents = URLComponents(url: self, resolvingAgainstBaseURL: false)
if urlComponents?.queryItems == nil {
urlComponents?.queryItems = []
}
urlComponents?.queryItems?.append(.init(name: "_", value: "\(NSDate().timeIntervalSince1970)"))
return urlComponents?.url ?? self
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ enum RemoteFeatureFlag: Int, CaseIterable {
case siteMonitoring
case readingPreferencesFeedback
case inAppUpdates
case gravatarQuickEditor
case dotComWebLogin

var defaultValue: Bool {
Expand Down Expand Up @@ -84,6 +85,8 @@ enum RemoteFeatureFlag: Int, CaseIterable {
return true
case .inAppUpdates:
return false
case .gravatarQuickEditor:
return BuildConfiguration.current ~= [.localDeveloper, .a8cBranchTest, .a8cPrereleaseTesting]
case .dotComWebLogin:
return false
}
Expand Down Expand Up @@ -144,6 +147,8 @@ enum RemoteFeatureFlag: Int, CaseIterable {
return "reading_preferences_feedback"
case .inAppUpdates:
return "in_app_updates"
case .gravatarQuickEditor:
return "gravatar_quick_editor"
case .dotComWebLogin:
return "jp_wpcom_web_login"
}
Expand Down Expand Up @@ -203,6 +208,8 @@ enum RemoteFeatureFlag: Int, CaseIterable {
return "Reading Preferences Feedback"
case .inAppUpdates:
return "In-App Updates"
case .gravatarQuickEditor:
return "Gravatar Quick Editor"
case .dotComWebLogin:
return "Log in to WordPress.com from web browser"
}
Expand Down
11 changes: 7 additions & 4 deletions WordPress/Classes/Utility/Media/ImageDownloader+Gravatar.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,22 @@ import Gravatar

extension ImageDownloader {

nonisolated func downloadGravatarImage(with email: String, completion: @escaping (UIImage?) -> Void) {
nonisolated func downloadGravatarImage(with email: String, forceRefresh: Bool = false, completion: @escaping (UIImage?) -> Void) {

guard let url = AvatarURL.url(for: email) else {
completion(nil)
return
}

if let cachedImage = ImageCache.shared.getImage(forKey: url.absoluteString) {
if !forceRefresh, let cachedImage = ImageCache.shared.getImage(forKey: url.absoluteString) {
completion(cachedImage)
return
}

downloadImage(at: url) { image, _ in
var urlToDownload = url
if forceRefresh {
urlToDownload = url.appendingGravatarCacheBusterParam()
}
downloadImage(at: urlToDownload) { image, _ in
DispatchQueue.main.async {

guard let image else {
Expand Down
9 changes: 9 additions & 0 deletions WordPress/Classes/Utility/Media/ImageDownloader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,15 @@ actor ImageDownloader {
return imageURL.absoluteString + (size.map { "?size=\($0)" } ?? "")
}

func clearURLSessionCache() {
urlSessionWithCache.configuration.urlCache?.removeAllCachedResponses()
urlSession.configuration.urlCache?.removeAllCachedResponses()
}

func clearMemoryCache() {
self.cache.removeAllObjects()
}

// MARK: - Networking

private func data(for request: URLRequest, options: ImageRequestOptions) async throws -> Data {
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/Utility/Media/ImageViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ final class ImageViewController {
var downloader: ImageDownloader = .shared
var onStateChanged: (State) -> Void = { _ in }

private var task: Task<Void, Never>?
private(set) var task: Task<Void, Never>?

enum State {
case loading
Expand Down
6 changes: 6 additions & 0 deletions WordPress/Classes/Utility/Media/MemoryCache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import WordPressUI

protocol MemoryCacheProtocol: AnyObject {
subscript(key: String) -> UIImage? { get set }
func removeAllObjects()
}

/// - note: The type is thread-safe because it uses thread-safe `NSCache`.
final class MemoryCache: MemoryCacheProtocol, @unchecked Sendable {

/// A shared image cache used by the entire system.
static let shared = MemoryCache()

Expand All @@ -23,6 +25,10 @@ final class MemoryCache: MemoryCacheProtocol, @unchecked Sendable {
cache.removeAllObjects()
}

func removeAllObjects() {
cache.removeAllObjects()
}

// MARK: - UIImage

subscript(key: String) -> UIImage? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import Gravatar

extension BlogDetailsViewController {

@objc func downloadGravatarImage(for row: BlogDetailsRow) {
@objc func downloadGravatarImage(for row: BlogDetailsRow, forceRefresh: Bool = false) {
guard let email = blog.account?.email else {
return
}

ImageDownloader.shared.downloadGravatarImage(with: email) { [weak self] image in
ImageDownloader.shared.downloadGravatarImage(with: email, forceRefresh: forceRefresh) { [weak self] image in
guard let image,
let gravatarIcon = image.gravatarIcon(size: Metrics.iconSize) else {
return
Expand All @@ -21,9 +21,17 @@ extension BlogDetailsViewController {
}

@objc func observeGravatarImageUpdate() {
NotificationCenter.default.addObserver(self, selector: #selector(refreshAvatar(_:)), name: .GravatarQEAvatarUpdateNotification, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(updateGravatarImage(_:)), name: .GravatarImageUpdateNotification, object: nil)
}

@objc private func refreshAvatar(_ notification: Foundation.Notification) {
guard let meRow,
let email = blog.account?.email,
notification.userInfoHasEmail(email) else { return }
downloadGravatarImage(for: meRow, forceRefresh: true)
}

@objc private func updateGravatarImage(_ notification: Foundation.Notification) {
guard let userInfo = notification.userInfo,
let email = userInfo["email"] as? String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1350,7 +1350,7 @@ - (BlogDetailsSection *)configurationSectionViewModel
callback:^{
[weakSelf showMe];
}];
[self downloadGravatarImageFor:row];
[self downloadGravatarImageFor:row forceRefresh: NO];
self.meRow = row;
[rows addObject:row];
}
Expand Down
33 changes: 26 additions & 7 deletions WordPress/Classes/ViewRelated/Gravatar/UIImageView+Gravatar.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,18 @@ extension UIImageView {
/// - email: The user's email
/// - gravatarRating: Expected image rating
/// - placeholderImage: Image to be used as Placeholder
/// - forceRefresh: Skip the cache and fetch the latest version of the avatar.
public func downloadGravatar(
for email: String,
gravatarRating: Rating = .general,
placeholderImage: UIImage = .gravatarPlaceholderImage
placeholderImage: UIImage = .gravatarPlaceholderImage,
forceRefresh: Bool = false
) {
let avatarURL = AvatarURL.url(for: email, preferredSize: .pixels(gravatarDefaultSize()), gravatarRating: gravatarRating)
downloadGravatar(fullURL: avatarURL, placeholder: placeholderImage, animate: false)
downloadGravatar(fullURL: avatarURL, placeholder: placeholderImage, animate: false, forceRefresh: forceRefresh)
}

public func downloadGravatar(_ gravatar: AvatarURL?, placeholder: UIImage, animate: Bool) {
public func downloadGravatar(_ gravatar: AvatarURL?, placeholder: UIImage, animate: Bool, forceRefresh: Bool = false) {
guard let gravatar = gravatar else {
self.image = placeholder
return
Expand All @@ -56,14 +58,31 @@ extension UIImageView {
layoutIfNeeded()

let size = Int(ceil(frame.width * min(2, UIScreen.main.scale)))
let url = gravatar.replacing(options: .init(preferredSize: .pixels(size)))?.url
downloadGravatar(fullURL: url, placeholder: placeholder, animate: animate)
guard let url = gravatar.replacing(options: .init(preferredSize: .pixels(size)))?.url else { return }
downloadGravatar(fullURL: url, placeholder: placeholder, animate: animate, forceRefresh: forceRefresh)
}

private func downloadGravatar(fullURL: URL?, placeholder: UIImage, animate: Bool) {
private func downloadGravatar(fullURL: URL?, placeholder: UIImage, animate: Bool, forceRefresh: Bool = false) {
wp.prepareForReuse()
if let fullURL {
wp.setImage(with: fullURL)
var urlToDownload = fullURL
if forceRefresh {
urlToDownload = fullURL.appendingGravatarCacheBusterParam()
}

wp.setImage(with: urlToDownload)

if forceRefresh {
// If this is a `forceRefresh`, the cache for the original URL should be updated too.
// Because the cache buster parameter modifies the download URL.
Task {
await wp.controller.task?.value // Wait until setting the new image is done.
if let image {
ImageCache.shared.setImage(image, forKey: fullURL.absoluteString) // Update the cache for the original URL
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about these changes. @kean Can you have a look please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The app clears caches on onAvatarUpdated and they will be automatically re-populated later. I'm not sure it's worth adding complexity and making task getter internal for this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we shouldn't need this yes. But unfortunately there's a delay at the backend that causes the old avatar to return for like 25 seconds after selecting a different avatar. So the old avatar comes back on the next request that doesn't have the cache buster parameter and looks like this:

I reported this issue to systems and following it. In the meantime I update the memory cache for the cache buster-less version of the URL so the correct image can be shown next time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this doesn't look great, especially since it replaced the new avatar with the old one. I don't see anything particular wrong with adding this bit of code, so I'm going to leave it up to you.

if image == nil { // If image wasn't found synchronously in memory cache
image = placeholder
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import Foundation
import GravatarUI
import WordPressShared
import WordPressAuthenticator

@MainActor
struct GravatarQuickEditorPresenter {
let email: String
let authToken: String

init?(email: String) {
let context = ContextManager.sharedInstance().mainContext
guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: context) else {
return nil
}
self.email = email
self.authToken = account.authToken
}

func presentQuickEditor(on presentingViewController: UIViewController) {
let presenter = QuickEditorPresenter(
email: Email(email),
scope: .avatarPicker(AvatarPickerConfiguration(contentLayout: .horizontal())),
configuration: .init(
interfaceStyle: presentingViewController.traitCollection.userInterfaceStyle
),
token: authToken
)
presenter.present(
in: presentingViewController,
onAvatarUpdated: {
AuthenticatorAnalyticsTracker.shared.track(click: .selectAvatar)
Task {
// Purge the cache otherwise the old avatars remain around.
await ImageDownloader.shared.clearURLSessionCache()
await ImageDownloader.shared.clearMemoryCache()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purging all cache entries seems a bit aggressive. Would only removing the cached image for email work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite being connected to the same email, avatars scattered throughout the app have usually different URLs depending on the query parameters like size, default image option, rating etc.

And unfortunately URLCache or NSCache doesn't give us chance to iterate over the cache keys and filter based on host and path. We need to know the exact URL to remove an entry which we don't.

Alternatively, we can remove these calls altogether but this causes some avatars to remain old in different places of the app which looks inconsistent and faulty. Also, killing & reopening the app can bring in the old avatar since URLCache has a disk cache. So I recommend keeping these purge operations but let me know if you prefer otherwise.

NotificationCenter.default.post(name: .GravatarQEAvatarUpdateNotification,
object: self,
userInfo: [GravatarQEAvatarUpdateNotificationKeys.email.rawValue: email])
}
}, onDismiss: {
// No op.
}
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class MyProfileHeaderView: UITableViewHeaderFooterView {
// MARK: - Public Properties and Outlets
@IBOutlet var gravatarImageView: CircularImageView!
@IBOutlet var gravatarButton: UIButton!

weak var presentingViewController: UIViewController?
// A fake button displayed on top of gravatarImageView.
let imageViewButton = UIButton(type: .system)

Expand All @@ -24,8 +24,8 @@ class MyProfileHeaderView: UITableViewHeaderFooterView {
}
var gravatarEmail: String? = nil {
didSet {
if let email = gravatarEmail {
gravatarImageView.downloadGravatar(for: email, gravatarRating: .x)
if gravatarEmail != nil {
downloadAvatar()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It produces a new Xcode warning (email unused).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I missed this. Fixed. 👍

}
}
}
Expand All @@ -51,10 +51,23 @@ class MyProfileHeaderView: UITableViewHeaderFooterView {
configureGravatarButton()
}

private func downloadAvatar(forceRefresh: Bool = false) {
if let email = gravatarEmail {
gravatarImageView.downloadGravatar(for: email, gravatarRating: .x, forceRefresh: forceRefresh)
}
}

@objc private func refreshAvatar(_ notification: Foundation.Notification) {
guard let email = gravatarEmail,
notification.userInfoHasEmail(email) else { return }
downloadAvatar(forceRefresh: true)
}

/// Overrides the current Gravatar Image (set via Email) with a given image reference.
/// Plus, the internal image cache is updated, to prevent undesired glitches upon refresh.
///
func overrideGravatarImage(_ image: UIImage) {
guard !RemoteFeatureFlag.gravatarQuickEditor.enabled() else { return }
gravatarImageView.image = image

// Note:
Expand All @@ -81,9 +94,23 @@ class MyProfileHeaderView: UITableViewHeaderFooterView {
gravatarImageView.addSubview(imageViewButton)
imageViewButton.translatesAutoresizingMaskIntoConstraints = false
imageViewButton.pinSubviewToAllEdges(gravatarImageView)
if RemoteFeatureFlag.gravatarQuickEditor.enabled() {
imageViewButton.addTarget(self, action: #selector(gravatarButtonTapped), for: .touchUpInside)
NotificationCenter.default.addObserver(self, selector: #selector(refreshAvatar), name: .GravatarQEAvatarUpdateNotification, object: nil)
}
}

private func configureGravatarButton() {
gravatarButton.tintColor = UIAppColor.primary
if RemoteFeatureFlag.gravatarQuickEditor.enabled() {
gravatarButton.addTarget(self, action: #selector(gravatarButtonTapped), for: .touchUpInside)
}
}

@objc private func gravatarButtonTapped() {
guard let email = gravatarEmail,
let presenter = GravatarQuickEditorPresenter(email: email),
let presentingViewController else { return }
presenter.presentQuickEditor(on: presentingViewController)
}
}
Loading