Fixes Issue #5832: Navigation Banner Appears in Media Details Screen #5839
Conversation
…ragment instance Before this commit, there was no easy way to check for and retrieve the ContributionFragment instance that was either the parent or grandparent (parent's parent) fragment. A complicated if check was required to retrieve it. After this commit, there is a simple helper method which will retrieve the ContributionFragment instance. Existing code can now be replaced by calling this method.
Before this commit, code would attempt to find and hide the nearby card that would appear when the user was looking at media details. However, this code did not work. After this commit, the old code has been replaced with code that correctly hides the nearby card. Also, this new code uses a helper method call and is overall easier to read.
|
I guess the loop will at least terminate when there is no parent, so I don't worry about infinite loop here. |
| } | ||
|
|
||
| //Hide the Nearby card when looking at media details | ||
| ContributionsFragment cf = this.getContributionsFragmentParent(); |
There was a problem hiding this comment.
In Java we prefer explicit variable names, even if long. So unless there is a reason I missed, would you mind renaming cf to contributionFragment?
| * be found, null is returned. | ||
| */ | ||
| private ContributionsFragment getContributionsFragmentParent(){ | ||
| Fragment f = this; |
There was a problem hiding this comment.
Similarly, f to fragment.
|
|
||
| //Hide the Nearby card when looking at media details | ||
| ContributionsFragment cf = this.getContributionsFragmentParent(); | ||
| if(cf != null && cf.binding != null){ |
There was a problem hiding this comment.
Sorry for nitpicking, but would you mind inserting a space character before if and while, here and below?
|
Before: screen-20240928-231429.mp4After: screen-20240928-225158.mp4 |
|
@rohit9625 If you have time, would you mind seeing whether this PR fixes the issue? As seen in my previous post above, for me both before and after are fast enough haha. |
I'm looking into it :》 |
|
It's still the same behavior on my device. Did you reproduce the issue before fixing it @Jason-Whitmore? |
Screen_recording_20240930_225134.mp4Explanation:- The Nearby Banner was still present even after opening I just realized @nicolas-raoul that |
|
@rohit9625 Ah thanks for pointing this out, I can replicate on main now. 🙂 I am out so I cannot test the PR now. |
|
No problem @nicolas-raoul :) |
|
Thank you all for the large discussion here. I'll try and address what I can. When I was developing the changes in this pull request, I was working on betaDebug. I had reproduced most of the issue there (on main), but the navigation banner never disappeared at all (rather than disappearing after a few seconds). Once the changes had been made, it appeared the bug was fixed. I assumed that fixing the bug on betaDebug would fix it on prodDebug. I did not test these changes directly on prodDebug before I submitted this pull request. I apologize. Just now I tested these changes on prodDebug and the bug appears fixed for me. However, I only have one contribution uploaded for my testing, rather than the 8 that @rohit9625 has in the most recent video. Could the number of contributions have some kind of effect on whether or not the bug appears? Thanks again for all the feedback. I'll keep working on this. |
|
After further debugging, I do not believe that the number of contributions is related to whether or not this bug appears. On the main branch, I can confirm (as @rohit9625 says) that moving the device's GPS location causes the navigation banner to hide when looking at the media details. The bug is more difficult to reproduce on the Android Studio emulator, which I had been exclusively using until recently. The bug is easier to reproduce using a physical device. On this branch (issue_5832), the bug appears to be fixed both on the Android Studio emulator and on my physical device. I've tried to reproduce the bug on this branch (prodDebug build), using both the Debug and Run options within Android Studio and the bug still appears to be fixed. Information about my Android Studio emulator and physical device is below: Emulator: Physical device: Other than differences in devices, I'm not sure what else could be causing the bug to appear fixed for me and unfixed for anyone else. Any suggestions or advice would be appreciated. |
Description (required)
Fixes #5832
What changes did you make and why?
There were two changes made.
First, a helper method was created to easily retrieve the ContributionsFragment. Accessing the ContributionsFragment is required to hide the navigation banner.
(One concern I have with this helper method is that it attempts to find the ContributionsFragment by looping through the parent fragments repeatedly in a while loop. The advantage of this strategy is that it can retrieve the ContributionsFragment anywhere in the Fragment parent-child structure if it exists. The primary disadvantage is that if the Fragment parent-child structure is a loop with no ContributionsFragment, then the while loop will never terminate.
I can rewrite this method to retrieve the ContributionsFragment directly (using two calls to getParentFragment()), but this strategy is less flexible. Should I rewrite this helper method to not use a while loop or is the current implementation okay?)
Second, old code which attempted to hide the navigation banner (but was unsuccessful) was replaced with a call to the helper method and then checking for null objects. If the objects are not null, then the navigation banner is successfully hidden.
Tests performed (required)
Tested betaDebug on Android Studio Emulator with API level 34.
Screenshots (for UI changes only)
issue_5832_compressed.webm
In this video, the navigation banner is visible from the contributions menu. When a specific contribution is selected, the navigation banner is hidden. Once the back button is pressed, the contributions menu appears with the navigation banner visible again.