-
-
Notifications
You must be signed in to change notification settings - Fork 45
Created base waiting loader and implemented in multiple connect screens #3378
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.
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
Conversation
📝 WalkthroughWalkthroughThis PR introduces a generic view-binding base fragment architecture with integrated loading overlay support. It adds a new Sequence DiagramsequenceDiagram
participant Fragment Lifecycle
participant BaseFragment
participant ConnectFragment
participant LoadingBinding
participant ChildBinding
Fragment Lifecycle->>BaseFragment: onCreate()
Fragment Lifecycle->>BaseFragment: onCreateView()
activate BaseFragment
BaseFragment->>BaseFragment: inflateBinding() [abstract]
BaseFragment->>ConnectFragment: inflateBinding() [concrete]
ConnectFragment->>ChildBinding: Inflate binding (via binding inflation)
BaseFragment->>LoadingBinding: Inflate loading binding
BaseFragment->>BaseFragment: Combine into FrameLayout root
BaseFragment-->>Fragment Lifecycle: Return root View
deactivate BaseFragment
Fragment Lifecycle->>BaseFragment: onViewCreated(view)
BaseFragment->>ConnectFragment: onViewCreated() [child override]
ConnectFragment->>ChildBinding: Initialize UI elements
opt Show loading
ConnectFragment->>BaseFragment: showLoading()
BaseFragment->>LoadingBinding: Set visibility VISIBLE
end
opt Hide loading
ConnectFragment->>BaseFragment: hideLoading()
BaseFragment->>LoadingBinding: Set visibility GONE
end
Fragment Lifecycle->>BaseFragment: onDestroyView()
BaseFragment->>BaseFragment: Cleanup bindings (_loadingBinding = null)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes involve substantial architectural refactoring across multiple layers—introducing new base abstractions ( Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (1)
81-87: Avoid integer overflow when computing money amount.
approved * unit.getAmount()can overflow int. Cast to long before multiplication.- String amount = job.getMoneyString(approved * unit.getAmount()); + long approvedAmount = ((long) approved) * unit.getAmount(); + String amount = job.getMoneyString(approvedAmount);app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (1)
24-29: Missing import breaks compilation.
PersonalIdOrConnectApiErrorCodesis referenced but not imported.import org.commcare.connect.network.connect.ConnectApiHandler; import org.commcare.connect.network.connectId.PersonalIdApiErrorHandler; +import org.commcare.connect.network.connectId.PersonalIdOrConnectApiErrorCodes;Also applies to: 92-99
app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java (1)
59-71: Fix lifecycle race and switch to binding to avoid NPEs and leaks.
- Risk: ResourceEngineListener callbacks can fire after onCreate() but before onViewCreated(), so statusText/progressBar are null when updateInstallStatus/updateResourceProgress run. This will crash.
- Leak: Keeping view fields in fragment outlives the view lifecycle. Use binding accessors instead.
Proposed fix:
- Move startAppDownload() to onViewCreated() after view init.
- Replace direct view fields with getBinding() references.
@@ public void onCreate(Bundle savedInstanceState) { - startAppDownload(); @@ } @@ - private ProgressBar progressBar; - private TextView statusText; + // Use binding views instead of fragment-held references @@ private void updateInstallStatus(String status) { - statusText.setVisibility(status != null ? View.VISIBLE : View.GONE); - statusText.setText(status); + FragmentConnectDownloadingBinding b = getBinding(); + b.connectDownloadingStatus.setVisibility(status != null ? View.VISIBLE : View.GONE); + b.connectDownloadingStatus.setText(status); } @@ public void updateResourceProgress(int done, int pending, int phase) { - progressBar.setMax(pending); - progressBar.setProgress(done); + FragmentConnectDownloadingBinding b = getBinding(); + b.connectDownloadingProgress.setMax(pending); + b.connectDownloadingProgress.setProgress(done); @@ @Override public void onViewCreated(@NonNull View view, @androidx.annotation.Nullable Bundle savedInstanceState) { super.onViewCreated(view, savedInstanceState); ConnectDownloadingFragmentArgs args = ConnectDownloadingFragmentArgs.fromBundle(getArguments()); requireActivity().setTitle(job.getTitle()); - - TextView titleTv = view.findViewById(R.id.connect_downloading_title); - titleTv.setText(args.getTitle()); - - statusText = view.findViewById(R.id.connect_downloading_status); - updateInstallStatus(null); - - progressBar = view.findViewById(R.id.connect_downloading_progress); + getBinding().connectDownloadingTitle.setText(args.getTitle()); + updateInstallStatus(null); + // Start download after views are ready + startAppDownload(); }Based on learnings
Also applies to: 134-137, 160-173, 196-214
app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (1)
126-136: Likely runtime crash: R.id.confirm is treated as TextView here but CardView in another fragment.ConnectResultsSummaryListFragment inflates the same layout and casts R.id.confirm to CardView. One of these will ClassCastException at runtime. Align both fragments to the same view type.
Two safe options:
- If the layout’s confirm is a TextView: change the Summary fragment to use TextView (or View) for confirm.
- If it’s a CardView: change this fragment to use View for confirm and show status text in a separate TextView (e.g., tv_payment_status), mirroring the Summary fragment.
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
75-92: Type mismatch confirmed: ConnectResultsListFragment will crash at runtime.The layout
connect_payment_item.xmldefines@id/confirmasCardView, butConnectResultsListFragment.PaymentViewHolderdeclares it asTextView. The code then callsconfirmText.setText()andconfirmText.setTextColor()(TextView-only methods), which will fail withClassCastExceptionwhenonBindViewHolder()executes.Fix: Change
ConnectResultsListFragment.PaymentViewHolderline 130 from:final TextView nameText, dateText, confirmText;to:
final TextView nameText, dateText; final CardView confirmText;Then update the action wiring to use
confirmText.setOnClickListener()instead of calling TextView methods. Alternatively, ifconfirmTextshould remain as a TextView for displaying text, the layout needs to be corrected.app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (1)
65-76: Guard UI updates against destroyed views in async callbackCallback may fire after onDestroyView(); getBinding() would crash. Guard before touching UI.
Apply:
- ConnectJobHelper.INSTANCE.updateLearningProgress(requireContext(), job, success -> { - if (success && isAdded()) { - updateLearningUI(); - } else if (!success && isAdded()) { - Toast.makeText( - requireContext(), - getString(R.string.connect_fetch_learning_progress_error), - Toast.LENGTH_LONG - ).show(); - } - }); + ConnectJobHelper.INSTANCE.updateLearningProgress(requireContext(), job, success -> { + if (!isAdded() || getView() == null) { + return; + } + if (success) { + updateLearningUI(); + } else { + Toast.makeText( + requireContext(), + getString(R.string.connect_fetch_learning_progress_error), + Toast.LENGTH_LONG + ).show(); + } + });Consider using a safe binding accessor once added (see BaseFragment comment).
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
133-143: Defensive null checks in filteringdelivery.getUnitName() or getStatus() could be null; equalsIgnoreCase would crash.
Apply:
- for (ConnectJobDeliveryRecord delivery : job.getDeliveries()) { - boolean matchesUnit = delivery.getUnitName().equalsIgnoreCase(unitName); - boolean matchesFilter = currentFilter.equals(ALL_IDENTIFIER) || delivery.getStatus().equalsIgnoreCase(currentFilter); + for (ConnectJobDeliveryRecord delivery : job.getDeliveries()) { + String unit = delivery.getUnitName(); + String status = delivery.getStatus(); + boolean matchesUnit = unit != null && unitName != null && unit.equalsIgnoreCase(unitName); + boolean matchesFilter = currentFilter.equals(ALL_IDENTIFIER) + || (status != null && status.equalsIgnoreCase(currentFilter)); if (matchesUnit && matchesFilter) { filteredList.add(delivery); } }app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
100-116: Guard async callback; add user null-check; use NavHostFragment.findNavControllerThe callback lacks lifecycle safety and the user parameter can be null. Apply guards before UI access and handle null user:
@Override public void onSuccess(Boolean success) { reportApiCall(success); + if (!isAdded() || getView() == null) { + return; + } + job.setStatus(ConnectJobRecord.STATUS_LEARNING); ConnectJobUtils.upsertJob(getContext(), job); if (appInstalled) { ConnectAppUtils.INSTANCE.launchApp(requireActivity(), true, job.getLearnAppInfo().getAppId()); } else { String title = getString(R.string.connect_downloading_learn); - Navigation.findNavController(getBinding().connectJobIntroStartButton).navigate( - ConnectJobIntroFragmentDirections. - actionConnectJobIntroFragmentToConnectDownloadingFragment( - title, true)); + androidx.navigation.fragment.NavHostFragment.findNavController(ConnectJobIntroFragment.this) + .navigate(ConnectJobIntroFragmentDirections + .actionConnectJobIntroFragmentToConnectDownloadingFragment(title, true)); } } }.connectStartLearning(requireContext(), user, job.getJobId());Also verify:
ConnectUserDatabaseUtil.getUser()can return null, butconnectStartLearning()expects non-nullableConnectUserRecord. Add null-check before the API call instartLearning():private void startLearning(boolean appInstalled) { ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getContext()); + if (user == null) { + Toast.makeText(requireContext(), "User not available", Toast.LENGTH_SHORT).show(); + return; + } new ConnectApiHandler<Boolean>(){
🧹 Nitpick comments (16)
app/src/org/commcare/interfaces/BaseView.kt (1)
3-6: Declare main-thread contract and document behavior.Codify that loading UI mutations must occur on the UI thread; add brief KDoc.
package org.commcare.interfaces +import androidx.annotation.MainThread + /** - interface BaseView { - fun showLoading() - fun hideLoading() - } + * Contract for views/screens that can show a global loading overlay. + */ +interface BaseView { + @MainThread fun showLoading() + @MainThread fun hideLoading() +}app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (1)
143-150: Unify nullability annotations (prefer androidx).Mixing
org.jetbrains.annotations.*andandroidx.annotation.*is inconsistent. Preferandroidx.annotation.NonNull/Nullableacross Java.- protected @NotNull FragmentConnectProgressDeliveryBinding inflateBinding(@NotNull LayoutInflater inflater, @Nullable ViewGroup container) { + @NonNull + @Override + protected FragmentConnectProgressDeliveryBinding inflateBinding(@NonNull LayoutInflater inflater, @Nullable ViewGroup container) {Also remove JetBrains imports at the top of this file after refactor.
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (2)
127-134: Unify nullability annotations (prefer androidx).Keep Java files consistent with
androidx.annotation.- protected @NotNull FragmentConnectDeliveryDetailsBinding inflateBinding(@NotNull LayoutInflater inflater, @Nullable ViewGroup container) { + @NonNull + @Override + protected FragmentConnectDeliveryDetailsBinding inflateBinding(@NonNull LayoutInflater inflater, @Nullable ViewGroup container) { return FragmentConnectDeliveryDetailsBinding.inflate(inflater, container, false); }Remove
org.jetbrains.annotations.NotNullimport after this change.
81-104: Show/hide loader around network call for better UX.Leverage the new base loader to signal work-in-progress on claim.
private void claimJob(ConnectJobRecord job, boolean appInstalled) { ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getContext()); new ConnectApiHandler<Boolean>() { @Override public void onSuccess(Boolean data) { + hideLoading(); proceedAfterJobClaimed(getBinding().connectDeliveryButton, job, appInstalled); FirebaseAnalyticsUtil.reportCccApiClaimJob(true); } @Override public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) { + hideLoading(); String message; if (errorCode == PersonalIdOrConnectApiErrorCodes.BAD_REQUEST_ERROR) { message = getString(R.string.recovery_unable_to_claim_opportunity); } else { message = PersonalIdApiErrorHandler.handle(requireActivity(), errorCode, t); } showSnackBarWithDismissAction(getBinding().getRoot(), message); FirebaseAnalyticsUtil.reportCccApiClaimJob(false); } - }.claimJob(requireContext(), user, job.getJobId()); + }.claimJob(requireContext(), user, job.getJobId()); + showLoading(); }If callbacks may execute off the main thread, ensure
hideLoading()is dispatched to the UI thread.app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
118-125: Wire only the relevant action (confirm vs revoke).Currently both confirm and revoke listeners are attached whenever enabled, which can present contradictory actions. Attach only the action that matches the current payment state.
- if (enabled) { - setupPaymentAction(holder, payment, amount, true, - R.drawable.ic_connect_payment_status_not_transferred, - R.string.connect_payment_confirm_transferred); - setupPaymentAction(holder, payment, amount, false, - R.drawable.ic_connect_payment_status_transferred, - R.string.connect_payment_revoke_transferred); - } + if (enabled) { + if (!payment.getConfirmed() && payment.allowConfirm()) { + setupPaymentAction(holder, payment, amount, true, + R.drawable.ic_connect_payment_status_not_transferred, + R.string.connect_payment_confirm_transferred); + } else if (payment.getConfirmed() && payment.allowConfirmUndo()) { + setupPaymentAction(holder, payment, amount, false, + R.drawable.ic_connect_payment_status_transferred, + R.string.connect_payment_revoke_transferred); + } + }app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (2)
150-155: Use ContextCompat.getColor (avoid deprecated Resources#getColor).Replace getResources().getColor(...) with ContextCompat.getColor(context, ...).
- confirmText.setTextColor( - context.getResources().getColor(enabled ? R.color.blue : R.color.dark_grey)); + confirmText.setTextColor( + androidx.core.content.ContextCompat.getColor(context, enabled ? R.color.blue : R.color.dark_grey));
94-101: Use payment-specific date format for consistency.Other screens use ConnectDateUtils.paymentDateFormat(...). Consider using the same here.
- holder.dateText.setText(ConnectDateUtils.INSTANCE.formatDate(payment.getDate())); + holder.dateText.setText(ConnectDateUtils.INSTANCE.paymentDateFormat(payment.getDate()));app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (2)
68-79: Unregister ViewPager2 page change callback to avoid leaks.Keep a field for the callback and unregister it in onDestroyView().
- getBinding().connectDeliveryProgressViewPager.registerOnPageChangeCallback(new ViewPager2.OnPageChangeCallback() { + // Keep a field to unregister later + if (pageChangeCallback != null) { + getBinding().connectDeliveryProgressViewPager.unregisterOnPageChangeCallback(pageChangeCallback); + } + pageChangeCallback = new ViewPager2.OnPageChangeCallback() { @Override public void onPageSelected(int position) { if (!isProgrammaticTabChange) { tabLayout.selectTab(tabLayout.getTabAt(position)); FirebaseAnalyticsUtil.reportConnectTabChange(tabLayout.getTabAt(position).getText().toString()); } else { isProgrammaticTabChange = false; } } - }); + }; + getBinding().connectDeliveryProgressViewPager.registerOnPageChangeCallback(pageChangeCallback);And add:
@@ public class ConnectDeliveryProgressFragment extends ConnectJobFragment<FragmentConnectDeliveryProgressBinding> @@ - private boolean isProgrammaticTabChange = false; + private boolean isProgrammaticTabChange = false; + private ViewPager2.OnPageChangeCallback pageChangeCallback; @@ @Override public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { @@ } + + @Override + public void onDestroyView() { + if (pageChangeCallback != null) { + getBinding().connectDeliveryProgressViewPager.unregisterOnPageChangeCallback(pageChangeCallback); + pageChangeCallback = null; + } + super.onDestroyView(); + }
171-182: Use centralized money formatting for the confirmation tile.Build the amount using job.getMoneyString(...) to keep formatting consistent with other screens.
- String date = ConnectDateUtils.INSTANCE.formatDate(paymentToConfirm.getDate()); - getBinding().connectPaymentConfirmLabel.setText(getString( - R.string.connect_payment_confirm_text, - paymentToConfirm.getAmount(), - job.getCurrency(), - date)); + String date = ConnectDateUtils.INSTANCE.formatDate(paymentToConfirm.getDate()); + String amount = job.getMoneyString(Integer.parseInt(paymentToConfirm.getAmount())); + getBinding().connectPaymentConfirmLabel.setText(getString( + R.string.connect_payment_confirm_text, + amount, + "", // currency included in amount if getMoneyString handles it + date));If getMoneyString does not include currency, pass currency as before and skip Integer.parseInt if amounts can be non-integer.
app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (2)
97-100: Clamp progress to [0,100] before settingAvoid invalid values propagating to the ProgressBar.
Apply:
- if (!hideProgress) { - getBinding().connectLearningProgressBar.setProgress(percent); - getBinding().connectLearningProgressText.setText(String.format(Locale.getDefault(), "%d%%", percent)); - } + if (!hideProgress) { + int safe = Math.max(0, Math.min(100, percent)); + getBinding().connectLearningProgressBar.setProgress(safe); + getBinding().connectLearningProgressText.setText( + String.format(Locale.getDefault(), "%d%%", safe)); + }
129-134: Null-check learnings listjob.getLearnings() may be null; loop would NPE.
Apply:
- } else { - for (ConnectJobLearningRecord l : job.getLearnings()) { + } else { + List<ConnectJobLearningRecord> learnings = job.getLearnings(); + if (learnings != null) { + for (ConnectJobLearningRecord l : learnings) { if (latestDate == null || latestDate.before(l.getDate())) { latestDate = l.getDate(); } - } + } + }app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (2)
229-245: Null-safe status handlingstatus.toLowerCase(...) will NPE if status is null.
Apply:
- private void updateStatusUI(Context context, String status) { + private void updateStatusUI(Context context, String status) { int bgResId, iconResId; - switch (status.toLowerCase(Locale.ENGLISH)) { + String s = status == null ? "" : status.toLowerCase(Locale.ENGLISH); + switch (s) { case APPROVED_IDENTIFIER :
186-189: Prefer DiffUtil for smoother list updatesnotifyDataSetChanged() causes full refresh and jank. Use DiffUtil to compute minimal changes.
If acceptable, I can provide a small DiffUtil.ItemCallback and ListAdapter patch.
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
65-67: Line breaks: prefer "\n\n" on Android“\r\n\r\n” is unnecessary; use “\n\n” or System.lineSeparator() consistently.
- return modules.isEmpty() ? getString(R.string.connect_job_no_learning_required) : - TextUtils.join("\r\n\r\n", lines); + return modules.isEmpty() ? getString(R.string.connect_job_no_learning_required) + : TextUtils.join("\n\n", lines);app/src/org/commcare/fragments/BaseFragment.kt (2)
49-51: Block touch-through when loader is visibleEnsure loading overlay consumes touches and focus.
Apply:
- // Hide loading by default - _loadingBinding?.root?.visibility = View.GONE + // Hide loading by default and ensure overlay consumes input when shown + _loadingBinding?.root?.apply { + visibility = View.GONE + isClickable = true + isFocusable = true + }
62-69: Optional helper to reduce boilerplateProvide withBinding helper to avoid null checks at call sites.
Apply:
override fun hideLoading() { _loadingBinding?.root?.visibility = View.GONE } + + // Convenience: run only if binding is active + protected inline fun withBinding(block: (B) -> Unit) { + _binding?.let(block) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/res/layout/loading.xml(1 hunks)app/src/org/commcare/fragments/BaseFragment.kt(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(5 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java(5 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(9 hunks)app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectJobFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(5 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(7 hunks)app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java(7 hunks)app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java(1 hunks)app/src/org/commcare/interfaces/BaseView.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
📚 Learning: 2025-03-10T08:16:59.436Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/BaseFragment.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.javaapp/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java
📚 Learning: 2025-07-29T14:11:36.386Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:10:58.243Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java
🧬 Code graph analysis (10)
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (3)
app/src/org/commcare/connect/ConnectDateUtils.kt (1)
formatDate(17-21)app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)app/src/org/commcare/AppUtils.java (1)
AppUtils(34-209)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (4)
app/src/org/commcare/fragments/RefreshableFragment.kt (2)
refresh(3-5)refresh(4-4)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(43-608)app/src/org/commcare/connect/ConnectDateUtils.kt (2)
formatDate(17-21)formatDateTime(31-33)app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (4)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
ConnectUserDatabaseUtil(10-56)app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(157-1279)app/src/org/commcare/connect/ConnectAppUtils.kt (1)
launchApp(117-125)app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
app/src/org/commcare/fragments/connect/ConnectResultsListFragment.java (1)
app/src/org/commcare/fragments/BaseFragment.kt (1)
inflateBinding(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (7)
app/src/org/commcare/fragments/connect/ConnectJobFragment.java (1)
15-23: Generic base migration verified and complete.All nine subclasses properly extend
ConnectJobFragment<T>with concrete binding types. No classes remain on the old pattern. TheObjects.requireNonNull(job)call provides appropriate fail-fast behavior.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (1)
66-71: The review comment's premise is incorrect.With
coreLibraryDesugaringenabled andminSdkVersionset to 23,Map#getOrDefault(API 24) is automatically backported to pre-24 devices via desugaring. There is no pre-N crash risk. The@RequiresApi(N)annotations are redundant given this configuration, but they do not cause crashes.Removing
@RequiresApi(N)from lines 66 and 147 is a valid cleanup, but the suggested diff to replacegetOrDefault()withcontainsKey()/get()is unnecessary and reduces code clarity.Likely an incorrect or invalid review comment.
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
205-207: Confirm intended sort order (ascending vs. most‑recent first).Currently sorts ascending by lastAccessed. If you want most recent first in the lists, reverse the comparator.
- Collections.sort(list, (job1, job2) -> job1.getLastAccessed().compareTo(job2.getLastAccessed())); + Collections.sort(list, (job1, job2) -> job2.getLastAccessed().compareTo(job1.getLastAccessed()));app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (1)
251-267: Migration to generic Base/ConnectJobFragment looks goodinflateBinding/onViewCreated wiring is correct; title/args initialization moved appropriately.
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
145-158: Binding migration looks correctinflateBinding/onViewCreated wiring and initial setup are solid.
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
128-155: Binding migration LGTMinflateBinding/onViewCreated conversion and UI wiring look correct.
app/src/org/commcare/fragments/BaseFragment.kt (1)
56-61: Good: binding cleanup in onDestroyView_nulling bindings prevents leaks; aligns with past guidance.
Based on learnings
app/res/layout/loading.xml
Outdated
| <FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:id="@+id/loadingContainer" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:gravity="center" | ||
| android:visibility="gone"> | ||
|
|
||
| <ProgressBar | ||
| android:id="@+id/progressBar" | ||
| android:gravity="center" | ||
| android:layout_gravity="center" | ||
| style="?android:attr/progressBarStyleLarge" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make loader intercept touches and be accessible; remove redundant gravity.
Ensure the overlay blocks interaction and is announced correctly. Also, android:gravity on ProgressBar is unnecessary.
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:id="@+id/loadingContainer"
android:layout_width="match_parent"
android:layout_height="match_parent"
- android:gravity="center"
- android:visibility="gone">
+ android:gravity="center"
+ android:visibility="gone"
+ android:clickable="true"
+ android:focusable="true"
+ android:importantForAccessibility="noHideDescendants"
+ android:elevation="8dp"
+ android:background="#80000000" <!-- optional scrim; adjust to theme -->
+ android:contentDescription="@string/loading">
<ProgressBar
android:id="@+id/progressBar"
- android:gravity="center"
android:layout_gravity="center"
style="?android:attr/progressBarStyleLarge"
android:layout_width="wrap_content"
android:layout_height="wrap_content" />
</FrameLayout>Optional: consider com.google.android.material.progressindicator.CircularProgressIndicator for Material styling if the project already depends on Material Components.
🤖 Prompt for AI Agents
In app/res/layout/loading.xml around lines 2 to 15, the overlay should block
touch events and be accessible and the ProgressBar's android:gravity is
redundant; make the FrameLayout intercept touches by setting it clickable="true"
and focusable="true" (so it consumes input), add an accessibility hint such as
android:importantForAccessibility="yes" and either a contentDescription on the
FrameLayout or on the ProgressBar (e.g., a localized "Loading" string) and/or
android:accessibilityLiveRegion="polite" so screen readers announce the loading
state; remove the unnecessary android:gravity attribute from the ProgressBar;
optionally swap to Material's CircularProgressIndicator if the project uses
Material Components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypanchal-13 the suggestion here seems something we should so specifically with gravity , contentDescription , importantForAccessibility and elevation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private var _binding: B? = null | ||
| protected val binding get() = _binding!! | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Expose safe binding access to avoid NPEs in async paths
Protected val binding uses “!!”. Provide nullable getter for Java/Kotlin callers.
Apply:
- private var _binding: B? = null
- protected val binding get() = _binding!!
+ private var _binding: B? = null
+ // Non-null accessor for use strictly within view lifecycle
+ protected val binding: B get() = _binding!!
+ // Nullable accessor for safe checks from callbacks
+ protected val bindingOrNull: B? get() = _binding📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var _binding: B? = null | |
| protected val binding get() = _binding!! | |
| private var _binding: B? = null | |
| // Non-null accessor for use strictly within view lifecycle | |
| protected val binding: B get() = _binding!! | |
| // Nullable accessor for safe checks from callbacks | |
| protected val bindingOrNull: B? get() = _binding | |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/BaseFragment.kt around lines 15-17, the
protected binding currently force-unwraps _binding with "!!" which can cause
NPEs in async paths; expose a nullable getter for callers instead. Replace or
supplement the non-null accessor with a nullable one (e.g., protected val
bindingOrNull: B? get() = _binding) so Java/Kotlin callers can safely check
null; keep any internal non-null helper (like requireBinding()) if you still
need a throw-on-null behavior, but remove uses of "!!" in public/protected APIs.
| ConnectDeliveryDetailsFragmentArgs args = ConnectDeliveryDetailsFragmentArgs.fromBundle(getArguments()); | ||
|
|
||
| requireActivity().setTitle(getString(R.string.connect_job_info_title)); | ||
| setupJobDetailsUI(job); | ||
| setupButtonBehavior(job, args.getIsButtonVisible()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use requireArguments() to avoid NPE.
fromBundle(getArguments()) will NPE if args are absent. requireArguments() is safer.
- ConnectDeliveryDetailsFragmentArgs args = ConnectDeliveryDetailsFragmentArgs.fromBundle(getArguments());
+ ConnectDeliveryDetailsFragmentArgs args =
+ ConnectDeliveryDetailsFragmentArgs.fromBundle(requireArguments());🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
around lines 135 to 140, the call to
ConnectDeliveryDetailsFragmentArgs.fromBundle(getArguments()) can throw an NPE
if arguments are missing; replace getArguments() with requireArguments() so the
fragment explicitly fails fast with a clear exception when arguments are absent
and avoids a potential null pointer later. Ensure you update the call to use
requireArguments() and run tests/flows that navigate to this fragment to confirm
behavior.
| int percent = total > 0 ? (100 * completed / total) : 100; | ||
|
|
||
| CircleProgressBar progress = binding.connectProgressProgressBar; | ||
| CircleProgressBar progress = getBinding().connectProgressProgressBar; | ||
| progress.setProgress(percent); | ||
| binding.connectProgressProgressText.setText(String.format(Locale.getDefault(), "%d%%", percent)); | ||
| getBinding().connectProgressProgressText.setText(String.format(Locale.getDefault(), "%d%%", percent)); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clamp computed values to safe ranges; avoid negative remaining and >100%.
Guard against data inconsistencies and division rounding quirks.
- int percent = total > 0 ? (100 * completed / total) : 100;
+ int percent = total > 0 ? (100 * completed / total) : 100;
+ if (percent > 100) percent = 100;
@@
- int remaining = unit.getMaxTotal() - approved;
+ int remaining = Math.max(0, unit.getMaxTotal() - approved);Also applies to: 80-83
🤖 Prompt for AI Agents
In
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java
around lines 44-49 (and also apply same change to lines 80-83): the computed
percent and any remaining/derived values must be clamped to the 0–100 range to
avoid negative values or >100 due to division/rounding errors; after computing
percent (and any remaining), bound it with Math.max(0, Math.min(100, percent))
before passing to setProgress and formatting the text, and ensure any remaining
or completed counts shown are similarly clamped to valid ranges to prevent
UI/logic errors.
| getBinding().btnSync.setOnClickListener(v -> { | ||
| ConnectDeliveryProgressFragment parentFragment = (ConnectDeliveryProgressFragment)getParentFragment(); | ||
| if (parentFragment != null) { | ||
| parentFragment.refresh(); | ||
| } | ||
| populateDeliveryProgress(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid ClassCastException when accessing parent fragment.
Casting before the null/type check can crash if the parent is not ConnectDeliveryProgressFragment.
- getBinding().btnSync.setOnClickListener(v -> {
- ConnectDeliveryProgressFragment parentFragment = (ConnectDeliveryProgressFragment)getParentFragment();
- if (parentFragment != null) {
- parentFragment.refresh();
- }
+ getBinding().btnSync.setOnClickListener(v -> {
+ Fragment parent = getParentFragment();
+ if (parent instanceof ConnectDeliveryProgressFragment) {
+ ((ConnectDeliveryProgressFragment) parent).refresh();
+ }
populateDeliveryProgress();
});🤖 Prompt for AI Agents
In
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java
around lines 151 to 157, avoid the ClassCastException caused by casting
getParentFragment() before verifying its type: first assign getParentFragment()
to a Fragment variable, check that it is non-null and instanceof
ConnectDeliveryProgressFragment, then cast and call refresh(); leave
populateDeliveryProgress() as-is so it always runs after the safe parent refresh
attempt.
| getBinding().connectLearningCertificateContainer.setVisibility( | ||
| complete && passed ? View.VISIBLE : View.GONE); | ||
|
|
||
| if (complete && passed) { | ||
| viewBinding.connectLearnCertSubject.setText(job.getTitle()); | ||
| viewBinding.connectLearnCertPerson.setText(ConnectUserDatabaseUtil.getUser(requireContext()).getName()); | ||
| getBinding().connectLearnCertSubject.setText(job.getTitle()); | ||
| getBinding().connectLearnCertPerson.setText(ConnectUserDatabaseUtil.getUser(requireContext()).getName()); | ||
|
|
||
| Date latestDate = getLatestCompletionDate(job); | ||
| viewBinding.connectLearnCertDate.setText( | ||
| getBinding().connectLearnCertDate.setText( | ||
| getString(R.string.connect_learn_completed, | ||
| ConnectDateUtils.INSTANCE.formatDate(latestDate))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null-safety: user may be absent in local DB
ConnectUserDatabaseUtil.getUser(...) can return null; current code dereferences getName() and will crash.
Apply:
- getBinding().connectLearningCertificateContainer.setVisibility(
- complete && passed ? View.VISIBLE : View.GONE);
+ getBinding().connectLearningCertificateContainer.setVisibility(
+ complete && passed ? View.VISIBLE : View.GONE);
if (complete && passed) {
- getBinding().connectLearnCertSubject.setText(job.getTitle());
- getBinding().connectLearnCertPerson.setText(ConnectUserDatabaseUtil.getUser(requireContext()).getName());
+ // Hide certificate if user is unavailable instead of crashing
+ final var user = ConnectUserDatabaseUtil.getUser(requireContext());
+ if (user == null) {
+ getBinding().connectLearningCertificateContainer.setVisibility(View.GONE);
+ return;
+ }
+ getBinding().connectLearnCertSubject.setText(job.getTitle());
+ getBinding().connectLearnCertPerson.setText(user.getName());Based on learnings.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java
around lines 104 to 115, the call to
ConnectUserDatabaseUtil.getUser(requireContext()).getName() can NPE because
getUser(...) may return null; update the code to retrieve the user into a local
variable, null-check it (or use Optional-like handling), and use a safe fallback
(e.g., empty string or a localized "Unknown" label) when setting
connectLearnCertPerson; ensure the visibility and date-setting logic remain
unchanged and avoid dereferencing user when null.
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to capture a screenshot of what the wait dialog looks like when visible?
| @@ -0,0 +1,6 @@ | |||
| package org.commcare.interfaces | |||
|
|
|||
| interface BaseView { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I wonder if we could name this something a bit more relevant to its purpose, like BlockableView or WaitableView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Since we are already touching a lot of Java code, what if we converted these files to Kotlin?
This comment goes for all the other Java files in this PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@conroy-ricketts Good suggestions although don't think we have talked about converting the existing Java code to Kotling as part of functional PRs. Few thoughts on this -
- We have mixed confidence on team with Kotlin vs Java and we are still in the phase where we are trying to build the Kotlin competency of team by writing new code in Kotlin.
- I do think it's a good one for us to do for Connect speicifc code but we probably need a separate tech-debt effort to do so rather than making it part of functional PRs to facilititate easy change management and reviews on PRs.
- We have also struggled to quantatize benefits of converting existing Java code to Kotlin (other than ease of development) so that it makes sense for the product management team to put Sprint time into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1g5 Makes sense! I appreciate you sharing your thoughts on this. I really like the idea of making this a separate tech-debt effort because of the reasons you described. I also think it would be good to convert our existing code to Kotlin at some point in the future if we ever want to convert our UI to Jetpack Compose and reap the full benefit of that - I think that may be the best reason to pitch to the Product Management team. All good stuff to think about!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrangeAndGreen I'm really curious what your thoughts are here?
| TextView titleTv = view.findViewById(R.id.connect_downloading_title); | ||
| titleTv.setText(args.getTitle()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Suggest renaming this variable to match the conventions in this class for consistency (e.g. statusText)
| TextView titleTv = view.findViewById(R.id.connect_downloading_title); | |
| titleTv.setText(args.getTitle()); | |
| TextView titleText = view.findViewById(R.id.connect_downloading_title); | |
| titleText.setText(args.getTitle()); |
| protected val binding get() = _binding!! | ||
|
|
||
| private var _loadingBinding: LoadingBinding? = null | ||
| private lateinit var rootView: View |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypanchal-13 Please remove this
| addView(mainView) | ||
| addView(loadingView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypanchal-13 This will add loading view on top of main view? I am not sure, can we check that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jignesh-dimagi Yes. Do we need to add loader back of main view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need loader view on the top of main view so that its always seen irrespective of main view
| private var _binding: B? = null | ||
| protected val binding get() = _binding!! | ||
|
|
||
| private var _loadingBinding: LoadingBinding? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypanchal-13 Change this to lateinit to make sure that loading is alway present
| @@ -0,0 +1,69 @@ | |||
| package org.commcare.fragments | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create package base and create inside it
| @@ -0,0 +1,6 @@ | |||
| package org.commcare.interfaces | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create package base and create inside it
| } | ||
|
|
||
| // Hide loading by default | ||
| _loadingBinding?.root?.visibility = View.GONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call hideLoading()
…698-custom-wait-pb
…698-custom-wait-pb # Conflicts: # app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java
…698-custom-wait-pb
…698-custom-wait-pb
|
| @@ -0,0 +1,7 @@ | |||
| package org.commcare.interfaces.base | |||
|
|
|||
| interface WaitableView { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be BaseView only as it can more functionality in future along with loading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we should be calling these BaseConnectView and BaseConnectFragment as these are only applicable to connect screens
| } | ||
|
|
||
| @Override | ||
| public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we moving this logic to onViewCreated from onCreateView ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already using onCreateView in BaseFragment. So moved logic to onViewCreated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- we can still override
onCreateViewand call super.onCreateView - Also the order of the methods should be retained as original as it's causing un-neecssary diff in the PR.
|
|
||
| private void setWaitDialogEnabled(boolean enabled) { | ||
| Activity activity = getActivity(); | ||
| if(activity instanceof ConnectActivity connectActivity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is always true and we should not need to safe-check this
shubham1g5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are still a bunch of fragments that are using onViewCreated instead of onCreateView
app/res/layout/loading.xml
Outdated
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="center" | ||
| android:gravity="center" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't think we need to specify gravity again given it's specified in parent
| } | ||
|
|
||
| @Override | ||
| public void onViewCreated(@NonNull View view, @androidx.annotation.Nullable Bundle savedInstanceState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still pending.
shubham1g5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good, one fine change.
| @Override | ||
| public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container, | ||
| Bundle savedInstanceState) { | ||
| public @NotNull View onCreateView(@NotNull LayoutInflater inflater, @org.jetbrains.annotations.Nullable ViewGroup container, @org.jetbrains.annotations.Nullable Bundle savedInstanceState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullable annotations here should be from javax.annotation.Nullable
| public void onDestroyView() { | ||
| super.onDestroyView(); | ||
| binding = null; | ||
| protected @NotNull FragmentConnectJobIntroBinding inflateBinding(@NotNull LayoutInflater inflater, @org.jetbrains.annotations.Nullable ViewGroup container) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct annotation import
|
@jaypanchal-13 Can you also put out a demo for this on slack #connect-tech-demos |
Product Description
Ticket-> https://dimagi.atlassian.net/browse/CCCT-1698
Demo->
Screen_recording_20251031_180451.mp4
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review