Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #6396 For #6553 - Update sync engine checkboxes and send telemetry once #7777

Merged
merged 1 commit into from
Jan 18, 2020
Merged

For #6396 For #6553 - Update sync engine checkboxes and send telemetry once #7777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ class SettingsFragment : PreferenceFragmentCompat() {
lifecycleScope.launch {
updateAccountUIState(
context = context,
profile = profile ?: context.components.backgroundServices.accountManager.accountProfile()
profile = profile
?: context.components.backgroundServices.accountManager.accountProfile()
)
}
}
Expand Down Expand Up @@ -107,7 +108,11 @@ class SettingsFragment : PreferenceFragmentCompat() {
try {
context?.let { context ->
context.components.analytics.metrics.track(
Event.PreferenceToggled(key, sharedPreferences.getBoolean(key, false), context)
Event.PreferenceToggled(
key,
sharedPreferences.getBoolean(key, false),
context
)
)
}
} catch (e: IllegalArgumentException) {
Expand Down Expand Up @@ -185,7 +190,6 @@ class SettingsFragment : PreferenceFragmentCompat() {
findPreference<Preference>(getPreferenceKey(pref_key_passwords))?.apply {
isVisible = FeatureFlags.logins
}

findPreference<PreferenceCategory>(getPreferenceKey(R.string.pref_key_advanced))?.apply {
isVisible = FeatureFlags.fenixLanguagePicker
}
Expand Down
28 changes: 16 additions & 12 deletions app/src/main/java/org/mozilla/fenix/settings/account/AccountSettingsFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,7 @@ class AccountSettingsFragment : PreferenceFragmentCompat() {
val historyNameKey = getPreferenceKey(R.string.pref_key_sync_history)
findPreference<CheckBoxPreference>(historyNameKey)?.apply {
setOnPreferenceChangeListener { _, newValue ->
requireComponents.analytics.metrics.track(Event.PreferenceToggled(
Copy link
Contributor Author

@ekager ekager Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We are already tracking these events in SettingsFragment so we were getting 2 pings here every time someone changed their setting (#6396).

preferenceKey = historyNameKey,
enabled = newValue as Boolean,
context = context
))
SyncEnginesStorage(context).setStatus(SyncEngine.History, newValue)
SyncEnginesStorage(context).setStatus(SyncEngine.History, newValue as Boolean)
@Suppress("DeferredResultUnused")
context.components.backgroundServices.accountManager.syncNowAsync(SyncReason.EngineChange)
true
Expand All @@ -181,12 +176,7 @@ class AccountSettingsFragment : PreferenceFragmentCompat() {
val bookmarksNameKey = getPreferenceKey(R.string.pref_key_sync_bookmarks)
findPreference<CheckBoxPreference>(bookmarksNameKey)?.apply {
setOnPreferenceChangeListener { _, newValue ->
requireComponents.analytics.metrics.track(Event.PreferenceToggled(
preferenceKey = bookmarksNameKey,
enabled = newValue as Boolean,
context = context
))
SyncEnginesStorage(context).setStatus(SyncEngine.Bookmarks, newValue)
SyncEnginesStorage(context).setStatus(SyncEngine.Bookmarks, newValue as Boolean)
@Suppress("DeferredResultUnused")
context.components.backgroundServices.accountManager.syncNowAsync(SyncReason.EngineChange)
true
Expand Down Expand Up @@ -327,13 +317,25 @@ class AccountSettingsFragment : PreferenceFragmentCompat() {
}
}

private fun setEnginesDisabledWhileSyncing(isSyncing: Boolean) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was weird behavior when you could interact with the toggles while syncing #6553 so I disabled them while syncing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a good solution to me

listOf(
R.string.pref_key_sync_bookmarks,
R.string.pref_key_sync_history,
R.string.pref_key_sync_logins
)
.map { getPreferenceKey(it) }
.map { findPreference<CheckBoxPreference>(it) }
.forEach { it?.isEnabled = !isSyncing }
}

private val syncStatusObserver = object : SyncStatusObserver {
override fun onStarted() {
lifecycleScope.launch {
val pref = findPreference<Preference>(getPreferenceKey(R.string.pref_key_sync_now))
view?.announceForAccessibility(getString(R.string.sync_syncing_in_progress))
pref?.title = getString(R.string.sync_syncing_in_progress)
pref?.isEnabled = false
setEnginesDisabledWhileSyncing(true)
}
}

Expand All @@ -350,6 +352,7 @@ class AccountSettingsFragment : PreferenceFragmentCompat() {
}
// Make sure out sync engine checkboxes are up-to-date.
updateSyncEngineStates()
setEnginesDisabledWhileSyncing(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we're intentionally leaving the keys disabled in onError (by not calling setEnginesDisabledWhileSyncing(false)). Is that correct? That makes sense to me (we can't do anything with their clicks until they reconnect anyway), but would you mind adding a comment in onError that states it explicitly?

}
}

Expand All @@ -359,6 +362,7 @@ class AccountSettingsFragment : PreferenceFragmentCompat() {
val pref = findPreference<Preference>(getPreferenceKey(R.string.pref_key_sync_now))
pref?.let {
pref.title = getString(R.string.preferences_sync_now)
// We want to only enable the sync button, and not the checkboxes here
pref.isEnabled = true

val failedTime = getLastSynced(requireContext())
Expand Down