For #6396 For #6553 - Update sync engine checkboxes and send telemetry once #7777
Conversation
ekager
force-pushed
the
logins-true
branch
2 times, most recently
from
2e452cd to
d30d91a
Compare
Codecov Report
@@ Coverage Diff @@
## master #7777 +/- ##
============================================
+ Coverage 19.14% 19.15% +<.01%
Complexity 452 452
============================================
Files 301 301
Lines 11668 11673 +5
Branches 1585 1586 +1
============================================
+ Hits 2234 2236 +2
- Misses 9249 9253 +4
+ Partials 185 184 -1
Continue to review full report at Codecov.
|
| @@ -166,12 +165,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( | |||
There was a problem hiding this comment.
We are already tracking these events in SettingsFragment so we were getting 2 pings here every time someone changed their setting (#6396).
| @@ -327,13 +315,29 @@ class AccountSettingsFragment : PreferenceFragmentCompat() { | |||
| } | |||
| } | |||
|
|
|||
| private fun setEnginesDisabledWhileSyncing(isSyncing: Boolean) { | |||
There was a problem hiding this comment.
There was weird behavior when you could interact with the toggles while syncing #6553 so I disabled them while syncing.
There was a problem hiding this comment.
Sounds like a good solution to me
| isEnabled = syncEnginesStatus.containsKey(SyncEngine.Passwords) | ||
| isChecked = syncEnginesStatus.getOrElse(SyncEngine.Passwords) { false } | ||
| isChecked = syncEnginesStatus.getOrElse(SyncEngine.Passwords) { true } |
There was a problem hiding this comment.
From #6737 (comment) it sounds like AS changes should paper over this problem, but shouldn't we default to false here anyway (in case of network problems, for example)?
There was a problem hiding this comment.
Sorry, I should have updated the bug! Since we have all the biometrics/pin checking set up now and we are removing the nightly flag, we are changing the default for syncing logins to true. cc @liuche to confirm
There was a problem hiding this comment.
Nvm changing this is dependent on #6704 being on for release users
| @@ -327,13 +315,29 @@ class AccountSettingsFragment : PreferenceFragmentCompat() { | |||
| } | |||
| } | |||
|
|
|||
| private fun setEnginesDisabledWhileSyncing(isSyncing: Boolean) { | |||
There was a problem hiding this comment.
Sounds like a good solution to me
| } | ||
| val loginsNameKey = getPreferenceKey(R.string.pref_key_sync_logins) | ||
| findPreference<CheckBoxPreference>(loginsNameKey)?.apply { | ||
| isEnabled = !isSyncing |
There was a problem hiding this comment.
Optional, but this could also be written as:
private fun setEnginesDisabledWhileSyncing(isSyncing: Boolean) {
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 }
}or if you prefer a more imperative structure
private fun setEnginesDisabledWhileSyncing(isSyncing: Boolean) {
fun setEnabled(@StringRes strId: Int) {
val key = getPreferenceKey(strId)
findPreference<CheckBoxPreference>(key)?.apply {
isEnabled = !isSyncing
}
}
setEnabled(R.string.pref_key_sync_bookmarks)
setEnabled(R.string.pref_key_sync_history)
setEnabled(R.string.pref_key_sync_logins)
}Both communicate to the reader that the same changes are being made to each pref, and reduce the space for typos to hide. But it's up to you if you want to change it, the original code is fine too.
| @@ -350,6 +354,7 @@ class AccountSettingsFragment : PreferenceFragmentCompat() { | |||
| } | |||
| // Make sure out sync engine checkboxes are up-to-date. | |||
| updateSyncEngineStates() | |||
| setEnginesDisabledWhileSyncing(false) | |||
There was a problem hiding this comment.
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?
| @@ -27,10 +26,9 @@ | |||
| android:title="@string/preferences_sync_history" /> | |||
|
|
|||
| <androidx.preference.CheckBoxPreference | |||
| android:defaultValue="false" | |||
| android:defaultValue="true" | |||
There was a problem hiding this comment.
Same question as above, I'm not clear on why this should default to true.
ekager
force-pushed
the
logins-true
branch
3 times, most recently
from
a551a74 to
a1d5880
Compare
ekager
changed the title
ekager
added
the
pr:needs-landing
…checkboxes and send telemetry once
|
Okay sorry for the confusion, the final decision was to keep the feature flags for logins for now. So this PR really is just fixing telemetry for sync and adding the disabled state for the engine checkboxes. Thanks for your help!! |
|
Filed #7796 to detail the steps required for turning on Logins, there are a few dependencies still. |
Pull Request checklist
After merge
To download an APK when reviewing a PR: