Skip to content

Delivery Progress UI fix #2989

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

Merged
OrangeAndGreen merged 2 commits into from
Mar 14, 2025
Merged

Delivery Progress UI fix #2989

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce1189a
Improved delivery progress view state adapter to not hold static refe…
OrangeAndGreen Mar 13, 2025
f5663df
Merge branch 'connect_qa' of https://github.com/dimagi/commcare-andro…
OrangeAndGreen Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 28 additions & 17 deletions app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void onCreate(Bundle savedInstanceState) {
}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container,
public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container,
Bundle savedInstanceState) {
ConnectJobRecord job = ConnectManager.getActiveJob();
getActivity().setTitle(R.string.connect_progress_delivery);
Expand Down Expand Up @@ -139,6 +139,12 @@ public void onPageSelected(int position) {
TabLayout.Tab tab = tabLayout.getTabAt(position);
tabLayout.selectTab(tab);

View view = viewStateAdapter.createFragment(position).getView();
if(view != null) {
pager.getLayoutParams().height = view.getMeasuredHeight();
pager.requestLayout();
}
Comment on lines +142 to +146
Copy link
Contributor

@shubham1g5 shubham1g5 Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@OrangeAndGreen OrangeAndGreen Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shubham1g5 shubham1g5 Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shubham1g5 shubham1g5 Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


FirebaseAnalyticsUtil.reportConnectTabChange(tab.getText().toString());
} else {
isTabChange = false;
Expand Down Expand Up @@ -214,6 +220,16 @@ private void jobCardDataHandle(View view, ConnectJobRecord job) {
}
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
if(item.getItemId() == R.id.action_sync) {
refreshData();
return true;
}

return false;
}

@Override
public void onResume() {
super.onResume();
Expand Down Expand Up @@ -337,37 +353,32 @@ private void updateUpdatedDate(Date lastUpdate) {
}

private static class ViewStateAdapter extends FragmentStateAdapter {
private static ConnectDeliveryProgressDeliveryFragment deliveryFragment = null;
private static ConnectResultsSummaryListFragment verificationFragment = null;
private final List<Fragment> fragmentList = new ArrayList<>();

public ViewStateAdapter(@NonNull FragmentManager fragmentManager, @NonNull Lifecycle lifecycle) {
super(fragmentManager, lifecycle);
fragmentList.add(ConnectDeliveryProgressDeliveryFragment.newInstance());
fragmentList.add(ConnectResultsSummaryListFragment.newInstance());
}

@NonNull
@Override
public Fragment createFragment(int position) {
if (position == 0) {
deliveryFragment = ConnectDeliveryProgressDeliveryFragment.newInstance();
return deliveryFragment;
}

verificationFragment = ConnectResultsSummaryListFragment.newInstance();
return verificationFragment;
return fragmentList.get(position);
}

@Override
public int getItemCount() {
return 2;
return fragmentList.size();
}

public void refresh() {
if (deliveryFragment != null) {
deliveryFragment.updateView();
}

if (verificationFragment != null) {
verificationFragment.updateView();
for (Fragment fragment : fragmentList) {
if (fragment instanceof ConnectDeliveryProgressDeliveryFragment) {
((ConnectDeliveryProgressDeliveryFragment) fragment).updateView();
} else if (fragment instanceof ConnectResultsSummaryListFragment) {
((ConnectResultsSummaryListFragment) fragment).updateView();
}
}
}
}
Expand Down