-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Integrate Gravatar Quick Editor #23729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
280d66f
2c5b3ae
d6dee63
d25747d
9737971
9fd8df8
52a32cd
16cc08a
2694718
7989b14
e33654f
54bb02f
9c5f288
f3daef7
74a9dd7
8d191d4
d4d8fd4
2fcba8d
e85a655
6b6e745
9247b27
896b756
0ebbc55
83e0a61
565fd9a
4b7b4fc
239f45c
f0b3eb9
157f792
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The app clears caches on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
|
|
||
| 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() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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) | ||
|
|
||
|
|
@@ -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() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It produces a new Xcode warning (email unused).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I missed this. Fixed. 👍 |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -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: | ||
|
|
@@ -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) | ||
| } | ||
| } | ||

Uh oh!
There was an error while loading. Please reload this page.