[Paywalls V2] Extract ImageLoader to use a single one by tonidero · Pull Request #2146 · RevenueCat/purchases-android

tonidero · 2025-02-12T11:59:59Z

Description

We should be using a single ImageLoader but we're actually creating a bunch. This refactors our code to create a single one and pass that one where needed.

emerge-tools · 2025-02-26T09:31:56Z

📸 Snapshot Test

Some snapshots failed to render

Name	Added	Removed	Modified	Renamed	Unchanged	Errored	Approval
TestPurchasesUIAndroidCompatibility com.revenuecat.testpurchasesuiandroidcompatibility	0	31	0	0	206	28	⏳ Needs approval

🛸 Powered by Emerge Tools

codecov · 2025-02-26T12:27:11Z

Codecov Report

Attention: Patch coverage is 15.78947% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.47%. Comparing base (a31c2a2) to head (7f84c79).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../com/revenuecat/purchases/PurchasesOrchestrator.kt	17.64%	14 Missing ⚠️
.../revenuecat/purchases/utils/CoilImageDownloader.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2146      +/-   ##
==========================================
- Coverage   80.54%   80.47%   -0.07%     
==========================================
  Files         277      277              
  Lines        9456     9465       +9     
  Branches     1334     1335       +1     
==========================================
+ Hits         7616     7617       +1     
- Misses       1280     1288       +8     
  Partials      560      560

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ajpallares

Looks good, but I'm not ready to approve these yet 🙏
A couple of comments though

ajpallares · 2025-02-26T13:24:15Z


 @Suppress("unused", "UNUSED_VARIABLE", "EmptyFunctionBlock", "DEPRECATION")
 private class PurchasesAPI {
+    @OptIn(InternalRevenueCatAPI::class)


Is this the equivalent to what we have in iOS with _spi(Internal). Just curious :)

Yup! You need to explicitly annotate usages of these APIs like this.

JayShortway

This is great! The garbage collector will thank us 😄 Just a suggestion on making this a static field instead.

JayShortway · 2025-02-26T14:08:49Z

            customerInfoUpdateHandler,
        ),
+    val imageLoader: ImageLoader? = null,
+    val processLifecycleOwnerProvider: () -> LifecycleOwner = { ProcessLifecycleOwner.get() },


Just curious, why was this change needed?

Ahh good question 😅 So, it seems that adding the testImplementation with the code library, changed our dependency with androidx.lifecycle. Before this PR, it had an implementation in Java, where we could mockk the ProcessLifecycleOwner using mockkStatic. After the new version, it was in kotlin, and our previous implementation didn't work. I thought of modifying the mocks to work, but then I thought we should try to inject it instead of mocking it like we were until now.

As I was thinking about injecting it, in order to avoid any behavior change, I preferred not to get it at the time the PurchasesOrchestrator is built (normally during Application's onCreate), instead, this is basically lazy loading it when it was being called before. I think it might be fine to get it at the time of the PurchasesOrchestrator creation, but not sure. Wdyt? Happy to change it to just store the object instead of lazy loading it.

Oh right, so we're using a different androidx.lifecycle version in tests vs production, because of the new Coil testing library we added? I think providing it like this is fine!

JayShortway · 2025-02-26T14:23:15Z

+            var newImageLoader = if (Purchases.isConfigured) {
+                Purchases.sharedInstance.getImageLoader()
+            } else {
+                null
+            }


Instead of creating it at configure() time, it would be nicer to have a static field in Purchases that gets set the first time it is accessed, e.g. Purchases.imageLoader. We could take inspiration from newImageLoader() in Coil 2.7.0.

This would avoid this entire scenario, because we'd always be able to get an image loader. That also means we don't need to duplicate the creation logic in both SDKs. What do you think?

Hmm I'm wondering... I think the accessor in Purchases should still be nullable in case the Coil dependency isn't added... Though TBH, you would need to add it in order to access the class of that property at all... So maybe I will just make it not nullable and say it will crash if Coil isn't present. It's meant for internal usage anyway.

Another thing as I was starting this, we do need the Application context to create the ImageLoader, which is only given to us at configure time. I guess we could lazy load it, but considering we need to prefetch images anyway, I feel it's better the way it is. Wdyt?

Hmm, yea my main concern is the duplication of the image loader configuration. I'd love to get rid of that with this effort. I see 2 options:

We just use Purchases.sharedInstance.imageLoader everywhere, and get rid of the Context.getRevenueCatUIImageLoader() function. We won't be showing any UI (and thus images) if we're not configured anyway.

Probably cleaner: we create a static Purchases.getImageLoader(Context) function, which only creates a new one the first time it is called, and otherwise just returns the same instance every time. (Good point on needing a context!)

What do you think about that second option in particular?

Hmm yeah I think that can work! I will do the second option then 👍

Ok done in 2e00191. Lmk what you think!

tonidero · 2025-02-27T11:57:34Z


    dokkaPlugin(project(":dokka-hide-internal"))

+    testImplementation libs.coil.compose


This is still needed because in a couple tests, due to some mockk internals, it needs to reference the actual coil.ImageLoader type, making us having to do this... We could rewrite those tests, but I think the new way in this PR probably makes more sense. Lmk what you think!

It's not the end of the world imo!

JayShortway

Beautiful! 💪 Just the thread safe comment.

JayShortway · 2025-02-27T12:18:01Z

            customerInfoUpdateHandler,
        ),
+    val imageLoader: ImageLoader? = null,
+    val processLifecycleOwnerProvider: () -> LifecycleOwner = { ProcessLifecycleOwner.get() },


Oh right, so we're using a different androidx.lifecycle version in tests vs production, because of the new Coil testing library we added? I think providing it like this is fine!

JayShortway · 2025-02-27T12:47:58Z

+        fun getImageLoader(context: Context): ImageLoader {
+            val currentImageLoader = cachedImageLoader
+            return if (currentImageLoader == null) {
+                val maxCacheSizeBytes = 25 * 1024 * 1024L // 25 MB
+                val cacheFolder = "revenuecatui_cache"
+                val imageLoader = ImageLoader.Builder(context)
+                    .diskCache {
+                        DiskCache.Builder()
+                            .directory(context.cacheDir.resolve(cacheFolder))
+                            .maxSizeBytes(maxCacheSizeBytes)
+                            .build()
+                    }
+                    .build()
+                cachedImageLoader = imageLoader
+                imageLoader
+            } else {
+                currentImageLoader
+            }
+        }


What do you think about making this thread safe, along the lines of this?

Whoops good point, thanks!!

@ajpallares

**This is an automatic release.** ## RevenueCat SDK ### ✨ New Features * Add `hasPaywall` property to `Offering` (#2212) via Antonio Pallares (@ajpallares) ### 🐞 Bugfixes * Fix empty options in NoActive subscriptions screen (#2168) via Cesar de la Vega (@vegaro) ## RevenueCatUI SDK ### Customer Center #### ✨ New Features * Create `CustomerCenterListener` (#2199) via Cesar de la Vega (@vegaro) #### 🐞 Bugfixes * Reload Customer Center after a successful restore (#2203) via Cesar de la Vega (@vegaro) * Fixes CustomerCenter state not refreshing when reopening (#2202) via Cesar de la Vega (@vegaro) ### 🔄 Other Changes * Improves PaywallsTester multi-API-key support (#2218) via JayShortway (@JayShortway) * [EXTERNAL] Bump Emerge Gradle Plugin and Snaphsots version (#2211) via @runningcode (#2217) via JayShortway (@JayShortway) * [AUTOMATIC][Paywalls V2] Updates Compose previews of all templates (#2207) via RevenueCat Git Bot (@RCGitBot) * [Paywalls V2] Enables template previews again (#2215) via JayShortway (@JayShortway) * Adds support for switching between 2 API keys to PaywallsTester (#2213) via JayShortway (@JayShortway) * Adds a `LocalPreviewImageLoader` `CompositionLocal`. (#2201) via JayShortway (@JayShortway) * Logs from RevenueCatUI are now tagged with `[Purchases]` too. (#2206) via JayShortway (@JayShortway) * [Paywalls V2] Ignores template previews for now. (#2209) via JayShortway (@JayShortway) * [Paywalls V2] Some more template previews optimizations (#2208) via JayShortway (@JayShortway) * chore: Delete key from customer center survey event (#2204) via Facundo Menzella (@facumenzella) * [Paywalls V2] Extract ImageLoader to use a single one (#2146) via Toni Rico (@tonidero) * [Paywalls V2] Adds progress indicator to buttons (#2198) via JayShortway (@JayShortway) * Avoids triggering "unscheduled" workflows when triggering workflows via the CircleCI API (#2200) via JayShortway (@JayShortway) * [Paywalls V2] Adds a note on publishing to the missing paywall error. (#2193) via JayShortway (@JayShortway) * Adds X-Kotlin-Version header. (#2197) via JayShortway (@JayShortway) * [Paywalls V2] Adds docs on ignored arguments for Paywalls V2 in more places. (#2195) via JayShortway (@JayShortway) * chore: Add backend integration test for events (#2189) via Facundo Menzella (@facumenzella) * [Paywalls V2] Adds CI job to update template previews (#2192) via JayShortway (@JayShortway) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Cesar de la Vega <cesarvegaro@gmail.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>

tonidero added pr:RevenueCatUI pr:other feat:PaywallV2 labels Feb 12, 2025

tonidero added 2 commits February 26, 2025 09:06

[Paywalls V2] Extract ImageLoader to use a single one

ccfe424

Fix some tests and cleanup

ab37428

tonidero force-pushed the refactor-to-single-image-loader branch from 0953e78 to ab37428 Compare February 26, 2025 09:21

tonidero and others added 4 commits February 26, 2025 11:26

Fix tests

239029b

Change RevenueCatUI to use same ImageLoader as RevenueCat

edb783f

Add api tests

c15f6e2

Merge branch 'main' into refactor-to-single-image-loader

187dc59

tonidero marked this pull request as ready for review February 26, 2025 10:33

tonidero requested a review from a team February 26, 2025 10:33

tonidero and others added 2 commits February 26, 2025 12:57

Fix case of purchases not being configured

d3bfe92

Merge branch 'main' into refactor-to-single-image-loader

a270e64

ajpallares reviewed Feb 26, 2025

View reviewed changes

Add back comment

9ab179e

JayShortway reviewed Feb 26, 2025

View reviewed changes

tonidero and others added 2 commits February 27, 2025 10:24

Convert to property instead of method

4e5038c

Merge branch 'main' into refactor-to-single-image-loader

5912182

tonidero requested a review from JayShortway February 27, 2025 09:25

tonidero added 5 commits February 27, 2025 10:43

Fix

9ecc6a3

Refactor to be a singleton function

2e00191

Fix tests

dbcd589

Small cleanup

79cdfb6

Revert unneeded changes after latest modifications

5638909

tonidero commented Feb 27, 2025

View reviewed changes

JayShortway approved these changes Feb 27, 2025

View reviewed changes

Handle concurrency

3236127

JayShortway approved these changes Feb 27, 2025

View reviewed changes

Merge branch 'main' into refactor-to-single-image-loader

7f84c79

tonidero enabled auto-merge (squash) February 27, 2025 13:18

tonidero merged commit 3812895 into main Feb 27, 2025

tonidero deleted the refactor-to-single-image-loader branch February 27, 2025 13:44

RCGitBot mentioned this pull request Mar 5, 2025

[AUTOMATIC] Release/8.14.0 #2223

Merged


		dokkaPlugin(project(":dokka-hide-internal"))

		testImplementation libs.coil.compose

Uh oh!

Conversation

tonidero commented Feb 12, 2025

Description

Uh oh!

emerge-tools Bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📸 Snapshot Test

Some snapshots failed to render

Uh oh!

codecov Bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ajpallares left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JayShortway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayShortway Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayShortway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emerge-tools Bot commented Feb 26, 2025 •

edited

Loading

codecov Bot commented Feb 26, 2025 •

edited

Loading

JayShortway Feb 27, 2025 •

edited

Loading