Fix part of #632: Move PromotedStoryListAdapter to BindableAdater #4874
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Checking out the latest code in 'develop'
|
@rt4914 PTAL |
…-bindableAdater' into move-promotedStoryListAdapter-to-bindableAdater
BenHenning
requested changes
Feb 8, 2023
There was a problem hiding this comment.
Thanks @masclot! Took a first pass--PTAL. Also, I think there are CI checks failing that should be fixed.
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt
Show resolved
Hide resolved
|
Also, please make sure to include the 'Fixes part of #632' within the PR description. |
…a factory. Turn some dependencies from AppCompatActivity to Context, so that unit tests do not require bringing up an activity/fragment. Add Robolectric unit tests for RecentlyPlayedViewModel.kt
|
I added unit tests |
|
Ah, for the first one, scroll up the page further and click "Load more". Here's the start of that thread: #4874 (comment) -- you can reply there. If the comment box says Pending, that means that you have draft comments that haven't been submitted. Go to the "Files Changed" tab, and submit them using the green button in the top right. |
|
Thanks! I thought that big green button was for reviewers, not for replies to review-comments. I also don't understand why some of my comments went through without clicking that button. |
|
Ah ok, great, thanks @masclot! Not sure about why some of the comments went through, that seems odd. Also just wondering, do you have any improvements to suggest to the docs here that would have made the process clearer? If so, would you be up for making a PR to update them? (Now that the wiki docs are stored in the main repo, that should hopefully be straightforward.) Thanks! |
cc @seanlip. @masclot, I believe what happened here was: in some cases you replied from the conversation view, which adds a single comment, and in a later instance, you chose to This vs
|
BenHenning
approved these changes
Mar 30, 2023
|
Syncing with develop & enabling auto-merge. |
|
Unassigning @BenHenning since they have already approved the PR. |
|
Hi @masclot, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
I'll try to compile a list of things I had trouble with. The documentation as a whole is big; I'm sure it has answers to at least some of my troubles, but I just could not remember the solution when I faced them. |
|
@masclot Sounds good, thanks a lot -- appreciate it! /cc @MohitGupta121 too, who is doing some work to update the wiki to make it easier to follow. |
Uticodes
pushed a commit
to Uticodes/oppia-android
that referenced
this pull request
Apr 4, 2023
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 | |-|-| |  |  | Unit tests:  Espresso Tests. The run for the new version has an extra error, but it is due to timings. In other runs it passes.  @rt4914 PTAL --------- Co-authored-by: Ben Henning <ben@oppia.org>
6 tasks
BenHenning
pushed a commit
that referenced
this pull request
Apr 21, 2023
…dableAdapter" (#4951) This reverts commit `7fd290d68f0440d926f2d443dbd7bfb28ab20547`, fixing part of #2116. While testing #2116, we found that the UI for default profile view does not display correctly, and traced the change to #4874 in [this discusson](#2116 (comment)). #4874 introduces a change to `MarginBindingAdapters` which utilizes `MarginLayoutParamsCompat` to compute the margins of a layout before drawing it. I suspect that this works for the `Promoted Story` and `Topic Summary` because we compute the start and end margins of each item relative to grid columns laid out on the HomeActivity, but no such implementation exists for the Profile Chooser view. We need to do some further investigation into this, hence the decision to revert. We will need to test all the associated screens to make sure they still look as expected:  ## 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)). ## For UI-specific PRs only This was mainly a re-implimentation per #632, and the UI did not change. Please refer to #4874 for screenshots. The Affected screen displays okay with this revert:  Other screens: | | | |---|---| |||
|](https://user-images.githubusercontent.com/59600948/233126283-45a860c7-26ac-47f6-8b38-e54b1ee7cb1f.png)||){kind=link}
6 tasks
6 tasks
adhiamboperes
added a commit
that referenced
this pull request
Jun 24, 2023
…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| |-|-| | | | ||| ||| ||| Test results:  ## 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>
|){kind=link}
|](https://user-images.githubusercontent.com/103062089/236451425-6766ed39-4a6b-4918-bf62-28bff5d05565.png)||){kind=link}
|](https://user-images.githubusercontent.com/103062089/236451427-ba5cc671-ff03-4718-ae45-9e3cc924efeb.png)||){kind=link}
|](https://user-images.githubusercontent.com/103062089/236451431-01e65a32-b448-422d-88a1-efb5326d5ab9.png)||){kind=link}
6 tasks
adhiamboperes
added a commit
that referenced
this pull request
Jul 11, 2023
Fix #632: Move PromotedStoryListAdapter to BindableAdater This change reverts partially #4951, which in turn reverted #4874 due to a bug. It is a partial revert because the bug was fixed independently in the previous PR #4965. I added RecentlyPlayedViewModel.kt to the list of classes not needing tests, as is usual for ViewModels. This was also part of the original PR. | current | new | |- | - | || | ||  | ||| ||| ## 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)). ## 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/op pia/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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes part of #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
Unit tests:
Espresso Tests. The run for the new version has an extra error, but it is due to timings. In other runs it passes.
@rt4914 PTAL