From 1c1841b36084a561c955010a34d4175b8e7e72b9 Mon Sep 17 00:00:00 2001 From: masclot <103062089+masclot@users.noreply.github.com> Date: Sat, 24 Jun 2023 08:34:42 +0200 Subject: [PATCH] Fix part of #632: Simplify MarginBindingAdapters and fix some RTL cases. Fix #2116: UI misalignment due to not resetting the layout. (#4965) ## Explanation Fix #2116: Fix part of #632: Simplify MarginBindingAdapters. The code is mostly the same as in PR #4874, which was reverted. This PR fixes the bug that caused the rollback. The rest of the reverted changes in PR #4874 will be included in a separate PR. The fix consists of resetting the layout params after changing the top and bottom margins, by calling View.setLayoutParams(), as explained in the [layout params documentation](https://developer.android.com/reference/android/view/ViewGroup.MarginLayoutParams#bottomMargin). The same call is not needed when calling a method instead of setting a property. It also fixes an RTL issue: if the view is not attached yet, there is no layout direction information available, making the old implementation render in LTR always. Some screenshots: |new|current| |-|-| |![home_rtl](https://user-images.githubusercontent.com/103062089/236451422-13843234-5f6c-4ed3-997a-e4b0459c3641.png) | ![home_rtl_current](https://user-images.githubusercontent.com/103062089/236451614-55595920-ed58-4329-8d7d-7bdbc0487688.png)| |![profile_chooser_landscape](https://user-images.githubusercontent.com/103062089/236451425-6766ed39-4a6b-4918-bf62-28bff5d05565.png)|![profile_chooser_landscape_current](https://user-images.githubusercontent.com/103062089/236451616-6979857b-f431-45cf-82c1-8db2a7618669.png)| |![profile_chooser_portrait](https://user-images.githubusercontent.com/103062089/236451427-ba5cc671-ff03-4718-ae45-9e3cc924efeb.png)|![profile_chooser_portrait_current](https://user-images.githubusercontent.com/103062089/236451621-058ec5b9-0cdb-4713-931e-c49bd9a5edba.png)| |![recently_played_rtl](https://user-images.githubusercontent.com/103062089/236451431-01e65a32-b448-422d-88a1-efb5326d5ab9.png)|![recently_played_rtl_current](https://user-images.githubusercontent.com/103062089/236451622-57a5cc80-f0d9-400d-8003-e9ff9735950b.png)| Test results: ![Test results](https://user-images.githubusercontent.com/103062089/236451433-d5a0e34f-75c0-4a6d-aeb5-88012913796f.png) ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [ ] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- .../databinding/MarginBindingAdapters.java | 45 +++---------------- .../databinding/MarginBindingAdaptersTest.kt | 37 ++++++++++++++- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java b/app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java index 5c336d0c05c..69dd1f379c4 100644 --- a/app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java +++ b/app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java @@ -3,7 +3,7 @@ import android.view.View; import android.view.ViewGroup.MarginLayoutParams; import androidx.annotation.NonNull; -import androidx.core.view.ViewCompat; +import androidx.core.view.MarginLayoutParamsCompat; import androidx.databinding.BindingAdapter; /** Holds all custom binding adapters that set margin values. */ @@ -14,12 +14,7 @@ public final class MarginBindingAdapters { public static void setLayoutMarginStart(@NonNull View view, float marginStart) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - float marginEnd = params.getMarginEnd(); - if (isRtlLayout(view)) { - setLayoutDirectionalMargins(view, (int) marginEnd, (int) marginStart); - } else { - setLayoutDirectionalMargins(view, (int) marginStart, (int) marginEnd); - } + MarginLayoutParamsCompat.setMarginStart(params, (int) marginStart); view.requestLayout(); } } @@ -29,36 +24,18 @@ public static void setLayoutMarginStart(@NonNull View view, float marginStart) { public static void setLayoutMarginEnd(@NonNull View view, float marginEnd) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - float marginStart = params.getMarginStart(); - if (isRtlLayout(view)) { - setLayoutDirectionalMargins(view, (int) marginEnd, (int) marginStart); - } else { - setLayoutDirectionalMargins(view, (int) marginStart, (int) marginEnd); - } + MarginLayoutParamsCompat.setMarginEnd(params, (int) marginEnd); view.requestLayout(); } } - private static void setLayoutDirectionalMargins( - @NonNull View view, - int marginStart, - int marginEnd - ) { - MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - params.setMargins(marginStart, params.topMargin, marginEnd, params.bottomMargin); - } - /** Used to set a margin-top for views. */ @BindingAdapter("app:layoutMarginTop") public static void setLayoutMarginTop(@NonNull View view, float marginTop) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - params.setMargins( - params.getMarginStart(), - (int) marginTop, - params.getMarginEnd(), - params.bottomMargin - ); + params.topMargin = (int) marginTop; + view.setLayoutParams(params); view.requestLayout(); } } @@ -68,12 +45,8 @@ public static void setLayoutMarginTop(@NonNull View view, float marginTop) { public static void setLayoutMarginBottom(@NonNull View view, float marginBottom) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - params.setMargins( - params.getMarginStart(), - params.topMargin, - params.getMarginEnd(), - (int) marginBottom - ); + params.bottomMargin = (int) marginBottom; + view.setLayoutParams(params); view.requestLayout(); } } @@ -92,8 +65,4 @@ public static void setLayoutMargin(@NonNull View view, float margin) { view.requestLayout(); } } - - private static boolean isRtlLayout(View view) { - return ViewCompat.getLayoutDirection(view) == ViewCompat.LAYOUT_DIRECTION_RTL; - } } diff --git a/app/src/sharedTest/java/org/oppia/android/app/databinding/MarginBindingAdaptersTest.kt b/app/src/sharedTest/java/org/oppia/android/app/databinding/MarginBindingAdaptersTest.kt index 35faf761900..5b7202c4d9b 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/databinding/MarginBindingAdaptersTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/databinding/MarginBindingAdaptersTest.kt @@ -4,6 +4,7 @@ import android.app.Activity import android.app.Application import android.content.Context import android.content.Intent +import android.view.View import android.widget.TextView import androidx.appcompat.app.AppCompatActivity import androidx.core.view.ViewCompat @@ -34,8 +35,10 @@ import org.oppia.android.app.application.ApplicationInjectorProvider import org.oppia.android.app.application.ApplicationModule import org.oppia.android.app.application.ApplicationStartupListenerModule import org.oppia.android.app.application.testing.TestingBuildFlavorModule +import org.oppia.android.app.databinding.MarginBindingAdapters.setLayoutMarginBottom import org.oppia.android.app.databinding.MarginBindingAdapters.setLayoutMarginEnd import org.oppia.android.app.databinding.MarginBindingAdapters.setLayoutMarginStart +import org.oppia.android.app.databinding.MarginBindingAdapters.setLayoutMarginTop import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule @@ -79,6 +82,7 @@ import org.oppia.android.testing.TestImageLoaderModule import org.oppia.android.testing.TestLogReportingModule import org.oppia.android.testing.junit.InitializeDefaultLocaleRule import org.oppia.android.testing.robolectric.RobolectricModule +import org.oppia.android.testing.threading.TestCoroutineDispatchers import org.oppia.android.testing.threading.TestDispatcherModule import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule @@ -109,8 +113,8 @@ class MarginBindingAdaptersTest { @get:Rule val initializeDefaultLocaleRule = InitializeDefaultLocaleRule() - @Inject - lateinit var context: Context + @Inject lateinit var context: Context + @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers @get:Rule val oppiaTestRule = OppiaTestRule() @@ -128,13 +132,27 @@ class MarginBindingAdaptersTest { fun setUp() { setUpTestApplicationComponent() Intents.init() + testCoroutineDispatchers.registerIdlingResource() } @After fun tearDown() { + testCoroutineDispatchers.unregisterIdlingResource() Intents.release() } + @Config(qualifiers = "land") + @Test + fun testMarginBindableAdapters_landscape_topAndBottomIsCorrect() { + testMarginBindableAdapters_topAndBottomIsCorrect() + } + + @Config(qualifiers = "sw600dp-land") + @Test + fun testMarginBindableAdapters_landscapeWide_topAndBottomIsCorrect() { + testMarginBindableAdapters_topAndBottomIsCorrect() + } + @Config(qualifiers = "port") @Test fun testMarginBindableAdapters_ltrIsEnabled_port_marginStartAndMarginEndForLtrIsCorrect() { @@ -275,6 +293,21 @@ class MarginBindingAdaptersTest { assertThat(textView.marginEnd.toFloat()).isWithin(TOLERANCE).of(40f) } + private fun testMarginBindableAdapters_topAndBottomIsCorrect() { + activityRule.scenario.runWithActivity { + val textView: TextView = it.findViewById(R.id.test_margin_text_view) + setLayoutMarginBottom(textView, /* marginBottom= */ 24f) + setLayoutMarginTop(textView, /* marginTop= */ 40f) + + testCoroutineDispatchers.runCurrent() + + val computedTopMargin = textView.y + val computedBottomMargin = (textView.parent as View).height - textView.y - textView.height + assertThat(computedTopMargin).isWithin(TOLERANCE).of(40f) + assertThat(computedBottomMargin).isWithin(TOLERANCE).of(24f) + } + } + private inline fun ActivityScenario.runWithActivity( crossinline action: (A) -> V ): V {