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.

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

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

Merged

Conversation

bbarthec
Copy link
Contributor

@bbarthec bbarthec commented Oct 1, 2020

Why

Partially resolves #10382 - only ExpoClient flow.
This PR resolves reading android.splash.*dpi images as splash image.

How

After integrating expo-splash-screen *dpi keys form the config were no longer read by the Android.
It restores previous behaviour.
Splash image is now searched for under following keys (and the first hit is used):

  • android.splash.*dpi (from xxxhdpi down to mdpi)
  • android.splash.image (this one is not working yet, needs schema update to transform image into imageUrl, to be delivered soon)
  • splash.image

Test Plan

  • create an app with the config:
{
  "splash": {} // intentionally empty,
  "android": {
    "splash": {
      "xxxhdpi": "<relativePathToTheSplashImage>"
    }
  }
}
  • start the app and see the splash image is presented

.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):

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

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

@brentvatne brentvatne Oct 1, 2020

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

@brentvatne brentvatne merged commit 9c26fbc into master Oct 1, 2020
@brentvatne brentvatne deleted the @bbarthec/splash-screen/fix-scoped-splash-image-config branch October 1, 2020 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

android.splash key not being used in standalone app builds on SDK 39
3 participants