From fe3c163a2078de93f1de3372d2dad6458e850b89 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Mon, 3 Jun 2019 18:58:39 -0700 Subject: [PATCH] Fix how we reflect FxA state in preference This cleans up how we're displaying account state in the main preference UI. Before when it worked, it worked mostly accidentally. 'launch' wrapper around "update ui" methods would trigger a race condition between binding the account pref view holder and actually updating that view with values. Sometimes the "update view with values" would happen after view was bound, and the UI will be correct. Most of the time it would happen before, and so there will be nothing to update and we'd get into an inconsistent state. This also splits up the "accountpreference" into two: account is good, and account needs re-auth. This greatly simplifies their management. --- .../components/toolbar/DefaultToolbarMenu.kt | 2 +- .../settings/AccountAuthErrorPreference.kt | 35 ++++ .../fenix/settings/AccountPreference.kt | 31 +-- .../fenix/settings/SettingsFragment.kt | 188 +++++++----------- .../java/org/mozilla/fenix/utils/Settings.kt | 9 - .../layout/account_auth_error_preference.xml | 67 +++++++ .../main/res/layout/account_preference.xml | 22 +- app/src/main/res/values/colors.xml | 2 +- app/src/main/res/values/preference_keys.xml | 1 + app/src/main/res/xml/preferences.xml | 8 +- 10 files changed, 202 insertions(+), 163 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/settings/AccountAuthErrorPreference.kt create mode 100644 app/src/main/res/layout/account_auth_error_preference.xml diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt index 0d057d508..4cb7f99e3 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt @@ -116,7 +116,7 @@ class DefaultToolbarMenu( highlight = if (hasAccountProblem) { BrowserMenuHighlightableItem.Highlight( imageResource = R.drawable.ic_alert, - backgroundResource = R.color.sync_error_color + backgroundResource = R.color.sync_error_background_color ) } else null ) { diff --git a/app/src/main/java/org/mozilla/fenix/settings/AccountAuthErrorPreference.kt b/app/src/main/java/org/mozilla/fenix/settings/AccountAuthErrorPreference.kt new file mode 100644 index 000000000..dca3e3df0 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/settings/AccountAuthErrorPreference.kt @@ -0,0 +1,35 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.settings + +import android.content.Context +import android.util.AttributeSet +import android.view.View +import android.widget.TextView +import androidx.preference.Preference +import androidx.preference.PreferenceViewHolder +import org.mozilla.fenix.R + +class AccountAuthErrorPreference : Preference { + var email: String? = null + + constructor(context: Context) : super(context) + constructor(context: Context, attrs: AttributeSet?) : super(context, attrs) + constructor(context: Context, attrs: AttributeSet?, attributeSetId: Int) : super(context, attrs, attributeSetId) + + init { + layoutResource = R.layout.account_auth_error_preference + } + + override fun onBindViewHolder(holder: PreferenceViewHolder) { + super.onBindViewHolder(holder) + val emailView = holder.findViewById(R.id.email) as TextView + emailView.text = email.orEmpty() + emailView.visibility = when (email.isNullOrEmpty()) { + true -> View.GONE + false -> View.VISIBLE + } + } +} diff --git a/app/src/main/java/org/mozilla/fenix/settings/AccountPreference.kt b/app/src/main/java/org/mozilla/fenix/settings/AccountPreference.kt index 88bacf8c5..34e77f549 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/AccountPreference.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/AccountPreference.kt @@ -7,26 +7,18 @@ package org.mozilla.fenix.settings import android.content.Context import android.util.AttributeSet import android.view.View -import android.widget.ImageView import android.widget.TextView import androidx.preference.Preference import androidx.preference.PreferenceViewHolder import org.mozilla.fenix.R class AccountPreference : Preference { - - var title: TextView? = null - var summary: TextView? = null - var errorIcon: ImageView? = null - var background: View? = null + var displayName: String? = null + var email: String? = null constructor(context: Context) : super(context) constructor(context: Context, attrs: AttributeSet?) : super(context, attrs) - constructor(context: Context, attrs: AttributeSet?, attributeSetId: Int) : super( - context, - attrs, - attributeSetId - ) + constructor(context: Context, attrs: AttributeSet?, attributeSetId: Int) : super(context, attrs, attributeSetId) init { layoutResource = R.layout.account_preference @@ -34,9 +26,18 @@ class AccountPreference : Preference { override fun onBindViewHolder(holder: PreferenceViewHolder) { super.onBindViewHolder(holder) - title = holder.findViewById(R.id.title) as TextView - summary = holder.findViewById(R.id.summary) as TextView - errorIcon = holder.findViewById(R.id.error_icon) as ImageView - background = holder.itemView + val displayNameView = holder.findViewById(R.id.displayName) as TextView + val emailView = holder.findViewById(R.id.email) as TextView + + displayNameView.text = displayName.orEmpty() + displayNameView.visibility = when (displayName.isNullOrEmpty()) { + true -> View.GONE + false -> View.VISIBLE + } + // There is a potential for a race condition here. We might not have the user profile by the time we display + // this field, in which case we won't have the email address (or the display name, but that we may just not have + // at all even after fetching the profile). We don't hide the email field or change its text if email is missing + // because in the layout a default value ("Firefox Account") is specified, which will be displayed instead. + email?.let { emailView.text = it } } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt index 6bf346cb9..81f2832bb 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt @@ -5,15 +5,14 @@ package org.mozilla.fenix.settings import android.content.ActivityNotFoundException +import android.content.Context import android.content.Intent import android.content.SharedPreferences import android.net.Uri import android.os.Bundle import android.provider.Settings -import android.view.View import android.widget.Toast import androidx.appcompat.app.AppCompatActivity -import androidx.core.content.ContextCompat import androidx.navigation.Navigation import androidx.preference.Preference import androidx.preference.Preference.OnPreferenceClickListener @@ -21,12 +20,10 @@ import androidx.preference.PreferenceCategory import androidx.preference.PreferenceFragmentCompat import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.Job import kotlinx.coroutines.launch import mozilla.components.concept.sync.AccountObserver import mozilla.components.concept.sync.OAuthAccount import mozilla.components.concept.sync.Profile -import mozilla.components.service.fxa.FxaUnauthorizedException import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.Config import org.mozilla.fenix.FenixApplication @@ -35,6 +32,7 @@ import org.mozilla.fenix.R import org.mozilla.fenix.R.string.pref_key_about import org.mozilla.fenix.R.string.pref_key_accessibility import org.mozilla.fenix.R.string.pref_key_account +import org.mozilla.fenix.R.string.pref_key_account_auth_error import org.mozilla.fenix.R.string.pref_key_account_category import org.mozilla.fenix.R.string.pref_key_data_choices import org.mozilla.fenix.R.string.pref_key_delete_browsing_data @@ -53,18 +51,12 @@ import org.mozilla.fenix.R.string.pref_key_tracking_protection_settings import org.mozilla.fenix.R.string.pref_key_your_rights import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.getColorFromAttr import org.mozilla.fenix.ext.getPreferenceKey import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.utils.ItsNotBrokenSnack -import kotlin.coroutines.CoroutineContext @SuppressWarnings("TooManyFunctions", "LargeClass") -class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObserver { - private lateinit var job: Job - override val coroutineContext: CoroutineContext - get() = Dispatchers.Main + job - +class SettingsFragment : PreferenceFragmentCompat(), AccountObserver { private val preferenceChangeListener = SharedPreferences.OnSharedPreferenceChangeListener { sharedPreferences, key -> try { @@ -83,10 +75,15 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) - job = Job() - setupAccountUI() - updateSignInVisibility() - displayAccountErrorIfNecessary() + + // Observe account changes to keep the UI up-to-date. + requireComponents.backgroundServices.accountManager.register(this, owner = this, autoPause = true) + + // It's important to update the account UI state in onCreate, even though we also call it in onResume, since + // that ensures we'll never display an incorrect state in the UI. For example, if user is signed-in, and we + // don't perform this call in onCreate, we'll briefly display a "Sign In" preference, which will then get + // replaced by the correct account information once this call is ran in onResume shortly after. + updateAccountUIState(context!!, requireComponents.backgroundServices.accountManager.accountProfile()) preferenceManager.sharedPreferences.registerOnSharedPreferenceChangeListener(preferenceChangeListener) } @@ -131,9 +128,8 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse aboutPreference?.title = getString(R.string.preferences_about, appName) setupPreferences() - setupAccountUI() - updateSignInVisibility() - displayAccountErrorIfNecessary() + + updateAccountUIState(context!!, requireComponents.backgroundServices.accountManager.accountProfile()) } @Suppress("ComplexMethod") @@ -182,11 +178,10 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse navigateToAbout() } resources.getString(pref_key_account) -> { - if (requireComponents.backgroundServices.accountManager.accountNeedsReauth()) { - navigateToAccountProblem() - } else { - navigateToAccountSettings() - } + navigateToAccountSettings() + } + resources.getString(pref_key_account_auth_error) -> { + navigateToAccountProblem() } resources.getString(pref_key_delete_browsing_data) -> { navigateToDeleteBrowsingData() @@ -215,19 +210,9 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse override fun onDestroy() { super.onDestroy() - job.cancel() preferenceManager.sharedPreferences.unregisterOnSharedPreferenceChangeListener(preferenceChangeListener) } - private fun setupAccountUI() { - val accountManager = requireComponents.backgroundServices.accountManager - // Observe account changes to keep the UI up-to-date. - accountManager.register(this, owner = this) - - updateAuthState(accountManager.authenticatedAccount()) - accountManager.accountProfile()?.let { updateAccountProfile(it) } - } - private fun getClickListenerForSignIn(): OnPreferenceClickListener { return OnPreferenceClickListener { val directions = SettingsFragmentDirections.actionSettingsFragmentToTurnOnSyncFragment() @@ -335,116 +320,85 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse } override fun onAuthenticated(account: OAuthAccount) { - updateAuthState(account) - updateSignInVisibility() - } - - override fun onError(error: Exception) { - // TODO we could display some error states in this UI. - when (error) { - is FxaUnauthorizedException -> { + CoroutineScope(Dispatchers.Main).launch { + context?.let { + updateAccountUIState(it, it.components.backgroundServices.accountManager.accountProfile()) } } } + override fun onError(error: Exception) {} + override fun onLoggedOut() { - updateAuthState() - updateSignInVisibility() + CoroutineScope(Dispatchers.Main).launch { + context?.let { + updateAccountUIState(it, it.components.backgroundServices.accountManager.accountProfile()) + } + } } override fun onProfileUpdated(profile: Profile) { - updateAccountProfile(profile) + CoroutineScope(Dispatchers.Main).launch { + context?.let { + updateAccountUIState(it, profile) + } + } } override fun onAuthenticationProblems() { - displayAccountErrorIfNecessary() + CoroutineScope(Dispatchers.Main).launch { + context?.let { + updateAccountUIState(it, it.components.backgroundServices.accountManager.accountProfile()) + } + } } - // --- Account UI helpers --- - private fun updateAuthState(account: OAuthAccount? = null) { - // Cache the user's auth state to improve performance of sign in visibility - org.mozilla.fenix.utils.Settings.getInstance(context!!).setHasCachedAccount(account != null) - } - - private fun updateSignInVisibility() { - val hasCachedAccount = org.mozilla.fenix.utils.Settings.getInstance(context!!).hasCachedAccount + /** + * Updates the UI to reflect current account state. + * Possible conditions are logged-in without problems, logged-out, and logged-in but needs to re-authenticate. + */ + private fun updateAccountUIState(context: Context, profile: Profile?) { val preferenceSignIn = - findPreference(context!!.getPreferenceKey(pref_key_sign_in)) + findPreference(context.getPreferenceKey(pref_key_sign_in)) val preferenceFirefoxAccount = - findPreference(context!!.getPreferenceKey(pref_key_account)) + findPreference(context.getPreferenceKey(pref_key_account)) + val preferenceFirefoxAccountAuthError = + findPreference(context.getPreferenceKey(pref_key_account_auth_error)) val accountPreferenceCategory = - findPreference(context!!.getPreferenceKey(pref_key_account_category)) + findPreference(context.getPreferenceKey(pref_key_account_category)) - if (hasCachedAccount) { + val accountManager = requireComponents.backgroundServices.accountManager + val account = accountManager.authenticatedAccount() + + // Signed-in, no problems. + if (account != null && !accountManager.accountNeedsReauth()) { preferenceSignIn?.isVisible = false preferenceSignIn?.onPreferenceClickListener = null + preferenceFirefoxAccountAuthError?.isVisible = false preferenceFirefoxAccount?.isVisible = true accountPreferenceCategory?.isVisible = true + + preferenceFirefoxAccount?.displayName = profile?.displayName + preferenceFirefoxAccount?.email = profile?.email + + // Signed-in, need to re-authenticate. + } else if (account != null && accountManager.accountNeedsReauth()) { + preferenceFirefoxAccount?.isVisible = false + preferenceFirefoxAccountAuthError?.isVisible = true + accountPreferenceCategory?.isVisible = true + + preferenceSignIn?.isVisible = false + preferenceSignIn?.onPreferenceClickListener = null + + preferenceFirefoxAccountAuthError?.email = profile?.email + + // Signed-out. } else { preferenceSignIn?.isVisible = true preferenceSignIn?.onPreferenceClickListener = getClickListenerForSignIn() preferenceFirefoxAccount?.isVisible = false + preferenceFirefoxAccountAuthError?.isVisible = false accountPreferenceCategory?.isVisible = false } } - - private fun updateAccountProfile(profile: Profile) { - launch { - context?.let { context -> - val preferenceFirefoxAccount = - findPreference(context.getPreferenceKey(pref_key_account)) - - preferenceFirefoxAccount?.title?.setTextColor( - R.attr.primaryText.getColorFromAttr(context) - ) - preferenceFirefoxAccount?.title?.text = profile.displayName.orEmpty() - preferenceFirefoxAccount?.title?.visibility = - if (preferenceFirefoxAccount?.title?.text.isNullOrEmpty()) View.GONE else View.VISIBLE - - preferenceFirefoxAccount?.summary?.setTextColor( - R.attr.primaryText.getColorFromAttr(context) - ) - preferenceFirefoxAccount?.summary?.text = profile.email.orEmpty() - - preferenceFirefoxAccount?.icon = ContextCompat.getDrawable(context, R.drawable.ic_shortcuts) - preferenceFirefoxAccount?.errorIcon?.visibility = View.GONE - preferenceFirefoxAccount?.background?.background = null - } - } - } - - private fun displayAccountErrorIfNecessary() { - launch { - context?.let { context -> - if (!context.components.backgroundServices.accountManager.accountNeedsReauth()) { return@launch } - - val preferenceFirefoxAccount = - findPreference(context.getPreferenceKey(pref_key_account)) - - preferenceFirefoxAccount?.title?.setTextColor( - ContextCompat.getColor( - context, - R.color.sync_error_text_color - ) - ) - preferenceFirefoxAccount?.title?.text = context.getString(R.string.preferences_account_sync_error) - preferenceFirefoxAccount?.title?.visibility = - if (preferenceFirefoxAccount?.title?.text.isNullOrEmpty()) View.GONE else View.VISIBLE - - preferenceFirefoxAccount?.summary?.setTextColor( - ContextCompat.getColor( - context, - R.color.sync_error_text_color - ) - ) - preferenceFirefoxAccount?.summary?.text = - context.components.backgroundServices.accountManager.accountProfile()?.email.orEmpty() - - preferenceFirefoxAccount?.icon = ContextCompat.getDrawable(context, R.drawable.ic_account_warning) - preferenceFirefoxAccount?.errorIcon?.visibility = View.VISIBLE - preferenceFirefoxAccount?.background?.background = - ContextCompat.getDrawable(context, R.color.sync_error_color) - } - } - } } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 1d134e821..7572d615b 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -152,9 +152,6 @@ class Settings private constructor(context: Context) { else -> appContext.getString(R.string.preference_light_theme) } - val hasCachedAccount: Boolean - get() = preferences.getBoolean(appContext.getPreferenceKey(R.string.pref_key_cached_account), false) - private val autoBounceQuickActionSheetCount: Int get() = (preferences.getInt(appContext.getPreferenceKey(R.string.pref_key_bounce_quick_action), 0)) @@ -229,12 +226,6 @@ class Settings private constructor(context: Context) { ) } - fun setHasCachedAccount(isCached: Boolean) { - preferences.edit() - .putBoolean(appContext.getPreferenceKey(R.string.pref_key_cached_account), isCached) - .apply() - } - private val SitePermissionsRules.Action.id: Int get() { return when (this) { diff --git a/app/src/main/res/layout/account_auth_error_preference.xml b/app/src/main/res/layout/account_auth_error_preference.xml new file mode 100644 index 000000000..faeeb3333 --- /dev/null +++ b/app/src/main/res/layout/account_auth_error_preference.xml @@ -0,0 +1,67 @@ + + + + + + + + + + + + + + + + + + + diff --git a/app/src/main/res/layout/account_preference.xml b/app/src/main/res/layout/account_preference.xml index 45d72aef8..5cddc705f 100644 --- a/app/src/main/res/layout/account_preference.xml +++ b/app/src/main/res/layout/account_preference.xml @@ -35,21 +35,20 @@ android:paddingBottom="16dp" android:layout_weight="1"> - - @@ -62,17 +61,4 @@ android:gravity="center_vertical" android:orientation="vertical"/> - - diff --git a/app/src/main/res/values/colors.xml b/app/src/main/res/values/colors.xml index 63d4e73d7..d1a2865e5 100644 --- a/app/src/main/res/values/colors.xml +++ b/app/src/main/res/values/colors.xml @@ -139,7 +139,7 @@ #cccccc #E7DFFF #232749 - #FFF36E + #FFF36E #960E18 diff --git a/app/src/main/res/values/preference_keys.xml b/app/src/main/res/values/preference_keys.xml index 536c35ef9..08d835413 100644 --- a/app/src/main/res/values/preference_keys.xml +++ b/app/src/main/res/values/preference_keys.xml @@ -22,6 +22,7 @@ pref_key_your_rights pref_key_account pref_key_sign_in + pref_key_account_auth_error pref_key_private_mode pref_key_theme pref_key_leakcanary diff --git a/app/src/main/res/xml/preferences.xml b/app/src/main/res/xml/preferences.xml index 021a4d585..5fa23c715 100644 --- a/app/src/main/res/xml/preferences.xml +++ b/app/src/main/res/xml/preferences.xml @@ -14,14 +14,18 @@ android:title="@string/preferences_sign_in" /> + android:key="@string/pref_key_account" /> + +