Automattic / Gravatar-SDK-iOS

Gravatar SDK is a Swift library that allows you to integrate Gravatar features into your own iOS applications.
https://gravatar.com
Mozilla Public License 2.0
36 stars 1 forks source link

Remove "async" from ImageCaching #386

Closed pinarol closed 6 days ago

pinarol commented 1 week ago

Solves a problem where a UI glitch occurs because of the async access to the cached image. The problem was first reported here: https://github.com/wordpress-mobile/WordPress-iOS/pull/23534#discussion_r1745530517

Check out the small "Me" icon:

https://github.com/user-attachments/assets/a3d1a82d-4897-4a0e-9b6c-5bb2346354b7

Description

Some History

We updated ImageCaching to comply with the new concurrency model with this PR: https://github.com/Automattic/Gravatar-SDK-iOS/pull/252 . As described there, we discovered that making protocol functions async doesn’t dictate it on the implementations. Types conforming to ImageCaching could still have their non-async setEntry(…), getEntry(..) implementations. So keeping the protocol functions async was the more "inclusive" way.

The problem

But here’s the downside, “Gravatar.ImageCache.shared” is public, so one can just directly get the cached image from there. However, since these methods are async, the system can suspend there to do sth else. So the UI change can’t happen immediately in a synchronous way and can cause UI glitches.

Task {
    if let cachedEntry = await Gravatar.ImageCache.shared.getEntry(with: url.absoluteString) {
        switch cachedEntry {
        case .ready(let image):
            await MainActor.run {
                completion(image) // here the image is updated in the UI
            }
        default:
            break
     }
 }

One should be able to just:

if let cachedEntry = Gravatar.ImageCache.shared.getEntry(with: url.absoluteString) {
      switch cachedEntry {
      case .ready(let image):
          completion(image) // the image is updated in the UI, the whole operation is done synchronously.
      default:
          break
      }
}

Is this critical/urgent?

Nope. WordPress injects its own cache to the Gravatar SDK, so they can access their cache synchronously inside the repo. This is more of a problem for cases where they rely on the SDK's cache.

Testing Steps

CI should be sufficient but let's also smoke test the UIKit demo app.

wpmobilebot commented 1 week ago
Gravatar SwiftUI Prototype Build📲 You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar SwiftUI Prototype Build Gravatar SwiftUI Prototype Build
Build Number1216
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-swiftui.prototype-build
Commit564c1ac2186c045d67edab00bc30c3fe4faafa81
App Center BuildGravatar SDK Demo - SwiftUI #104
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.
wpmobilebot commented 1 week ago
Gravatar UIKit Prototype Build📲 You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar UIKit Prototype Build Gravatar UIKit Prototype Build
Build Number1216
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-uikit.prototype-build
Commit564c1ac2186c045d67edab00bc30c3fe4faafa81
App Center BuildGravatar SDK Demo - UIKit #105
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.