Restore Connect buttons in beta #3189
Conversation
📝 WalkthroughWalkthroughThe changes update the logic controlling the visibility and enabled state of the "connect" button in two areas of the application. In Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivityUIController
participant PersonalIdManager
participant SelectInstallModeFragment
User->>LoginActivityUIController: Triggers refreshConnectView()
LoginActivityUIController->>PersonalIdManager: Check logged-in status
PersonalIdManager-->>LoginActivityUIController: Returns status
LoginActivityUIController->>LoginActivityUIController: setConnectButtonVisible(true/false)
User->>SelectInstallModeFragment: Triggers updateConnectButton(connectEnabled, listener)
SelectInstallModeFragment->>PersonalIdManager: Check logged-in status
PersonalIdManager-->>SelectInstallModeFragment: Returns status
SelectInstallModeFragment->>SelectInstallModeFragment: Enable/disable connect button, set listener, update visibility
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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/SelectInstallModeFragment.java (1)
213-221: Click-listener is never cleared when the button is disabled
updateConnectButtonassigns the listener only whenenabled == true, but it never resets/clears the listener whenenabled == false.
If this method is called repeatedly (e.g. the user logs out → logs in again), an old listener might survive an intermediate disabled state, leading to:• Potential double invocation
• Memory-leak risks via captured outer referencesMinimal fix:
if (enabled && listener != null) { mConnectButton.setOnClickListener(listener); +} else { + mConnectButton.setOnClickListener(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/activities/LoginActivityUIController.java(1 hunks)app/src/org/commcare/fragments/SelectInstallModeFragment.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/SelectInstallModeFragment.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(65-676)
🔇 Additional comments (1)
app/src/org/commcare/activities/LoginActivityUIController.java (1)
565-574: Visibility now tied to “logged-in” state – confirm UX requirement
setConnectButtonVisible(true)is only executed whenPersonalIdManager.getInstance().isloggedIn()istrue; the button remains hidden otherwise.
If the intent is for the button to start the Connect sign-in flow, users who are not logged in will never see it.
- Confirm that Connect sign-in is triggered elsewhere (e.g. a separate screen or dialog) before this Activity is reached.
- If the button should initiate sign-in when the user is not logged in, drop the
isloggedIn()guard or add a second branch that shows the button with the “Sign in with Connect” text.Example patch if the latter is desired:
if (PersonalIdManager.getInstance().isloggedIn()) { connectLoginButton.setText(activity.getString(R.string.connect_button_logged_in)); - setConnectButtonVisible(true); + setConnectButtonVisible(true); } else { - setConnectButtonVisible(false); + connectLoginButton.setText(activity.getString(R.string.connect_button_sign_in)); + setConnectButtonVisible(true); }
|
this is already fixed in |
The Connect buttons (in Setup and Login pages) was hard-coded to be invisible.
This is what we want on master, but beta needs the button to behave as usual.