Use new resource format for cross validation screenshots and fix lint #2413
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the paywall screenshot resource handling to support a new resource format and addresses lint issues.
- Replace static resource path with a dynamic directory search for paywall assets.
- Refactor JSON parsing to process multiple offerings from individual directories.
Comments suppressed due to low confidence (1)
ui/revenuecatui/src/debug/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/TemplatePreviews.kt:183
- [nitpick] Matching the image file solely by its name might lead to ambiguities if duplicate names exist in different subdirectories. Consider incorporating additional path context to uniquely identify the file.
val matchingFile = getResourcesBaseDir().walkTopDown().firstOrNull { it.isFile && it.name == relativePath.substringAfterLast("/") }
...tui/src/debug/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/TemplatePreviews.kt
Outdated
Show resolved
Hide resolved
📸 Snapshot Test186 added, 29 renamed, 418 unchanged
🛸 Powered by Emerge Tools |
| // Look for a matching file in all subdirectories | ||
| val matchingFile = getResourcesBaseDir().walkTopDown() | ||
| .firstOrNull { it.isFile && it.name == relativePath.substringAfterLast("/") } |
There was a problem hiding this comment.
This seems very unoptimized. The previous approach was based on the fact that we could be certain of a file's location given its URL. Just walking the file tree every time is a bit wasteful. The Emerge Snapshots environment is also very sensitive to performance problems, so that's why it caught my eye.
There was a problem hiding this comment.
Actually it seems that Emerge does trip over the changes in this PR. It only registers removed screenshots, but that is unexpected. There are also some errors. My hunch is that we need to optimize for memory, as I had to do in the previous iteration of this.
| private fun getResourcesBaseDir(): File { | ||
| return File(BuildConfig.PROJECT_DIR) | ||
| .resolve("../../upstream/paywall-preview-resources/resources") | ||
| } |
There was a problem hiding this comment.
This only works in Android Studio. When the screenshot is rendered on the actual Android OS (device or emulator, as Emerge does), it won't be able to find the resources. We need to check the class loader instead. Check the implementation of getResourceStream() for an example.
|
TODO:
|
1 build increased size
TestPurchasesUIAndroidCompatibility 1.0 (1)
|
| Item | Install Size Change | Download Size Change |
|---|---|---|
| 📝 957060_1741639342.heic | ⬆️ 1.2 MB | ⬆️ 1.2 MB |
| 📝 387178_1739250499.heic | ⬆️ 1.2 MB | ⬆️ 1.2 MB |
| 📝 453869_1740097911.heic | ⬆️ 930.3 kB | ⬆️ 930.6 kB |
| 📝 1201192_1741375926.heic | ⬆️ 818.8 kB | ⬆️ 819.0 kB |
| 📝 1188871_1745890350.heic | ⬆️ 708.4 kB | ⬆️ 708.4 kB |
View Treemap
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
| internal fun Result<FontSpec, PaywallValidationError>.recoverFromFontAliasError(): Result<FontSpec?, PaywallValidationError> = | ||
| flatMapError { error -> | ||
| if (error is PaywallValidationError.MissingFontAlias && error.alias.value.isBlank()) { | ||
| // Treating this as a dashboard error and just ignoring it. No need to log. | ||
| Result.Success(null) | ||
| } else if (error is PaywallValidationError.MissingFontAlias) { | ||
| Logger.e( | ||
| "Font named '${error.alias}' was not found in the font config. Try re-adding it in the Paywall editor.", | ||
| ) | ||
| Result.Success(null) |
There was a problem hiding this comment.
@tonidero FYI, I don't think this should conflict with your stuff right?
There was a problem hiding this comment.
Oh it's a different part of the code so I don't think so either. Thanks for checking!
There was a problem hiding this comment.
I can't review my own PR even though this was all @JayShortway but... looks good to me!
| val offering = paywall.offering | ||
| val parentFolder = paywall.parentFolder | ||
| // validatePaywallComponentsDataOrNullForPreviews should only return null if the Offering has no paywallComponents, | ||
| // but we filter those out in the PaywallResourcesProvider. |
There was a problem hiding this comment.
I think this is fine for previews 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2413 +/- ##
=======================================
Coverage 79.66% 79.66%
=======================================
Files 284 284
Lines 10263 10263
Branches 1457 1457
=======================================
Hits 8176 8176
Misses 1460 1460
Partials 627 627 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
**This is an automatic release.** ## RevenueCatUI SDK ### Paywallv2 #### 🐞 Bugfixes * [Paywalls] Fix Bold text in Markdown on higher Weights Text composables (#2421) via Toni Rico (@tonidero) ### 🔄 Other Changes * Add coroutines dependency to core SDK (#2423) via Toni Rico (@tonidero) * [Paywalls] Add `font_weight_int` to `TextComponent` (#2419) via Toni Rico (@tonidero) * [Paywalls V2] Downloadable fonts (#2414) via Toni Rico (@tonidero) * [AUTOMATIC][Paywalls V2] Updates paywall-preview-resources submodule (#2412) via RevenueCat Git Bot (@RCGitBot) * Use new resource format for cross validation screenshots and fix lint (#2413) via Josh Holtz (@joshdholtz) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
Motivation
Use new paywall resources format to have multiple sets of paywalls to screenshot
Description
Looking more dynamically for screenshots