From bc0483dad104f7c4d0f0d2040233a26fa1c2742c Mon Sep 17 00:00:00 2001 From: Tanish Moral <134790673+TanishMoral11@users.noreply.github.com> Date: Sat, 21 Dec 2024 01:29:42 +0530 Subject: [PATCH] Fix #5150: Correct home screen topic grid misalignment after returning from lesson (#5563) ## Explanation Fixes #5150 , where the home screen topic grid becomes misaligned after returning from a lesson. The grid alignment is now maintained upon returning to the home screen, ensuring a consistent layout for topics. ## Issue Description The home screen topics grid view experiences misalignment when navigating back from a lesson. This issue stems from inconsistent layout parameter management within the margin binding adapters, causing unexpected UI disruptions across different screen configurations. ## Changes Made - Refactored `MarginBindingAdapters` to prioritize `setLayoutParams()` over `requestLayout()` - Implemented a more robust approach to setting and preserving layout parameters - Ensured consistent margin application across various screen orientations and device types ## Reasoning The core of the fix lies in how layout parameters are managed: - `View.setLayoutParams()` explicitly sets layout parameters for a view - This method inherently calls `requestLayout()`, guaranteeing the view is redrawn with updated parameters - Direct parameter setting provides more predictable margin management across different screen configurations ## Test Coverage Comprehensive testing was added to `MarginBindingAdaptersTest` to validate: - Margin preservation across multiple screen configurations - Accurate application of start, end, top, and bottom margins - Consistency of layout parameter management - Robust handling of different device orientations (portrait, landscape, tablet) ## Essential Checklist - [x] The PR title and explanation each start with "Fix #5150: ". - [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". - [x] 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)). --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- .../databinding/MarginBindingAdapters.java | 21 +---- .../databinding/MarginBindingAdaptersTest.kt | 77 ++++++++++++++++++- 2 files changed, 77 insertions(+), 21 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 2c7cd8be971..a94e1deb30e 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 @@ -15,7 +15,7 @@ public static void setLayoutMarginStart(@NonNull View view, float marginStart) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); MarginLayoutParamsCompat.setMarginStart(params, (int) marginStart); - view.requestLayout(); + view.setLayoutParams(params); } } @@ -25,7 +25,7 @@ public static void setLayoutMarginEnd(@NonNull View view, float marginEnd) { if (view.getLayoutParams() instanceof MarginLayoutParams) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); MarginLayoutParamsCompat.setMarginEnd(params, (int) marginEnd); - view.requestLayout(); + view.setLayoutParams(params); } } @@ -36,7 +36,6 @@ public static void setLayoutMarginTop(@NonNull View view, float marginTop) { MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); params.topMargin = (int) marginTop; view.setLayoutParams(params); - view.requestLayout(); } } @@ -47,22 +46,6 @@ public static void setLayoutMarginBottom(@NonNull View view, float marginBottom) MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); params.bottomMargin = (int) marginBottom; view.setLayoutParams(params); - view.requestLayout(); - } - } - - /** Used to set a margin for views. */ - @BindingAdapter("layoutMargin") - public static void setLayoutMargin(@NonNull View view, float margin) { - if (view.getLayoutParams() instanceof MarginLayoutParams) { - MarginLayoutParams params = (MarginLayoutParams) view.getLayoutParams(); - params.setMargins( - (int) margin, - (int) margin, - (int) margin, - (int) margin - ); - view.requestLayout(); } } } 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 0ac0a8483b9..a8d68ef8997 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 @@ -5,6 +5,7 @@ import android.app.Application import android.content.Context import android.content.Intent import android.view.View +import android.view.ViewGroup import android.widget.TextView import androidx.appcompat.app.AppCompatActivity import androidx.core.view.ViewCompat @@ -112,8 +113,11 @@ class MarginBindingAdaptersTest { @get:Rule val initializeDefaultLocaleRule = InitializeDefaultLocaleRule() - @Inject lateinit var context: Context - @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers + @Inject + lateinit var context: Context + + @Inject + lateinit var testCoroutineDispatchers: TestCoroutineDispatchers @get:Rule val oppiaTestRule = OppiaTestRule() @@ -292,6 +296,75 @@ class MarginBindingAdaptersTest { assertThat(textView.marginEnd.toFloat()).isWithin(TOLERANCE).of(40f) } + @Config(qualifiers = "port") + @Test + fun testMarginBindableAdapters_setLayoutParams_preservesMargins() { + val textView = activityRule.scenario.runWithActivity { + val textView: TextView = it.findViewById(R.id.test_margin_text_view) + + // Set initial margins + setLayoutMarginStart(textView, /* marginStart= */ 24f) + setLayoutMarginEnd(textView, /* marginEnd= */ 40f) + setLayoutMarginTop(textView, /* marginTop= */ 16f) + setLayoutMarginBottom(textView, /* marginBottom= */ 32f) + + return@runWithActivity textView + } + + assertThat(textView.marginStart.toFloat()).isWithin(TOLERANCE).of(24f) + assertThat(textView.marginEnd.toFloat()).isWithin(TOLERANCE).of(40f) + + val layoutParams = textView.layoutParams as ViewGroup.MarginLayoutParams + assertThat(layoutParams.topMargin.toFloat()).isWithin(TOLERANCE).of(16f) + assertThat(layoutParams.bottomMargin.toFloat()).isWithin(TOLERANCE).of(32f) + } + + @Config(qualifiers = "land") + @Test + fun testMarginBindableAdapters_landscapeMode_setLayoutParams_preservesMargins() { + val textView = activityRule.scenario.runWithActivity { + val textView: TextView = it.findViewById(R.id.test_margin_text_view) + + // Set initial margins + setLayoutMarginStart(textView, /* marginStart= */ 24f) + setLayoutMarginEnd(textView, /* marginEnd= */ 40f) + setLayoutMarginTop(textView, /* marginTop= */ 16f) + setLayoutMarginBottom(textView, /* marginBottom= */ 32f) + + return@runWithActivity textView + } + + assertThat(textView.marginStart.toFloat()).isWithin(TOLERANCE).of(24f) + assertThat(textView.marginEnd.toFloat()).isWithin(TOLERANCE).of(40f) + + val layoutParams = textView.layoutParams as ViewGroup.MarginLayoutParams + assertThat(layoutParams.topMargin.toFloat()).isWithin(TOLERANCE).of(16f) + assertThat(layoutParams.bottomMargin.toFloat()).isWithin(TOLERANCE).of(32f) + } + + @Config(qualifiers = "sw600dp-port") + @Test + fun testMarginBindableAdapters_tabletMode_setLayoutParams_preservesMargins() { + val textView = activityRule.scenario.runWithActivity { + val textView: TextView = it.findViewById(R.id.test_margin_text_view) + + // Set initial margins + setLayoutMarginStart(textView, /* marginStart= */ 24f) + setLayoutMarginEnd(textView, /* marginEnd= */ 40f) + setLayoutMarginTop(textView, /* marginTop= */ 16f) + setLayoutMarginBottom(textView, /* marginBottom= */ 32f) + + return@runWithActivity textView + } + + assertThat(textView.marginStart.toFloat()).isWithin(TOLERANCE).of(24f) + assertThat(textView.marginEnd.toFloat()).isWithin(TOLERANCE).of(40f) + + val layoutParams = textView.layoutParams as ViewGroup.MarginLayoutParams + assertThat(layoutParams.topMargin.toFloat()).isWithin(TOLERANCE).of(16f) + assertThat(layoutParams.bottomMargin.toFloat()).isWithin(TOLERANCE).of(32f) + } + private fun testMarginBindableAdapters_topAndBottomIsCorrect() { activityRule.scenario.runWithActivity { val textView: TextView = it.findViewById(R.id.test_margin_text_view)