Skip to content

Commit

Permalink
Fix part of #632: Simplify MarginBindingAdapters and fix some RTL cas…
Browse files Browse the repository at this point in the history 
…es. 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
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
<!-- Delete these section if this PR does not include UI-related
changes. -->
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>
  • Loading branch information
@masclot @adhiamboperes
masclot and adhiamboperes authored Jun 24, 2023
1 parent 2feb00d commit 1c1841b
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 40 deletions.
45 changes: 7 additions & 38 deletions app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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();
}
}
Expand All @@ -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();
}
}
Expand All @@ -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();
}
}
Expand All @@ -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;
}
}
37 changes: 35 additions & 2 deletions app/src/sharedTest/java/org/oppia/android/app/databinding/MarginBindingAdaptersTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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() {
Expand Down Expand Up @@ -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 <reified V, A : Activity> ActivityScenario<A>.runWithActivity(
crossinline action: (A) -> V
): V {
Expand Down

0 comments on commit 1c1841b

Please sign in to comment.