Delivery Progress UI fix #2989
Delivery Progress UI fix #2989
Conversation
…rences to Context objects. Updating tab height at bottom of page when changing tabs (to fix a vertical squishing bug).
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes update the Sequence Diagram(s)sequenceDiagram
participant U as User
participant CP as ConnectDeliveryProgressFragment
participant VSA as ViewStateAdapter
participant F as Fragment
U->>CP: Select sync action (menu item)
CP->>CP: onOptionsItemSelected(MenuItem)
CP->>VSA: Call refresh()
VSA->>F: Iterate over fragment list
F-->>VSA: updateView() invoked (if applicable)
sequenceDiagram
participant CP as ConnectDeliveryProgressFragment
participant VP as ViewPager2
participant CF as Current Fragment
CP->>CP: onPageSelected(pageIndex)
CP->>CF: Retrieve current fragment view
CF-->>CP: Return measured view height
CP->>VP: Adjust height based on measured value
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (1)
354-359: Improved refresh method with fragment type checking.The updated approach properly iterates through all fragments and calls the appropriate update method based on the fragment type. This is more maintainable and avoids static references.
Consider using an interface instead of instanceof checks to make this more extensible:
- for (Fragment fragment : fragmentList) { - if (fragment instanceof ConnectDeliveryProgressDeliveryFragment) { - ((ConnectDeliveryProgressDeliveryFragment) fragment).updateView(); - } else if (fragment instanceof ConnectResultsSummaryListFragment) { - ((ConnectResultsSummaryListFragment) fragment).updateView(); - } - } + // Define an interface in another file + public interface Updatable { + void updateView(); + } + + // Then implement it in both fragment classes + + // Here, simply call: + for (Fragment fragment : fragmentList) { + if (fragment instanceof Updatable) { + ((Updatable) fragment).updateView(); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(5 hunks)
🔇 Additional comments (8)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (8)
6-6: Added the necessary import for MenuItem handling.This addition supports the new
onOptionsItemSelectedmethod that you'll implement later in the class.
70-70: Good use of @nonnull annotation for type safety.Adding the
@NonNullannotation to theLayoutInflaterparameter enhances type safety and helps prevent potential null pointer exceptions.
201-209: Added options menu handling for sync action.This implementation properly handles the sync menu item selection and refreshes data appropriately.
334-334: Removed static references by using a dynamic fragment list.This change addresses the potential memory leak issue mentioned in the PR by storing fragments in a dynamic list instead of using static references.
338-339: Proper initialization of fragment instances.The fragments are now instantiated once during adapter creation and stored in the list, which is cleaner than creating them on demand.
345-345: Simplified fragment creation by retrieving from list.This change makes the code more maintainable by eliminating conditional fragment creation logic.
350-350: Dynamic item count based on fragment list size.This change makes the adapter more flexible for potential future changes where the number of fragments might vary.
138-143:Details
❓ Verification inconclusive
Nice fix for the vertically squished tabs issue.
This change dynamically adjusts the ViewPager2 height based on the measured height of the current fragment's view, which should resolve the UI issue described in the PR.
However, this approach assumes the fragment's view has already been measured. Consider adding a check to handle cases where measurement hasn't occurred yet.
🏁 Script executed:
#!/bin/bash # Check if there are any similar approaches in the codebase rg -A 5 "getMeasuredHeight" --glob "*.java" | grep -v "ConnectDeliveryProgressFragment.java"Length of output: 5326
Suggestion: Add a safeguard to ensure the view has been measured before updating the ViewPager2 height
The fix overall looks good and resolves the issue of squished tabs by dynamically adapting to the fragment’s view height. However, as noted, the current implementation assumes that the fragment’s view has already been measured. This can be risky if the view’s measurement returns 0 (or isn’t ready), potentially leading to layout issues.
Recommendations:
- In app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (lines 138–143), consider checking if
view.getMeasuredHeight()returns a positive value before applying it to the ViewPager2’s layout parameters. For example, an extra condition like:if(view != null && view.getMeasuredHeight() > 0) { pager.getLayoutParams().height = view.getMeasuredHeight(); pager.requestLayout(); }- Alternatively, if there’s a chance the view is not yet measured, you might defer the height update (for example, by using a layout listener) to ensure that the measured height is reliable.
This approach is similar to other parts of the codebase that rely on
getMeasuredHeight()(e.g., inEnhancedWrapContentViewPager.javaand widgets withinQuestionWidget.java), so consider whether a uniform strategy across similar components could further improve consistency.
| View view = viewStateAdapter.createFragment(position).getView(); | ||
| if(view != null) { | ||
| pager.getLayoutParams().height = view.getMeasuredHeight(); | ||
| pager.requestLayout(); | ||
| } |
There was a problem hiding this comment.
can you talk more about the issue here ? As though why does the view get cut off and can't it be corrected from view xml instead by adding margin to the xml view ?
There was a problem hiding this comment.
I'm having a hard time figuring out what the underlying issue is here, it seems to me that the FragmentStateAdapter should be handling the size of the fragments. After spending some time and not making much progress I decided to let Github Copilot try to help me! This was the solution it suggested, and when I tried it out it indeed worked.
But I'm open to looking for a better fix, I'd love to understand what's getting confused such that the fragment ends up too short despite having clear layout params. Another odd thing to note is that changing between the tabs seems to work fine at first. The issue only occurs when the user is on the second tab and presses a button (which causes some UI to change), then navigates back to the first tab.
I don't think I'll be able to get to that before the cohort starts on Monday though, so is the current change acceptable for now to fix the user-facing bug? I can create another ticket to revisit this next week and if I'm still struggling then I'll recruit Jignesh for some fresh eyes.
There was a problem hiding this comment.
sounds good to me, we should probably add a check that view.getMeasuredHeight > 0 before height assignment as well to make this code more fail-safe.
There was a problem hiding this comment.
It would also be good to add a code comment there to denote why we are doing it as it's not entirely obvious by looking at the code.
… into dv/delivery_progress_ui_fix
|
Thanks Shubham, I created a ticket to revisit this next week. Merging the existing fix for now. |
https://dimagi.atlassian.net/browse/CCCT-838
cross-request: dimagi/commcare-core#1455
Product Description
Delivery progress tabs no longer squished vertically when changing between tabs.
Technical Summary
This PR fixes a bug in which part of the UI would sometimes be sized too short (matching a different UI), such that elements were squished. Also while working in this code, I cleaned things up slightly so as not to hold static references to the two fragments (which equated to holding static references to Context objects).
Showing the error:
Feature Flag
Connect
Safety Assurance
Safety story
Tested the code out to verify things size correctly now and nothing got broken in my cleanup.
Otherwise this is mostly just about updating the height of a UI element, pretty simple change.
Automated test coverage
No automated tests for Connect yet.
QA Plan
See steps to reproduce in the Jira ticket. The UI should no longer get squished when following the steps.
Labels and Review