Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG]: Home screen topics Grid view unaligned after coming back from Lesson #5150

Closed
MohitGupta121 opened this issue Sep 12, 2023 · 22 comments · Fixed by #5563
Closed

[BUG]: Home screen topics Grid view unaligned after coming back from Lesson #5150

MohitGupta121 opened this issue Sep 12, 2023 · 22 comments · Fixed by #5563
Assignees
@TanishMoral11
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Milestone
1.x GA Follow-up ...

Comments

@MohitGupta121
Copy link
Member

MohitGupta121 commented Sep 12, 2023 •
edited
Loading

Describe the bug

Home screen topics Grid view unaligned after coming back from Lesson

VID_20230912_120527.mp4

Steps To Reproduce

Go to any topic and then any lesson
Go back by (System or Navigation)
See the Topic Grids now

Expected Behavior

Topic Grid should remain aligned.

Screenshots/Videos

No response

What device/emulator are you using?

Poco M2 Pro

Which Android version is your device/emulator running?

Android 11

Which version of the Oppia Android app are you using?

0.11-oppia-f0837d22fe

Additional Context

No response
@MohitGupta121 MohitGupta121 added bug End user-perceivable behaviors which are not desirable. triage needed labels Sep 12, 2023
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. good first issue This item is good for new contributors to make their pull request. and removed triage needed labels Sep 12, 2023
@adhiamboperes adhiamboperes added this to the 1.0 Global availability milestone Sep 12, 2023
@adhiamboperes adhiamboperes added Impact: Low Low perceived user impact (e.g. edge cases). and removed Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). labels Sep 15, 2023
@Kshitiz-Mhto
Copy link

Kshitiz-Mhto commented Sep 18, 2023 •
edited
Loading

I am new to Oppia-Android project, I would like to work on this issue, could you plz assign it to me? @adhiamboperes @MohitGupta121

@adhiamboperes
Copy link
Collaborator

I am new to Oppia-Android project, I would like to work on this issue, could you plz assign it to me? @adhiamboperes @MohitGupta121

Could you please give a brief description of how you would go about solving this.

@adhiamboperes
Copy link
Collaborator

This happens on tablets as well.
Screenshot_1695036566 Screenshot_1695036506

@MohitGupta121
Copy link
Member Author

Thanks @adhiamboperes for tablet investigation.

@adhiamboperes adhiamboperes added Work: High It's not clear what the solution is. and removed Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels Sep 19, 2023
@geektan123
Copy link

Assign#5150

@adhiamboperes
Copy link
Collaborator

Assign#5150

Hi, in order to be assigned any issue, please give a brief description of how you would go about solving it.

nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 22, 2023
@nikitaevg
fix oppia#5150, simplify the margins' calculation
2164255 
This is just a proposal, will brush it up if the proposal is approved
nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 22, 2023
@nikitaevg
fix oppia#5150, simplify the margins' calculation
037ee0e 
This is just a proposal, will brush it up if the proposal is approved
@nikitaevg
Copy link

nikitaevg commented Sep 22, 2023 •
edited
Loading

Hey there!

I want to work on this project as well.

I tried to understand what's going on here, I failed:). It has to do something with margins (this function).

I had some non viable hypothesis. One interesting thing here is that when this bug happens this function I linked first is called for the 5th (last) element and only then for 0th, 1st...

Anyway, I came up with this solution. I don't know why it works, but the code becomes simpler and it works - the problem doesn't appear anymore, see the video. I checked it on Pixel 3 and Pixel C.

I understand it changes the layout, but I think it's not that noticeable. I also understand it's not a proper solution, but it works:) See the new layout for Pixel 3 and Pixel C attached.

WDYT about this approach?

Pixel3
Pixel3_landscape
Pixel C

nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 22, 2023
@nikitaevg
fix oppia#5150, simplify the margins' calculation
023d5b0 
This is just a proposal, will brush it up if the proposal is approved
@seanlip
Copy link
Member

seanlip commented Sep 23, 2023 •
edited
Loading

Hi @nikitaevg -- thanks for this! Just from the PM side -- in the new layout, I think that the tiles in the right column in the Pixel 3 mocks are too close to the right edge of the screen. The margin should be consistent on the left and on the right. Is that achievable with your approach?

Also, I suggest showing what the equivalent mocks are for the current codebase so that the difference between the old/new ones can be seen more easily,

@adhiamboperes do you have any thoughts on the technical approach used here? Just as an FYI I found the relevant PR that introduced the changes (using the blame functionality in GitHub): see https://github.com/oppia/oppia-android/pull/2246/files#diff-9d65508457d46773757a6d2bb48b791b0dee5f9fb9e46c673a808c5de2294172R49 -- @nikitaevg maybe the comments and context there might be interesting as well.

nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 24, 2023
@nikitaevg
proposal of a fix oppia#5150, update recycler view
961cf00
@nikitaevg
Copy link

nikitaevg commented Sep 24, 2023 •
edited
Loading

Hey Sean!

Thanks for your comment! Yeah, it was my bad they were inconsistent on the left and right. I thought it was like this before. Sure, I don't want to break it:)

I found a real problem here. My new proposal is here, will create a proper PR soon.

The gist is these cards that become unaligned - they are being reused after returning to the main screen. Their margin is updated in the code, but the RecyclerView is not notified about those updates.

Check this video where you can see the bug is gone.

@adhiamboperes
Copy link
Collaborator

Hey Sean!

Thanks for your comment! Yeah, it was my bad they were inconsistent on the left and right. I thought it was like this before. Sure, I don't want to break it:)

I found a real problem here. My new proposal is here, will create a proper PR soon.

The gist is these cards that become unaligned - they are being reused after returning to the main screen. Their margin is updated in the code, but the RecyclerView is not notified about those updates.

Check this video where you can see the bug is gone.

Hi,
I think this approach is better and it is consistent with my own investigation into the root cause of the bug as well.

@adhiamboperes adhiamboperes removed their assignment Sep 24, 2023
nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 26, 2023
@nikitaevg
fix oppia#5150: set layout params correctly
eb6db65
@nikitaevg nikitaevg mentioned this issue Sep 26, 2023
Closed
6 tasks
nikitaevg added a commit to nikitaevg/oppia-android that referenced this issue Sep 26, 2023
@nikitaevg
fix oppia#5150: set layout params correctly
77764f1
@adhiamboperes
Copy link
Collaborator

@nikitaevg proposed calling View.setLayoutParams() instead of View.requestLayout() in MarginBindingAdapters in #5160.

View.setLayoutParams() sets the layout parameters associated with this view. These supply parameters to the parent of this view specifying how it should be arranged.
View.requestLayout() should be called when something has changed which has invalidated the layout of this view. This will schedule a layout pass of the view tree. View.setLayoutParams() calls this method. At a glance, this does not handle the provided layout params, unlike the other method.

We need to test this code to prevent regressions in the UI, and the first step is to set up a test that does something very similar to the bug and see if we can repro the breakage in Robolectric, then we can step-debug to see what is causing the breakage, and set up a proper test for the code.

  • This means setting up the test in HomeFragmentTest which has the layout under test.
  • Observe the behavior when manually reproing the issue and try to turn it into a test. This acts like a debuggable environment for analyzing android.

@adhiamboperes adhiamboperes added Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels Nov 6, 2023
@Abinash6000
Copy link
Contributor

Unable to reproduce the error. Am I doing something wrong?

screenrec.mp4

@adhiamboperes
Copy link
Collaborator

Hi @Abinash6000, could you please try to repro this on a tablet device like a nexus 9?

@Abinash6000
Copy link
Contributor

Abinash6000 commented Nov 22, 2023 •
edited
Loading

Still unable to reproduce the error.

portrait.mp4
landscape.mp4

@AyeshaLoladia
Copy link

Hii, it would be of great help if anyone let's me know whether this issue has been solved yet or not. I would like to work on it

@adhiamboperes
Copy link
Collaborator

@AyeshaLoladia, please try to reproduce this issue on mobile and tablet emulators and then let us know so we can assign it to you!

@Vinay-Khanagavi
Copy link

Vinay-Khanagavi commented Oct 19, 2024 •
edited
Loading

@MohitGupta121 It's about a year since this issue was created why has nobody solved this issue, I just saw this bug and wanted to open an issue and found It already exists and it is a terrible sign for any app,
it would be better if you could make this impact medium from low, Thank you

@seanlip seanlip added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). and removed Impact: Low Low perceived user impact (e.g. edge cases). labels Oct 28, 2024
@seanlip
Copy link
Member

seanlip commented Oct 28, 2024

@Vinay-Khanagavi Thanks for the note! I agree with you that it is medium-impact, and I have updated the label accordingly.

@TanishMoral11
Copy link
Contributor

Hey @adhiamboperes , I’d like to start working on this issue, but I noticed PR #5160 . already seems to address it effectively.
Could you let me know if there were any specific concerns that prevented it from being merged?
This would help me understand how to proceed or make any needed improvements.
Thank you!

@adhiamboperes
Copy link
Collaborator

@TanishMoral11, there were some incomplete tests and the contributor abandoned the PR. The solution is otherwise acceptable

@TanishMoral11
Copy link
Contributor

Thank you for clarifying, @adhiamboperes !

Would it be alright if I submit a new PR with the same changes and ensure that all the tests are completed?
I’d be happy to take it forward and address any additional requirements if needed.
Thank you!

@adhiamboperes
Copy link
Collaborator

Assigned this issue to you @TanishMoral11

TanishMoral11 added a commit to TanishMoral11/oppia-android that referenced this issue Nov 2, 2024
@TanishMoral11
Fix: Correct home screen topic grid misalignment after returning from…
9a89bed 
… lesson (oppia#5150)
@TanishMoral11 TanishMoral11 mentioned this issue Nov 2, 2024
Merged
6 tasks
adhiamboperes added a commit that referenced this issue Dec 20, 2024
@TanishMoral11 @adhiamboperes
Fix #5150: Correct home screen topic grid misalignment after returnin…
bc0483d 
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@TanishMoral11 TanishMoral11

Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Projects
[Team] Core Learner and Mastery flows & UI Frontend - Android
Status: Done
Milestone
1.x GA Follow-up Items