Fix multiple paywalls display issues #1854
Conversation
| @@ -175,6 +175,7 @@ internal fun getPaywallViewModel( | |||
| ): PaywallViewModel { | |||
| val applicationContext = LocalContext.current.applicationContext | |||
| val viewModel = viewModel<PaywallViewModelImpl>( | |||
| key = options.hashCode().toString(), | |||
There was a problem hiding this comment.
This is the main change. With this we can have multiple view models living at the same time.
But more importantly, this is not new but the view model isn't currently cleared when using composables or views and dismissing the paywall, since the view model owner (the activity) isn't cleared. With the increase in the possible number of view models, this means we can potentially increase our memory usage if the paywall is displayed with multiple different options. This should normally not be a problem, but something to consider...
To solve this, it might be tricky to scope the view model to a screen though, since we don't have access to the navigation graph... I've been thinking of ways to clear the view models when dismissing the paywall but I don't think there is a straightforward way to do it, since the ViewModelStoreOwner is designed to handle this themselves... Any ideas?
There was a problem hiding this comment.
When using Jetpack Navigation (or another navigation library with support for ViewModelStoreOwner), wouldn't the LocalViewModelStoreOwner provide the navigation graph as the closest ViewModelStoreOwner? That means it's automatically scoped to a screen. (I'm deriving that from these docs.)
There was a problem hiding this comment.
Yes, if using jetpack navigation. So we should be mostly ok in that case. However, it's not guaranteed for devs to be using that library... But yeah, it further constraints this limitation 👍
There was a problem hiding this comment.
Yea you're right. It's a good limitation to be aware of!
| override fun hashCode(): Int { | ||
| var result = offeringSelection.offeringIdentifier.hashCode() | ||
| result = 31 * result + shouldDisplayDismissButton.hashCode() | ||
| result = 31 * result + mode.hashCode() |
There was a problem hiding this comment.
Right now only using the offeringId+shouldDisplayDismissButton+mode as identifier of the paywall. The others are objects with callbacks which would be much trickier to compare.
tonidero
force-pushed
the
fix-multiple-paywalls-display
branch
from
7ee1fb2 to
1e5e544
Compare
| @@ -43,6 +43,13 @@ data class PaywallOptions internal constructor( | |||
| dismissRequest = builder.dismissRequest, | |||
| ) | |||
|
|
|||
| internal val dataHash: String = run { | |||
There was a problem hiding this comment.
Note that I'm not overriding the hashCode/equals functions here since we are comparing them (including the listeners) for other methods, like the updateOptions method. This was the least disruptive approach that didn't mean recreating the view model for every recreation of the listeners.
There was a problem hiding this comment.
that makes sense, but let's add a comment so future our future selves understand why we have dataHash in the first place
tonidero
force-pushed
the
fix-multiple-paywalls-display
branch
from
1e5e544 to
1bbdd1d
Compare
tonidero
force-pushed
the
fix-multiple-paywalls-display
branch
from
1bbdd1d to
7a16d8d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1854 +/- ##
=======================================
Coverage 82.80% 82.80%
=======================================
Files 222 222
Lines 7689 7689
Branches 1084 1084
=======================================
Hits 6367 6367
Misses 899 899
Partials 423 423
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -43,6 +43,13 @@ data class PaywallOptions internal constructor( | |||
| dismissRequest = builder.dismissRequest, | |||
| ) | |||
|
|
|||
| internal val dataHash: String = run { | |||
There was a problem hiding this comment.
that makes sense, but let's add a comment so future our future selves understand why we have dataHash in the first place
**This is an automatic release.** ## RevenueCat SDK ### ✨ New Features * Add `kochava` integration (#1844) via Toni Rico (@tonidero) ## RevenueCatUI SDK ### 🐞 Bugfixes * Fix multiple paywalls display issues (#1854) via Toni Rico (@tonidero) * Fix interaction not disabled during purchases (#1850) via Toni Rico (@tonidero) * Fix crash if activity finished while calculating presentation logic (#1846) via Toni Rico (@tonidero) ### 🔄 Other Changes * Adds some more test cases validating {{ total_price_and_per_month }} for quarterly packages. (#1853) via JayShortway (@JayShortway) * Converts CustomEntitlementComputationSample's Gradle files to Kotlin (#1852) via JayShortway (@JayShortway) * Converts MagicWeatherCompose's Gradle files to Kotlin (#1851) via JayShortway (@JayShortway) * [EXTERNAL] Wireup Emerge gradle plugin config for PR snapshot diffs (#1841) by @rbro112 (#1843) via Toni Rico (@tonidero) * Bump fastlane-plugin-revenuecat_internal from `5140dbc` to `55a0455` (#1845) via Cesar de la Vega (@vegaro) Co-authored-by: revenuecat-ops <[email protected]>
Description
This is an approach to fix DENG-955
If trying to display multiple paywalls at the same time, all paywalls would currently share the same view model. This caused that all of them would be exactly the same, which was not great for some use cases. This adds a key to the view model injection using the paywall options view model hash code. This means we will have one view model for each combination of paywall options parameters.