Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[splash-screen] Fix reading splash image from the android.splash config #10494

Merged
merged 1 commit into from Oct 1, 2020
Merged

[splash-screen] Fix reading splash image from the android.splash config #10494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion android/expoview/src/main/java/host/exp/exponent/experience/ExperienceActivity.java
View file
Open in desktop
Expand Up @@ -400,8 +400,8 @@ protected void showOrReconfigureManagedAppSplashScreen(final JSONObject manifest
}

this.hideLoadingView();
ManagedAppSplashScreenConfiguration config = ManagedAppSplashScreenConfiguration.parseManifest(manifest);
if (mManagedAppSplashScreenViewProvider == null) {
ManagedAppSplashScreenConfiguration config = ManagedAppSplashScreenConfiguration.parseManifest(manifest);
Copy link
Member

@brentvatne brentvatne Oct 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the motivation for moving this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config variable is only used in the if block, os it was just natural for me to scope it into if block as well 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok. i just wasn't sure if it was somehow intended to be related to the fix

mManagedAppSplashScreenViewProvider = new ManagedAppSplashScreenViewProvider(config);
SplashScreen.show(this, mManagedAppSplashScreenViewProvider, getRootViewClass(manifest), true);
} else {
Expand Down
37 changes: 30 additions & 7 deletions ...ain/java/host/exp/exponent/experience/splashscreen/ManagedAppSplashScreenConfiguration.kt
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import org.json.JSONObject
class ManagedAppSplashScreenConfiguration private constructor() {
var resizeMode: SplashScreenImageResizeMode = SplashScreenImageResizeMode.CONTAIN
private set

@ColorInt
var backgroundColor = Color.parseColor("#ffffff")
private set
Expand All @@ -36,7 +37,7 @@ class ManagedAppSplashScreenConfiguration private constructor() {
}

private fun parseResizeMode(manifest: JSONObject): SplashScreenImageResizeMode? {
val resizeMode = getStringFromManifest(
val resizeMode = getStringFromJSONObject(
manifest,
arrayOf(
ExponentManifest.MANIFEST_ANDROID_INFO_KEY,
Expand All @@ -52,7 +53,7 @@ class ManagedAppSplashScreenConfiguration private constructor() {
}

private fun parseBackgroundColor(manifest: JSONObject): Int? {
val backgroundColor = getStringFromManifest(
val backgroundColor = getStringFromJSONObject(
manifest,
arrayOf(
ExponentManifest.MANIFEST_ANDROID_INFO_KEY,
Expand All @@ -69,8 +70,30 @@ class ManagedAppSplashScreenConfiguration private constructor() {
} else null
}

/**
* Tries to retrieve imageUrl from the manifest checking for value for keys/paths in following order
* - android-scoped splash dpi images (starting from 'xxx-hdpi" and ending with 'mdpi')
* - android-scoped splash imageUrl
* - generic splash imageUrl
*/
private fun parseImageUrl(manifest: JSONObject): String? {
return getStringFromManifest(
val androidSplash = manifest
.optJSONObject(ExponentManifest.MANIFEST_ANDROID_INFO_KEY)
?.optJSONObject(ExponentManifest.MANIFEST_SPLASH_INFO_KEY)
if (androidSplash != null) {
val dpiRelatedImageUrl = getStringFromJSONObject(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So… would this return the first non-empty value for any of the keys defined in *arrayOf("xxxhdpi"…)? Wouldn't we want to use a lower-resolution image on a lower-resolution device?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been always doing this that way (code from the sdk-38 branch):

expo/android/expoview/src/main/java/host/exp/exponent/LoadingView.java

Lines 271 to 295 in 9db4ea6

private String backgroundImageURL(final JSONObject manifest) {
if (isUsingNewSplashScreenStyle(manifest)) {
JSONObject splash;
if (manifest.has("android")) {
final JSONObject android = manifest.optJSONObject("android");
if (android.has(ExponentManifest.MANIFEST_SPLASH_INFO_KEY)) {
splash = android.optJSONObject(ExponentManifest.MANIFEST_SPLASH_INFO_KEY);
// Use the largest available image in the `android.splash` object, or `splash.imageUrl` if none is available .
final String[] keys = {"xxxhdpiUrl", "xxhdpiUrl", "xhdpiUrl", "hdpiUrl", "mdpiUrl", "ldpiUrl"};
for (String key : keys) {
if (splash.has(key) && splash.optString(key) != null) {
return splash.optString(key);
}
}
}
}
if (manifest.has(ExponentManifest.MANIFEST_SPLASH_INFO_KEY)) {
splash = manifest.optJSONObject(ExponentManifest.MANIFEST_SPLASH_INFO_KEY);
if (splash.has(ExponentManifest.MANIFEST_SPLASH_IMAGE_URL)) {
return splash.optString(ExponentManifest.MANIFEST_SPLASH_IMAGE_URL);
}
}

The image is fetched into ImageView, so we rather prefer to always have the best resolution here.
These *dpi are only usable in standalone's builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so this won't be used in standalone apps, sorry. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's the way we've always done it then let's stick with that. we can follow up with improvements over status quo if needed in another pr

androidSplash,
*arrayOf("xxxhdpi", "xxhdpi", "xhdpi", "hdpi", "mdpi"
)
.map { s -> "${s}Url" }
.map { s -> arrayOf(s) }
.toTypedArray())
if (dpiRelatedImageUrl != null) {
return dpiRelatedImageUrl
}
}

return getStringFromJSONObject(
manifest,
arrayOf(
ExponentManifest.MANIFEST_ANDROID_INFO_KEY,
Expand All @@ -84,18 +107,18 @@ class ManagedAppSplashScreenConfiguration private constructor() {
)
}

private fun getStringFromManifest(manifest: JSONObject, vararg paths: Array<String>): String? {
private fun getStringFromJSONObject(jsonObject: JSONObject, vararg paths: Array<String>): String? {
for (path in paths) {
val pathResult = getStringFromManifest(manifest, path)
val pathResult = getStringFromJSONObject(jsonObject, path)
if (pathResult != null) {
return pathResult
}
}
return null
}

private fun getStringFromManifest(manifest: JSONObject, path: Array<String>): String? {
var json: JSONObject? = manifest
private fun getStringFromJSONObject(jsonObject: JSONObject, path: Array<String>): String? {
var json: JSONObject? = jsonObject
for (i in path.indices) {
val isLastKey = i == path.size - 1
val key = path[i]
Expand Down