From 2829958456f8b7348c11a04b3bf214ada14b24df Mon Sep 17 00:00:00 2001 From: Fan Zhang Date: Thu, 8 Dec 2016 11:28:31 -0800 Subject: [PATCH] Improve UI pref when sync/cancel account syncs. The core of the change is in ManageAccountSettings#showSyncState(). New code caches as much information as it can. And break out of loops as early as possible. Bug: 28575620 Test: make RunSettingsRoboTests Change-Id: I076ce148e3d8db55f6cadfd9491f037f7a55a986 --- .../{ => accounts}/AccountPreference.java | 10 +- .../accounts/AccountPreferenceBase.java | 2 + .../accounts/ManageAccountsSettings.java | 135 +++++++++++------- .../accounts/AccountPreferenceTest.java | 73 ++++++++++ .../accounts/ManageAccountsSettingsTest.java | 135 ++++++++++++++++++ 5 files changed, 300 insertions(+), 55 deletions(-) rename src/com/android/settings/{ => accounts}/AccountPreference.java (95%) create mode 100644 tests/robotests/src/com/android/settings/accounts/AccountPreferenceTest.java create mode 100644 tests/robotests/src/com/android/settings/accounts/ManageAccountsSettingsTest.java diff --git a/src/com/android/settings/AccountPreference.java b/src/com/android/settings/accounts/AccountPreference.java similarity index 95% rename from src/com/android/settings/AccountPreference.java rename to src/com/android/settings/accounts/AccountPreference.java index fe392448803..7d613b080df 100644 --- a/src/com/android/settings/AccountPreference.java +++ b/src/com/android/settings/accounts/AccountPreference.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 The Android Open Source Project + * Copyright (C) 2016 The Android Open Source Project * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -14,7 +14,7 @@ * limitations under the License. */ -package com.android.settings; +package com.android.settings.accounts; import android.accounts.Account; import android.content.Context; @@ -24,6 +24,8 @@ import android.support.v7.preference.PreferenceViewHolder; import android.util.Log; import android.widget.ImageView; +import com.android.settings.R; + import java.util.ArrayList; /** @@ -78,6 +80,10 @@ public class AccountPreference extends Preference { } public void setSyncStatus(int status, boolean updateSummary) { + if (mStatus == status) { + Log.d(TAG, "Status is the same, not changing anything"); + return; + } mStatus = status; if (!mShowTypeIcon && mSyncStatusIcon != null) { mSyncStatusIcon.setImageResource(getSyncStatusIcon(status)); diff --git a/src/com/android/settings/accounts/AccountPreferenceBase.java b/src/com/android/settings/accounts/AccountPreferenceBase.java index c6581ac525a..aa5c5183eac 100644 --- a/src/com/android/settings/accounts/AccountPreferenceBase.java +++ b/src/com/android/settings/accounts/AccountPreferenceBase.java @@ -32,6 +32,7 @@ import android.os.UserHandle; import android.os.UserManager; import android.support.v7.preference.PreferenceScreen; import android.text.format.DateFormat; +import android.text.format.DateUtils; import android.util.Log; import com.android.settings.SettingsPreferenceFragment; @@ -46,6 +47,7 @@ abstract class AccountPreferenceBase extends SettingsPreferenceFragment implements AuthenticatorHelper.OnAccountsUpdateListener { protected static final String TAG = "AccountSettings"; + protected static final boolean VERBOSE = Log.isLoggable(TAG, Log.VERBOSE); public static final String AUTHORITIES_FILTER_KEY = "authorities"; public static final String ACCOUNT_TYPES_FILTER_KEY = "account_types"; diff --git a/src/com/android/settings/accounts/ManageAccountsSettings.java b/src/com/android/settings/accounts/ManageAccountsSettings.java index abfa6a17d8a..ce717e25eb3 100644 --- a/src/com/android/settings/accounts/ManageAccountsSettings.java +++ b/src/com/android/settings/accounts/ManageAccountsSettings.java @@ -34,9 +34,11 @@ import android.content.pm.ResolveInfo; import android.graphics.drawable.Drawable; import android.os.Bundle; import android.os.UserHandle; +import android.support.annotation.VisibleForTesting; import android.support.v7.preference.Preference; import android.support.v7.preference.Preference.OnPreferenceClickListener; import android.support.v7.preference.PreferenceScreen; +import android.util.ArraySet; import android.util.Log; import android.view.LayoutInflater; import android.view.Menu; @@ -47,7 +49,6 @@ import android.view.ViewGroup; import android.widget.TextView; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; -import com.android.settings.AccountPreference; import com.android.settings.R; import com.android.settings.SettingsActivity; import com.android.settings.Utils; @@ -56,8 +57,8 @@ import com.android.settingslib.accounts.AuthenticatorHelper; import java.util.ArrayList; import java.util.Date; -import java.util.HashSet; import java.util.List; +import java.util.Set; import static android.content.Intent.EXTRA_USER; @@ -74,7 +75,7 @@ public class ManageAccountsSettings extends AccountPreferenceBase "com.android.settings.accounts.LAUNCHING_LOCATION_SETTINGS"; private static final int MENU_SYNC_NOW_ID = Menu.FIRST; - private static final int MENU_SYNC_CANCEL_ID = Menu.FIRST + 1; + private static final int MENU_SYNC_CANCEL_ID = Menu.FIRST + 1; private static final int REQUEST_SHOW_SYNC_SETTINGS = 1; @@ -87,6 +88,8 @@ public class ManageAccountsSettings extends AccountPreferenceBase // mFirstAccount is used for the injected preferences private Account mFirstAccount; + protected Set mUserFacingSyncAuthorities; + @Override public int getMetricsCategory() { return MetricsEvent.ACCOUNTS_MANAGE_ACCOUNTS; @@ -131,7 +134,7 @@ public class ManageAccountsSettings extends AccountPreferenceBase final Activity activity = getActivity(); final View view = getView(); - mErrorInfoView = (TextView)view.findViewById(R.id.sync_settings_error_info); + mErrorInfoView = (TextView) view.findViewById(R.id.sync_settings_error_info); mErrorInfoView.setVisibility(View.GONE); mAuthorities = activity.getIntent().getStringArrayExtra(AUTHORITIES_FILTER_KEY); @@ -188,8 +191,7 @@ public class ManageAccountsSettings extends AccountPreferenceBase @Override public void onPrepareOptionsMenu(Menu menu) { super.onPrepareOptionsMenu(menu); - boolean syncActive = !ContentResolver.getCurrentSyncsAsUser( - mUserHandle.getIdentifier()).isEmpty(); + boolean syncActive = !getCurrentSyncs(mUserHandle.getIdentifier()).isEmpty(); menu.findItem(MENU_SYNC_NOW_ID).setVisible(!syncActive); menu.findItem(MENU_SYNC_CANCEL_ID).setVisible(syncActive); } @@ -197,12 +199,12 @@ public class ManageAccountsSettings extends AccountPreferenceBase @Override public boolean onOptionsItemSelected(MenuItem item) { switch (item.getItemId()) { - case MENU_SYNC_NOW_ID: - requestOrCancelSyncForAccounts(true); - return true; - case MENU_SYNC_CANCEL_ID: - requestOrCancelSyncForAccounts(false); - return true; + case MENU_SYNC_NOW_ID: + requestOrCancelSyncForAccounts(true); + return true; + case MENU_SYNC_CANCEL_ID: + requestOrCancelSyncForAccounts(false); + return true; } return super.onOptionsItemSelected(item); } @@ -223,7 +225,7 @@ public class ManageAccountsSettings extends AccountPreferenceBase SyncAdapterType sa = syncAdapters[j]; if (syncAdapters[j].accountType.equals(mAccountType) && ContentResolver.getSyncAutomaticallyAsUser(account, sa.authority, - userId)) { + userId)) { if (sync) { ContentResolver.requestSyncAsUser(account, sa.authority, userId, extras); @@ -238,47 +240,58 @@ public class ManageAccountsSettings extends AccountPreferenceBase @Override protected void onSyncStateUpdated() { - showSyncState(); - // Catch any delayed delivery of update messages final Activity activity = getActivity(); - if (activity != null) { - activity.invalidateOptionsMenu(); + // Catch any delayed delivery of update messages + if (activity == null || activity.isFinishing()) { + return; + } + showSyncState(); + activity.invalidateOptionsMenu(); + } + + private void tryInitUserFacingSyncAuthorities(int userId) { + if (mUserFacingSyncAuthorities != null) { + return; + } + mUserFacingSyncAuthorities = new ArraySet<>(); + + // only track userfacing sync adapters when deciding if account is synced or not + final SyncAdapterType[] syncAdapters = ContentResolver.getSyncAdapterTypesAsUser(userId); + for (int k = 0, n = syncAdapters.length; k < n; k++) { + final SyncAdapterType sa = syncAdapters[k]; + if (sa.isUserVisible()) { + mUserFacingSyncAuthorities.add(sa.authority); + } } } /** * Shows the sync state of the accounts. Note: it must be called after the accounts have been - * loaded, @see #showAccountsIfNeeded(). + * loaded. + * + * @see {@link #showAccountsIfNeeded()}. */ - private void showSyncState() { - // Catch any delayed delivery of update messages - if (getActivity() == null || getActivity().isFinishing()) return; - + @VisibleForTesting + void showSyncState() { final int userId = mUserHandle.getIdentifier(); + tryInitUserFacingSyncAuthorities(userId); // iterate over all the preferences, setting the state properly for each - List currentSyncs = ContentResolver.getCurrentSyncsAsUser(userId); + final List currentSyncs = getCurrentSyncs(userId); boolean anySyncFailed = false; // true if sync on any account failed Date date = new Date(); - // only track userfacing sync adapters when deciding if account is synced or not - final SyncAdapterType[] syncAdapters = ContentResolver.getSyncAdapterTypesAsUser(userId); - HashSet userFacing = new HashSet(); - for (int k = 0, n = syncAdapters.length; k < n; k++) { - final SyncAdapterType sa = syncAdapters[k]; - if (sa.isUserVisible()) { - userFacing.add(sa.authority); - } - } - for (int i = 0, count = getPreferenceScreen().getPreferenceCount(); i < count; i++) { - Preference pref = getPreferenceScreen().getPreference(i); - if (! (pref instanceof AccountPreference)) { + final PreferenceScreen screen = getPreferenceScreen(); + final int prefCount = screen.getPreferenceCount(); + for (int i = 0; i < prefCount; i++) { + Preference pref = screen.getPreference(i); + if (!(pref instanceof AccountPreference)) { continue; } - AccountPreference accountPref = (AccountPreference) pref; - Account account = accountPref.getAccount(); + final AccountPreference accountPref = (AccountPreference) pref; + final Account account = accountPref.getAccount(); int syncCount = 0; long lastSuccessTime = 0; boolean syncIsFailing = false; @@ -286,28 +299,33 @@ public class ManageAccountsSettings extends AccountPreferenceBase boolean syncingNow = false; if (authorities != null) { for (String authority : authorities) { - SyncStatusInfo status = ContentResolver.getSyncStatusAsUser(account, authority, - userId); + SyncStatusInfo status = getSyncStatusInfo(account, authority, userId); boolean syncEnabled = isSyncEnabled(userId, account, authority); - boolean authorityIsPending = ContentResolver.isSyncPending(account, authority); boolean activelySyncing = isSyncing(currentSyncs, account, authority); boolean lastSyncFailed = status != null && syncEnabled && status.lastFailureTime != 0 && status.getLastFailureMesgAsInt(0) - != ContentResolver.SYNC_ERROR_SYNC_ALREADY_IN_PROGRESS; - if (lastSyncFailed && !activelySyncing && !authorityIsPending) { + != ContentResolver.SYNC_ERROR_SYNC_ALREADY_IN_PROGRESS; + if (lastSyncFailed && !activelySyncing + && !ContentResolver.isSyncPending(account, authority)) { syncIsFailing = true; anySyncFailed = true; + break; } - syncingNow |= activelySyncing; + if (status != null && lastSuccessTime < status.lastSuccessTime) { lastSuccessTime = status.lastSuccessTime; } - syncCount += syncEnabled && userFacing.contains(authority) ? 1 : 0; + syncCount += syncEnabled && mUserFacingSyncAuthorities.contains(authority) + ? 1 : 0; + syncingNow |= activelySyncing; + if (syncingNow) { + break; + } } } else { - if (Log.isLoggable(TAG, Log.VERBOSE)) { + if (VERBOSE) { Log.v(TAG, "no syncadapters found for " + account); } } @@ -332,14 +350,14 @@ public class ManageAccountsSettings extends AccountPreferenceBase accountPref.setSyncStatus(AccountPreference.SYNC_DISABLED, true); } } - - mErrorInfoView.setVisibility(anySyncFailed ? View.VISIBLE : View.GONE); + if (mErrorInfoView != null) { + mErrorInfoView.setVisibility(anySyncFailed ? View.VISIBLE : View.GONE); + } } - private boolean isSyncing(List currentSyncs, Account account, String authority) { final int count = currentSyncs.size(); - for (int i = 0; i < count; i++) { + for (int i = 0; i < count; i++) { SyncInfo syncInfo = currentSyncs.get(i); if (syncInfo.account.equals(account) && syncInfo.authority.equals(authority)) { return true; @@ -348,7 +366,8 @@ public class ManageAccountsSettings extends AccountPreferenceBase return false; } - private boolean isSyncEnabled(int userId, Account account, String authority) { + @VisibleForTesting + protected boolean isSyncEnabled(int userId, Account account, String authority) { return ContentResolver.getSyncAutomaticallyAsUser(account, authority, userId) && ContentResolver.getMasterSyncAutomaticallyAsUser(userId) && (ContentResolver.getIsSyncableAsUser(account, authority, userId) > 0); @@ -436,7 +455,7 @@ public class ManageAccountsSettings extends AccountPreferenceBase */ private void updatePreferenceIntents(PreferenceScreen prefs) { final PackageManager pm = getActivity().getPackageManager(); - for (int i = 0; i < prefs.getPreferenceCount();) { + for (int i = 0; i < prefs.getPreferenceCount(); ) { Preference pref = prefs.getPreference(i); Intent intent = pref.getIntent(); if (intent != null) { @@ -486,8 +505,8 @@ public class ManageAccountsSettings extends AccountPreferenceBase } else { Log.e(TAG, "Refusing to launch authenticator intent because" - + "it exploits Settings permissions: " - + prefIntent); + + "it exploits Settings permissions: " + + prefIntent); } return true; } @@ -536,4 +555,14 @@ public class ManageAccountsSettings extends AccountPreferenceBase } } } + + @VisibleForTesting + protected List getCurrentSyncs(int userId) { + return ContentResolver.getCurrentSyncsAsUser(userId); + } + + @VisibleForTesting + protected SyncStatusInfo getSyncStatusInfo(Account account, String authority, int userId) { + return ContentResolver.getSyncStatusAsUser(account, authority, userId); + } } diff --git a/tests/robotests/src/com/android/settings/accounts/AccountPreferenceTest.java b/tests/robotests/src/com/android/settings/accounts/AccountPreferenceTest.java new file mode 100644 index 00000000000..eeca90bf8db --- /dev/null +++ b/tests/robotests/src/com/android/settings/accounts/AccountPreferenceTest.java @@ -0,0 +1,73 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.accounts; + +import android.accounts.Account; +import android.content.Context; + +import com.android.settings.R; +import com.android.settings.SettingsRobolectricTestRunner; +import com.android.settings.TestConfig; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.annotation.Config; +import org.robolectric.shadows.ShadowApplication; + +import java.util.ArrayList; + +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + +@RunWith(SettingsRobolectricTestRunner.class) +@Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION) +public class AccountPreferenceTest { + + private Context mContext; + private Account mAccount; + private ArrayList mAuthorities; + private AccountPreference mPreference; + + @Before + public void setUp() { + mContext = ShadowApplication.getInstance().getApplicationContext(); + mAccount = new Account("name", "type"); + mAuthorities = new ArrayList<>(); + mAuthorities.add("authority"); + + mPreference = spy(new AccountPreference( + mContext, mAccount, null /* icon */, mAuthorities, false /* showTypeIcon */)); + } + + @Test + public void setSyncStatus_differentStatus_shouldUpdate() { + mPreference.setSyncStatus(AccountPreference.SYNC_ERROR, true); + verify(mPreference).setSummary(R.string.sync_error); + } + + @Test + public void setSyncStatus_sameStatus_shouldNotUpdate() { + // Set it once, should update summary + mPreference.setSyncStatus(AccountPreference.SYNC_ERROR, true); + verify(mPreference).setSummary(R.string.sync_error); + + // Set it again, should not update summary + mPreference.setSyncStatus(AccountPreference.SYNC_ERROR, true); + verify(mPreference).setSummary(R.string.sync_error); + } +} diff --git a/tests/robotests/src/com/android/settings/accounts/ManageAccountsSettingsTest.java b/tests/robotests/src/com/android/settings/accounts/ManageAccountsSettingsTest.java new file mode 100644 index 00000000000..916e395d46b --- /dev/null +++ b/tests/robotests/src/com/android/settings/accounts/ManageAccountsSettingsTest.java @@ -0,0 +1,135 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.accounts; + +import android.accounts.Account; +import android.content.SyncInfo; +import android.content.SyncStatusInfo; +import android.os.UserHandle; +import android.support.v7.preference.PreferenceScreen; +import android.util.ArraySet; + +import com.android.settings.SettingsRobolectricTestRunner; +import com.android.settings.TestConfig; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Answers; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.robolectric.annotation.Config; + +import java.util.ArrayList; +import java.util.List; + +import static org.mockito.Matchers.anyInt; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(SettingsRobolectricTestRunner.class) +@Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION) +public class ManageAccountsSettingsTest { + + @Mock(answer = Answers.RETURNS_DEEP_STUBS) + private AccountPreference mAccountPref; + private Account mAccount; + private ArrayList mAuthorities; + private TestFragment mSettings; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + mAuthorities = new ArrayList<>(); + mAuthorities.add("authority"); + mAccount = new Account("name", "type"); + when(mAccountPref.getAccount()).thenReturn(mAccount); + when(mAccountPref.getAuthorities()).thenReturn(mAuthorities); + mSettings = new TestFragment(); + } + + @Test + public void showSyncState_noAccountPrefs_shouldUpdateNothing() { + when(mAccountPref.getAuthorities()).thenReturn(null); + mSettings.showSyncState(); + verify(mSettings.getPreferenceScreen(), never()).getPreference(anyInt()); + } + + @Test + public void showSyncState_syncInProgress_shouldUpdateInProgress() { + mSettings.mUserFacingSyncAuthorities.add(mAuthorities.get(0)); + mSettings.mSyncInfos.add(new SyncInfo(0, mAccount, mAuthorities.get(0), 0)); + mSettings.mSyncStatusInfo = new SyncStatusInfo(0); + when(mSettings.getPreferenceScreen().getPreferenceCount()).thenReturn(1); + when(mSettings.getPreferenceScreen().getPreference(0)).thenReturn(mAccountPref); + + mSettings.showSyncState(); + + verify(mSettings.getPreferenceScreen()).getPreference(anyInt()); + verify(mAccountPref).setSyncStatus(AccountPreference.SYNC_IN_PROGRESS, true); + } + + @Test + public void showSyncState_noUserFacingSynclets_shouldUpdateToDisabled() { + mSettings.mSyncInfos.add(new SyncInfo(0, mAccount, mAuthorities.get(0), 0)); + mSettings.mSyncStatusInfo = new SyncStatusInfo(0); + when(mSettings.getPreferenceScreen().getPreferenceCount()).thenReturn(1); + when(mSettings.getPreferenceScreen().getPreference(0)).thenReturn(mAccountPref); + + mSettings.showSyncState(); + + verify(mSettings.getPreferenceScreen()).getPreference(anyInt()); + verify(mAccountPref).setSyncStatus(AccountPreference.SYNC_DISABLED, true); + } + + public static class TestFragment extends ManageAccountsSettings { + + private PreferenceScreen mScreen; + private List mSyncInfos; + private SyncStatusInfo mSyncStatusInfo; + + public TestFragment() { + mUserHandle = mock(UserHandle.class); + mScreen = mock(PreferenceScreen.class); + mUserFacingSyncAuthorities = new ArraySet<>(); + mSyncInfos = new ArrayList<>(); + } + + @Override + public PreferenceScreen getPreferenceScreen() { + return mScreen; + } + + @Override + protected boolean isSyncEnabled(int userId, Account account, String authority) { + return true; + } + + @Override + protected List getCurrentSyncs(int userId) { + return mSyncInfos; + } + + @Override + protected SyncStatusInfo getSyncStatusInfo(Account account, String authority, int userId) { + return mSyncStatusInfo; + } + } + +} \ No newline at end of file