Skip to content

Commit

Permalink
Fix part of oppia#632: Move PromotedStoryListAdapter to BindableAdater (
Browse files Browse the repository at this point in the history
oppia#4874)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

Fixes part of oppia#632: Reimplementation of PromotedStoryListAdapter as
BindableAdater
Espresso tests are failing, but they were already failing. The errors
with the new adapter are the same. The same tests pass when using
gradle.

There was an error in MarginBindingAdapter which I'm also fixing in this
cl: MarginBindingAdapter was replacing start/end margin changes for
left/right margin changes. However, this replacement relied on getting
the layout direction from the View being changed. When the direction is
inherited and the View is not attached to a parent (recursively until
top level View), the direction defaults to LTR, no matter the
Locale/language. This situation happens for example in item Views within
RecyclerView. The change does not replace start/end any more, but sets
them through MarginLayoutParamsCompat so that it is compatible with api
< 17.

## 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".
- [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)).

| Current | new |
|-|-|
|
![bindable_adapter_current](https://user-images.githubusercontent.com/103062089/217546850-30d59ac3-d996-4856-9a55-43a61e533eb2.png)
|
![bindable_adapter_new](https://user-images.githubusercontent.com/103062089/217546857-224a287d-b55a-4af9-8b05-e4855d78bd44.png)
|

Unit tests:

![recently_played_span_test](https://user-images.githubusercontent.com/103062089/217547368-f86fa7c4-c915-4257-948c-90a8e73c3ab6.png)

Espresso Tests. The run for the new version has an extra error, but it
is due to timings. In other runs it passes.

![recently_played_espresso_tests_current_vs_new](https://user-images.githubusercontent.com/103062089/217548889-21e0599e-e8b0-463d-a3f6-eeaaa93e674a.png)

@rt4914 PTAL

---------

Co-authored-by: Ben Henning <[email protected]>
  • Loading branch information
2 people authored and Uticodes committed Apr 4, 2023
1 parent 9a747f5 commit 29e786c
Show file tree
Hide file tree
Showing 7 changed files with 241 additions and 323 deletions.
1 change: 1 addition & 0 deletions app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ VIEW_MODELS_WITH_RESOURCE_IMPORTS = [
"src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt",
"src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryViewModel.kt",
"src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt",
"src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt",
"src/main/java/org/oppia/android/app/home/topiclist/TopicSummaryViewModel.kt",
"src/main/java/org/oppia/android/app/onboarding/OnboadingSlideViewModel.kt",
"src/main/java/org/oppia/android/app/onboarding/OnboardingViewModel.kt",
Expand Down
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,17 @@ 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.requestLayout();
}
}
Expand All @@ -68,12 +44,7 @@ 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.requestLayout();
}
}
Expand All @@ -92,8 +63,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;
}
}

This file was deleted.

Loading

0 comments on commit 29e786c

Please sign in to comment.