-
Notifications
You must be signed in to change notification settings - Fork 541
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
Fix #4072 and Fix Part of #4938: Revised profile chooser UI #5468
base: develop
Are you sure you want to change the base?
Conversation
…le-domain-config # Conflicts: # model/src/main/proto/oppia_logger.proto # testing/src/main/java/org/oppia/android/testing/logging/EventLogSubject.kt # utility/src/main/java/org/oppia/android/util/logging/EventBundleCreator.kt
…reen # Conflicts: # app/src/sharedTest/java/org/oppia/android/app/onboarding/CreateProfileFragmentTest.kt
…le-domain-config # Conflicts: # app/src/main/java/org/oppia/android/app/onboarding/IntroFragment.kt
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 43 KiB (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 35 KiB (Added) Method count: 260219 (old), 260523 (new), 304 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6868 (new), 50 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 42 KiB (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 36 bytes (Added) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 48 bytes (Added) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 48 bytes (Added) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 60 bytes (Added) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 36 bytes (Added) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 48 bytes (Added) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 60 bytes (Added) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 116287 (old), 116472 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5827 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 30 KiB (Added) Method count: 116293 (old), 116478 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5827 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 27 KiB (Added) Method count: 116293 (old), 116478 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5827 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
@adhiamboperes I think you mentioned in our quick sync today that you'd like another review on this PR. Is that correct, and does that mean that this PR is ready for another pass? I'm asking since it's still assigned to you, so I wasn't sure. :) |
…file-chooser-ui-views
@BenHenning, PTAL. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 38 KiB (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 30 KiB (Added) Method count: 260263 (old), 260552 (new), 289 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6820 (old), 6870 (new), 50 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 37 KiB (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 36 bytes (Added) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 48 bytes (Added) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 48 bytes (Added) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 60 bytes (Added) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 36 bytes (Added) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 48 bytes (Added) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 60 bytes (Added) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 33 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 27 KiB (Added) Method count: 115793 (old), 115978 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5788 (old), 5829 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 33 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 34 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115799 (old), 115984 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5788 (old), 5829 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 33 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 34 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115799 (old), 115984 (new), 185 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5788 (old), 5829 (new), 41 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 33 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adhiamboperes! Took another pass. I think most of my previous comments have been addressed, but a few haven't. Besides the open conversations, this includes:
- The 'add more profiles' and floating action button feel a bit crowded on portrait and would benefit from a bit more space (similar to the landscape version).
- As discussed during today's CLaM meeting, the landscape layout looks a bit off due to the solo learner bubble being so far left. I'm not quite sure that it should be centered, though, as landscape layouts are usually designed to have important elements easily reachable with one's thumbs. Perhaps placing it at the 1/4 or 1/3 portion of the screen from the left might be a bit cleaner? This seems even worse on tablet landscape mode--that's a lot of blank whitespace.
- As discussed during today's CLaM meeting, how does the new layout behave with Talkback?
PTAL at the open comment threads and the above.
app/src/main/java/org/oppia/android/app/profile/ProfileChooserFragmentPresenter.kt
Show resolved
Hide resolved
val offset = | ||
if (isLeft) scrollDistance - scrollableWidth else scrollableWidth - scrollDistance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice! Just to check (since I couldn't quite determine from the video): does RTL mode start with being scrolled all the way to the right? I see that the profile list is correctly reversed (with admin starting at the right), so just wanted to check for initial scroll state.
@@ -1225,7 +1226,7 @@ class ProfileManagementController @Inject constructor( | |||
// TODO(#3616): Migrate to the proper SDK 29+ APIs. | |||
@Suppress("DEPRECATION") // The code is correct for targeted versions of Android. | |||
val bitmap = MediaStore.Images.Media.getBitmap(context.contentResolver, avatarImagePath) | |||
val fileName = avatarImagePath.pathSegments.last() | |||
val fileName = UUID.randomUUID().toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder on this comment since I don't think it was addressed yet.
Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
@BenHenning, PTAL. I have addressed all pending comments, and update the screenshots in the main PR body. I should be able to upload talkback video tomorrow -- currently facing some physical device issues. |
Unassigning @adhiamboperes since a re-review was requested. @adhiamboperes, please make sure you have addressed all review comments. Thanks! |
Coverage ReportResultsNumber of files assessed: 19 Exempted coverageFiles exempted from coverage
|
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 40 KiB (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 32 KiB (Added) Method count: 260299 (old), 260600 (new), 301 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6830 (old), 6882 (new), 52 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 39 KiB (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 36 bytes (Added) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 48 bytes (Added) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 48 bytes (Added) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 60 bytes (Added) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 36 bytes (Added) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 48 bytes (Added) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 60 bytes (Added) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 26 KiB (Added) Method count: 115822 (old), 116011 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5841 (new), 43 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 34 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115828 (old), 116017 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5841 (new), 43 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 34 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 35 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115828 (old), 116017 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5841 (new), 43 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 34 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenHenning, PTAL.
I have addressed all pending comments, and update the screenshots in the main PR body. I should be able to upload talkback video tomorrow -- currently facing some physical device issues.
Thanks @adhiamboperes! Took a pass on the new code and left some comments. Just to check, did you address all of my 3 points in the main review message above (quoted from 2 reviews ago)? I did a scan on the images changed in the main PR post, but they don't seem different in any noticeable way and it seems like the original concerns may still be present (minus perhaps Talkback but I don't yet see a video for that to review).
|
||
@Test | ||
@Config(qualifiers = "land") | ||
fun testFragment_enableOnboardingV2_rtl_checkRightArrowScrollBehaviourIscorrect() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are this & the one above actually verifying RTL? Only testFragment_enableOnboardingV2_rtl_checkListIsAlphabetical seems to be enabling RTL explicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this test to use the Arabic locale.
|
||
launch(ProfileChooserActivity::class.java).use { | ||
it.onActivity { activity -> | ||
activity.window.decorView.layoutDirection = ViewCompat.LAYOUT_DIRECTION_RTL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually cause the activity to relayout into RTL?
An alternative that might work is switching the language to Arabic since that should force RTL as well, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored tests to use the Arabic locale, thanks for the suggestion!
|
||
@Test | ||
@Config(qualifiers = "land") | ||
fun testFragment_enableOnboardingV2_rtl_checkLeftArrowScrollBehaviourIscorrect() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun testFragment_enableOnboardingV2_rtl_checkLeftArrowScrollBehaviourIscorrect() { | |
fun testFragment_enableOnboardingV2_rtl_checkLeftArrowScrollBehaviourIsCorrect() { |
Ditto above. Though perhaps we can be more specific than 'correct'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated name to testFragment_enableOnboardingV2_rtl_checkLeftArrowScrollsListToTheRight
, ditto for similar tests.
) | ||
) | ||
|
||
testCoroutineDispatchers.runCurrent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Can't the test just end?
Ditto below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not required. I have removed it everywhere.
recyclerViewId = R.id.profiles_list_landscape, | ||
position = 0, | ||
targetViewId = R.id.profile_name_text, | ||
itemMatcher = allOf(withText("Admin"), isDisplayed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't technically verify that it's alphabetical, just that admin is first. Do we already verify elsewhere the whole list is populated correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored the test to verify the position of all the created profiles.
@@ -100,7 +100,8 @@ | |||
android:layout_width="@dimen/profile_selection_fragment_icon_size" | |||
android:layout_height="@dimen/profile_selection_fragment_icon_size" | |||
android:layout_marginEnd="@dimen/profile_selection_fragment_add_profile_button_margin_end" | |||
android:contentDescription="@string/profile_selection_profile_icon_description" | |||
android:layout_marginBottom="@dimen/profile_selection_fragment_add_button_margin_bottom" | |||
android:contentDescription="@string/profile_selection_profile_icon_description" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please remove extra indentation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* Used to retrieve the layout direction that should be used to mirror the direction of the | ||
* list based on locale. | ||
*/ | ||
@Inject lateinit var resourceHandler: AppLanguageResourceHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this class is a presenter, this can be injected directly via the class's constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored.
@@ -1225,7 +1226,7 @@ class ProfileManagementController @Inject constructor( | |||
// TODO(#3616): Migrate to the proper SDK 29+ APIs. | |||
@Suppress("DEPRECATION") // The code is correct for targeted versions of Android. | |||
val bitmap = MediaStore.Images.Media.getBitmap(context.contentResolver, avatarImagePath) | |||
val fileName = avatarImagePath.pathSegments.last() | |||
val fileName = UUID.randomUUID().toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to document what you've tried and what hasn't worked in a new issue that we could file to track this? While I'm fine with postponing adding a test for this now, it will eventually become a requirement as we work in the long-term toward 100% code coverage. It'd be nice to capture your efforts so that they don't need to be redone in the future.
val offset = | ||
if (isLeft) scrollDistance - scrollableWidth else scrollableWidth - scrollDistance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think that quite answers my question. The video seems to start with the list of profiles scrolled in the middle, so it's unclear which side of the list the user begins on. Is it the right or left when in RTL?
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 43 KiB (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 34 KiB (Added) Method count: 260321 (old), 260615 (new), 294 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6830 (old), 6887 (new), 57 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 42 KiB (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 60 bytes (Added) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 48 bytes (Added) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 48 bytes (Added) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 84 bytes (Added) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 60 bytes (Added) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 72 bytes (Added) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 84 bytes (Added) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 37 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 26 KiB (Added) Method count: 115840 (old), 116029 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5846 (new), 48 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 24 bytes (Added) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 24 bytes (Added) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 24 bytes (Added) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115846 (old), 116035 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5846 (new), 48 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 35 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 24 bytes (Added) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 24 bytes (Added) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 24 bytes (Added) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 36 KiB (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 31 KiB (Added) Method count: 115846 (old), 116035 (new), 189 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5846 (new), 48 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 11 MiB (new), 35 KiB (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 24 bytes (Added) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 24 bytes (Added) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 24 bytes (Added) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 24 bytes (Added) |
Explanation
Fixes #4072.
The selected image uri created by
com.miui.gallery
app in Xiaomi devices has a different format from the android recommended uri, and is therefore not parsed correctly. Changing the name of the stored file, to be generated using a random UUID, fixes the issue. Please see #4072 (comment) for more information.Fixes Part of #4938: Creates the new Profile chooser UI.
The portrait mode layout is a grid layout recylerview, while landscape mode utilizes a custom recyclerview to create a carousel. This design choice is kind of inconsistent, but more intuitive for users in landscape mode.
ProfileChooserFragmentPresenter
handles the data binding and UI interactions for both layouts.Care has been taken to ensure the existing profile behaviour e.g. random color selection and 10 profiles limit have been retained.
Additionally, a new
ParentScreen
enum has been created for the onboarding IntroActivity to control when to show the step count. Per PRD, the step count should not show for additional learners.Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Phone
Tablet