Skip to content

Commit

Permalink
Fix #5150: Correct home screen topic grid misalignment after returnin…
Browse files Browse the repository at this point in the history
…g 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 <[email protected]>
  • Loading branch information
TanishMoral11 and adhiamboperes authored Dec 20, 2024
1 parent 05d01a4 commit bc0483d
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand All @@ -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);
}
}

Expand All @@ -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();
}
}

Expand All @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit bc0483d

Please sign in to comment.