Paywalls: Fix wrong view model data after configuration changes

[tonidero]

Description

I noticed that after a configuration change (for example moving from landscape to portrait), the close button didn't work as expected on the dialogs. I tracked it down to the fact that the view model kept the first version of the dismiss callback, so it wasn't calling the new one created after the configuration change.

I decided to remove the current approach of using keys to create individual view models and instead I'm adding a method to the view model that gets called every time we try to get the view model. If we detect that the PaywallOptions have changed, we will update it on the view model, ensuring we have the latest version of the callbacks.

[NachoSoto]

Thanks for fixing this! I have a suggestion to simplify this.

[NachoSoto]

If we made PaywallOptions a data class would we get this for free?

[NachoSoto]

Though my other suggestion might remove the need for this too.

[tonidero]

If we made PaywallOptions a data class would we get this for free?

I was trying to avoid it, since it's not recommended to expose data classes... But honestly, it's reached a point where things will be much easier, so I think I will do the change yeah.

[tonidero]

Made the change to data classes e262e9a

[NachoSoto]

👍🏻
Why is it not recommended?

[tonidero]

Mentioned it over slack but this article has some of the drawbacks: https://jakewharton.com/public-api-challenges-in-kotlin/

[NachoSoto]

updateState only uses options.offeringSelection, so it seems silly to fetch everything if we only change something else like shouldDisplayDismissButton.

Suggestion: compare only options.offeringSelection here, but the risk is that if updateState uses something else in the future, it would break.
So to deal with that, we can extract a createState method like suspend createState(offeringSelection: OfferingSelection): PaywallState.

Then make updateState simply do:

private fun updateState() {
    viewModelScope.launch {
         _state.value = createState(options.offeringSelection)
    }
}

And we can do the exact same here, therefore ensuring that if createState ever needs more data from options, that's explicit here.

What do you think?

[tonidero]

Actually, we also use the mode (it's masked behind a computed property in PaywallViewModelImpl), and options.shouldDisplayDismissButton in calculateState. So we would need those 3.

The other fields are the dismissRequest callback, the listener and the fontProvider... So I kinda think it's worth to just compare everything and if anything changes, just recompute the state... Wdyt?

[NachoSoto]

Oh that's right. This is fine then 👍

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d1329ed) 84.25% compared to head (d235f94) 84.25%.
Report is 1 commits behind head on paywalls.

❗ Current head d235f94 differs from pull request most recent head e262e9a. Consider uploading reports for the commit e262e9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           paywalls    #1417   +/-   ##
=========================================
  Coverage     84.25%   84.25%           
=========================================
  Files           197      197           
  Lines          6619     6619           
  Branches        960      960           
=========================================
  Hits           5577     5577           
  Misses          674      674           
  Partials        368      368           

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

[NachoSoto]

Much better!