Fix #2652: Add dimensions for splash screen logo #2894

ArpitShukIa · 2021-03-12T12:40:44Z

Explanation

Fixes #2652 : Added dimensions for splash screen logo for different screen sizes.

Screenshots:
Device: Realme 3 Pro (Landscape)

Other device models:

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

ArpitShukIa · 2021-03-12T12:48:03Z

@rt4914 PTAL.

FareesHussain · 2021-03-13T08:41:30Z

@ArpitShukIa were you able to produce this issue in any device

ArpitShukIa · 2021-03-13T08:51:31Z

Yeah, I was able to produce it in my Realme 3 Pro.

Potrait:

Landscape:

FareesHussain · 2021-03-13T09:00:48Z

Thanks for confirming.

rt4914

@ArpitShukIa Thanks. suggested few things.

app/src/main/res/drawable/splash_screen.xml

rt4914 · 2021-03-15T17:28:23Z

app/src/main/res/drawable/splash_screen.xml

+  <item android:drawable="@color/oppiaPrimaryLight" />
+  <item
+    android:width="@dimen/splash_screen_logo_width"
+    android:height="@dimen/splash_screen_logo_height"


The width and height works only for API 23+. Have you checked on API 19 or 20?

I checked and it's not working for API below 23. The logo gets stretched in lower apis.
I tried searching but couldn't find any good solution.
One solution can be to use a png logo for those lower APIs instead of a vector drawable.

Can we use ConstraintLayout Guidelines as mentioned in the issues-mocks?

I am not sure if this is possible. We need to set a drawable for android:windowBackground. I don't think we can set a layout as windowBackground.

If any other approach works, that's also fine. Can you investigate and come with possible solutions for this?

app/src/main/res/drawable/splash_page.xml

app/src/main/res/drawable/splash_screen.xml

ArpitShukIa · 2021-03-19T18:21:37Z

@rt4914 For API 23 and above, display is the same as in screenshots send above.

For API below 23: (I used API 19 here)

Pixel 2 5" (Portrait)

Pixel 2 5" (Land)

Pixel 4 XL 6.3" (Port)

Pixel 4 XL 6.3" (Land)

Nexus 9 Tablet 8.86" (Port)

Nexus 9 Tablet 8.86" (Land)

rt4914

The solution does look good but still adding @BenHenning as it might lead to added apk size.

WDYT @BenHenning about this implementation?

ArpitShukIa · 2021-03-24T04:47:32Z

The combined size of the 4 PNGs that I added is 65.39 kB.

BenHenning · 2021-03-25T03:11:47Z

Thanks @ArpitShukIa. One thing that's unclear to me: why do we need to load PNGs here? The Oppia logo drawable is a vector drawable and should therefore be scalable to different densities. Can you provide a bit more detail about why the vector drawable isn't sufficient?

ArpitShukIa · 2021-03-25T04:47:43Z

@BenHenning PTAL at this stackoverflow answer.

ArpitShukIa · 2021-03-28T03:01:06Z

Thanks @ArpitShukIa. One thing that's unclear to me: why do we need to load PNGs here? The Oppia logo drawable is a vector drawable and should therefore be scalable to different densities. Can you provide a bit more detail about why the vector drawable isn't sufficient?

@BenHenning The problem with just using vector drawable is that the drawable contains the logo as well as the green background in the height to width ratio of 640dp to 360dp. What we desire is that the green background should stretch to cover the whole screen but the white logo should maintain its aspect ratio. This is not possible, since both are part of a single vector drawable.
And so, to cover the whole screen the drawable goes into a kind of fitXY state and the white logo gets stretched along with the green background.

A solution to this is to use a layered-list with the background as one layer and the white logo as another. Here we need to provide height and width to the logo which vary with different screen sizes, that's why I put these values in different dimensions.xml.

The only problem with the above solution is that the height and width attributes for an item in layered-list are not available for APIs below 23. So, for lower APIs, we need to provide PNG versions of our logo and put them as a bitmap in the layered-list.

BenHenning · 2021-03-31T05:36:30Z

Thanks @ArpitShukIa. That's really interesting...I'll need to think on this a bit more before reviewing it. It seems like this might be our only option, but I want to be sure before we introduce binary files to the codebase.

ArpitShukIa · 2021-04-18T11:42:28Z

@BenHenning Did you review this? Does it look good?

BenHenning

Thanks @ArpitShukIa. Apologies for pushing this back, and thanks for your patience. I left two comments, but I'm also wondering: is it possible for us to add splash tests for different configurations in Robolectric to verify that a bitmap vs. a picture drawable is used? I think you might even be able to verify the bitmap's dimensions for different configurations, so we can actually have tests for each of the new images and the drawable. Roboletric also lets you configure SDK per-test so you can validate the v23 qualifier.

Other than that, I still want to check this out locally and inspect the new bitmap images before signing off. It seems like this is the best solution available (quite possibly the only), and I think we can afford adding a few more small binary files into the repository since we are unlikely to change them.

BenHenning · 2021-04-22T22:06:38Z

app/src/main/res/drawable-v23/splash_page.xml

+  <item
+    android:width="@dimen/splash_screen_logo_width"
+    android:height="@dimen/splash_screen_logo_height"
+    android:drawable="@drawable/oppia_logo"


Could we maybe rename this to be 'oppia_logo_svg' and the png version 'oppia_logo_bitmap' to contrast?

BenHenning · 2021-04-22T22:06:58Z

app/src/main/res/drawable/splash_page.xml

+  <item>
+    <bitmap
+      android:gravity="center"
+      android:src="@drawable/oppia_png_logo" />


Could you upload webp versions of the images instead of png? webp are often smaller.

BenHenning · 2021-04-22T22:10:43Z

app/src/main/res/values-land/dimens.xml

+  <!--  Splash Screen-->
+  <dimen name="splash_screen_logo_width">360dp</dimen>
+  <dimen name="splash_screen_logo_height">640dp</dimen>
+  <dimen name="splash_screen_logo_margin_top">100dp</dimen>


For this & others: how did you come up with these margins? Also, will these work correctly for both the bitmap & vector drawable (given that they're both used in landscape/other scenarios depending on SDK version).

For this & others: how did you come up with these margins?

I figured out these margins just by hit and trial. I used multiple devices (phones & tablets) to find the best fits.
These height and width are in the same ratio as that of the svg logo.

Also, will these work correctly for both the bitmap & vector drawable

Currently, these margins are just for the svg drawable. For the bitmap, I haven't set any margin yet. As a result, it is centred vertically as of now. To get the behaviour as shown in the mock we will have to provide a bottom margin to the bitmap. (I think different bottom margins will be required for mobile and tablet layouts.)

I wanted to confirm that is it necessary for the logo (in portrait mode) to sit above the central horizontal axis as shown in the mocks. Like, how bad do these vertically centred logos look?
If we place the logo at the centre (as shown above), we can get rid of those top and bottom margins.

Another point is that I think we should update the path data for svg logo to only contain the logo and no extra space. Right now it has a viewportWidth=360dp and viewportHeight=640dp but the logo is only in a small portion of this area. This blank area was earlier required when that green background was a part of this svg but we don't need this now. Also, removing the blank area will allow us to use the same dimensions (and margins) for both the svg and the bitmaps.
Unfortunately, I won't be able to do this part as I don't have any prior experience in editing svgs.

Mocks for reference:

Mobile (Portrait, Landscape)

Tablet (Portrait, Landscape)

(Plz ignore the percentage values in the mocks, I don't think it's possible to set percentage margins in a drawable. Also, please let me know if there's actually some way to achieve this.)

Thanks for the really detailed response @ArpitShukIa!

@mschanteltc do you have any thoughts on how we should relatively position the logo? Should it be center-center? Also, might it be possible to provide us with a new version of the Oppia logo SVG that doesn't include any padding around it? I think we need to reimport this, and convert that exact SVG to WEBP so that we can make sure the bitmap/vector images are exactly the same so that positioning is consistent for both.

For (mobile, tablet) landscape views, we would vertically center the logo. For (mobile, tablet) portrait views, we should place the icon on top of the vertical center.

The svg in the Android Images folder should already be fit to bounds, so there shouldn't be any padding around it. Is there a specific way I should export the file? Should I try to convert it to a WEBP file?

@BenHenning I have updated the svg logo and created new webp images of different sizes for the new svg. Also, updated the dimens.xml with new values.
The results are pretty much the same as those in the screenshots in the above comments.
Should I send the new screenshots for different screen sizes or will you, as you said, check out the changes locally by yourself?

ArpitShukIa · 2021-04-29T10:13:18Z

I'm also wondering: is it possible for us to add splash tests for different configurations in Robolectric to verify that a bitmap vs. a picture drawable is used? I think you might even be able to verify the bitmap's dimensions for different configurations, so we can actually have tests for each of the new images and the drawable. Robolectric also lets you configure SDK per-test so you can validate the v23 qualifier.

@BenHenning I understand what needs to be done but am having little trouble figuring out how to do it.
I tried that "verifying the bitmap's dimensions for different configurations" part and this is what I came up with:

@Test
@Config(qualifiers = "port")
fun testSplashScreen_correctBitmapIsLoaded() {
  val context = ApplicationProvider.getApplicationContext<Application>()

  val drawable = context.resources.getDrawable(R.drawable.oppia_logo_bitmap)
  val expectedWidth = context.resources.getDimension(R.dimen.splash_screen_logo_width)
  val expectedHeight = context.resources.getDimension(R.dimen.splash_screen_logo_height)

  assertThat(drawable.intrinsicWidth).isEqualTo(expectedWidth.toInt())
  assertThat(drawable.intrinsicHeight).isEqualTo(expectedHeight.toInt())
}

I am not sure if this is the right way or not. This test verifies that the bitmap present for that config is of the same dimensions as those of the svg (for that config).
If this test makes sense, one problem with it is that it only passes with the png images and fails with the webp ones. The height and width of the webp drawables it calculates are incorrect. I also tried using BitmapFactory.decodeResource(...) followed by bitmap.width instead of drawable.intrinsicWidth but results are the same in both cases.

And, I need some help with that "verify webp images are used below API 23 and vector drawable is used for API>=23" part.
How to test this thing?

ArpitShukIa · 2021-05-05T07:08:33Z

@BenHenning PTAL at this (comment) and this (comment)

BenHenning · 2021-05-06T09:14:03Z

Sorry, been really busy with non-work items this week. Will need to take a look later in the week.

BenHenning · 2021-05-16T21:10:11Z

I'm currently traveling & won't be available next week for this review. I will take a look soon after I returned on or after 24th of May. I haven't lost track of this PR, I just haven't been able to look at it quite yet.

I appreciate your continued patience @ArpitShukIa.

Add dimensions for splash screen logo

88417cc

ArpitShukIa requested a review from rt4914 as a code owner March 12, 2021 12:40

ArpitShukIa changed the title ~~Fix #2655: Add dimensions for splash screen logo~~ Fix #2652: Add dimensions for splash screen logo Mar 12, 2021

anandwana001 assigned rt4914 Mar 12, 2021

rt4914 suggested changes Mar 15, 2021

View reviewed changes

rt4914 assigned ArpitShukIa and unassigned rt4914 Mar 15, 2021

ArpitShukIa removed their assignment Mar 16, 2021

rt4914 assigned rt4914 and ArpitShukIa and unassigned rt4914 Mar 17, 2021

ArpitShukIa removed their assignment Mar 18, 2021

rt4914 assigned ArpitShukIa Mar 19, 2021

Add png images for splash screen in lower APIs

d643375

ArpitShukIa requested a review from BenHenning as a code owner March 19, 2021 18:02

ArpitShukIa removed their assignment Mar 19, 2021

anandwana001 assigned rt4914 Mar 22, 2021

rt4914 suggested changes Mar 24, 2021

View reviewed changes

rt4914 assigned BenHenning and unassigned rt4914 Mar 24, 2021

BenHenning assigned ArpitShukIa Mar 25, 2021

ArpitShukIa removed their assignment Mar 25, 2021

rt4914 assigned ArpitShukIa and unassigned BenHenning Mar 28, 2021

ArpitShukIa removed their assignment Mar 28, 2021

rt4914 assigned BenHenning Mar 30, 2021

BenHenning requested changes Apr 22, 2021

View reviewed changes

BenHenning assigned ArpitShukIa and unassigned BenHenning Apr 22, 2021

BenHenning reviewed Apr 22, 2021

View reviewed changes

ArpitShukIa assigned BenHenning and unassigned ArpitShukIa Apr 23, 2021

Use webp instead of png images

1cf3abe

BenHenning assigned mschanteltc Apr 24, 2021

ArpitShukIa added 2 commits April 29, 2021 08:35

Merge branch 'develop' into splash-screen

fb8d39a

Update svg & webp logos and their dimensions and margins

e039219

anandwana001 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 7, 2021

rt4914 closed this Jun 22, 2021

BenHenning added the TODO(Ben): Revive label Mar 18, 2022

BenHenning mentioned this pull request Jan 13, 2023

Splash Screen Stretched #2652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2652: Add dimensions for splash screen logo #2894

Fix #2652: Add dimensions for splash screen logo #2894

ArpitShukIa commented Mar 12, 2021 •

edited

Loading

ArpitShukIa commented Mar 12, 2021

FareesHussain commented Mar 13, 2021

ArpitShukIa commented Mar 13, 2021

FareesHussain commented Mar 13, 2021

rt4914 left a comment

rt4914 Mar 15, 2021

ArpitShukIa Mar 16, 2021

rt4914 Mar 17, 2021

ArpitShukIa Mar 17, 2021

rt4914 Mar 19, 2021

ArpitShukIa commented Mar 19, 2021

rt4914 left a comment

ArpitShukIa commented Mar 24, 2021

BenHenning commented Mar 25, 2021

ArpitShukIa commented Mar 25, 2021

ArpitShukIa commented Mar 28, 2021 •

edited

Loading

BenHenning commented Mar 31, 2021

ArpitShukIa commented Apr 18, 2021

BenHenning left a comment

BenHenning Apr 22, 2021

ArpitShukIa Apr 23, 2021

BenHenning Apr 22, 2021

ArpitShukIa Apr 23, 2021

BenHenning Apr 22, 2021

ArpitShukIa Apr 23, 2021 •

edited

Loading

BenHenning Apr 24, 2021

mschanteltc Apr 28, 2021 •

edited

Loading

ArpitShukIa Apr 29, 2021

ArpitShukIa commented Apr 29, 2021

ArpitShukIa commented May 5, 2021

BenHenning commented May 6, 2021

BenHenning commented May 16, 2021

Fix #2652: Add dimensions for splash screen logo #2894

Fix #2652: Add dimensions for splash screen logo #2894

Conversation

ArpitShukIa commented Mar 12, 2021 • edited Loading

Explanation

Checklist

ArpitShukIa commented Mar 12, 2021

FareesHussain commented Mar 13, 2021

ArpitShukIa commented Mar 13, 2021

FareesHussain commented Mar 13, 2021

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArpitShukIa commented Mar 19, 2021

For API below 23: (I used API 19 here)

Pixel 2 5" (Portrait)

Pixel 2 5" (Land)

Pixel 4 XL 6.3" (Port)

Pixel 4 XL 6.3" (Land)

Nexus 9 Tablet 8.86" (Port)

Nexus 9 Tablet 8.86" (Land)

rt4914 left a comment

Choose a reason for hiding this comment

ArpitShukIa commented Mar 24, 2021

BenHenning commented Mar 25, 2021

ArpitShukIa commented Mar 25, 2021

ArpitShukIa commented Mar 28, 2021 • edited Loading

BenHenning commented Mar 31, 2021

ArpitShukIa commented Apr 18, 2021

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArpitShukIa Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschanteltc Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArpitShukIa commented Apr 29, 2021

ArpitShukIa commented May 5, 2021

BenHenning commented May 6, 2021

BenHenning commented May 16, 2021

ArpitShukIa commented Mar 12, 2021 •

edited

Loading

ArpitShukIa commented Mar 28, 2021 •

edited

Loading

ArpitShukIa Apr 23, 2021 •

edited

Loading

mschanteltc Apr 28, 2021 •

edited

Loading