From 4cb2727989a48e1470f01233b78685165e9b193b Mon Sep 17 00:00:00 2001 From: Fan Zhang Date: Thu, 14 Mar 2019 14:18:45 -0700 Subject: [PATCH] Tie ContextualWifiSlice to UI instead of garbage collector - Introduce a static long in SliceFeatureProvider that updates once every "session". A session is when user is in an UI (screen rotation, going to subpage, etc does not break the continuation of session). - Use session token in ContextualWifiSlice to determine when to refresh, instead of relying on WeakHashMap from CustomSliceManager. WeakHashMap can be cleaned up at any time by gc so it doesn't match what we want on the UI. - Also as a side fix, merged CustomSliceManager into SliceFeatureProvider. Fixes: 123937830 Test: robo Change-Id: I199bceceb208b99a32f3f08e624787b5a03e73a9 --- .../contextualcards/ContextualCardLoader.java | 3 +- .../ContextualCardManager.java | 4 +- .../ContextualCardsFragment.java | 12 +++- .../settings/slices/CustomSliceManager.java | 60 ------------------- .../settings/slices/CustomSliceRegistry.java | 4 +- .../settings/slices/CustomSliceable.java | 2 +- .../slices/SettingsSliceProvider.java | 16 +++-- .../slices/SlicesFeatureProvider.java | 24 +++++++- .../slices/SlicesFeatureProviderImpl.java | 30 ++++++++-- .../wifi/slice/ContextualWifiSlice.java | 17 ++++-- .../slices/SettingsSliceProviderTest.java | 1 - .../slices/SlicesDatabaseHelperTest.java | 1 + .../wifi/slice/ContextualWifiSliceTest.java | 30 +++++++++- 13 files changed, 113 insertions(+), 91 deletions(-) delete mode 100644 src/com/android/settings/slices/CustomSliceManager.java diff --git a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java index 13564b5bc59..ff2ee91468e 100644 --- a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java +++ b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java @@ -67,7 +67,8 @@ public class ContextualCardLoader extends AsyncLoaderCompat @VisibleForTesting Uri mNotifyUri; - private Context mContext; + + private final Context mContext; ContextualCardLoader(Context context) { super(context); diff --git a/src/com/android/settings/homepage/contextualcards/ContextualCardManager.java b/src/com/android/settings/homepage/contextualcards/ContextualCardManager.java index c829015ed27..8f7e84acd2f 100644 --- a/src/com/android/settings/homepage/contextualcards/ContextualCardManager.java +++ b/src/com/android/settings/homepage/contextualcards/ContextualCardManager.java @@ -109,13 +109,13 @@ public class ContextualCardManager implements ContextualCardLoader.CardContentLo } } - void loadContextualCards(ContextualCardsFragment fragment) { + void loadContextualCards(LoaderManager loaderManager) { mStartTime = System.currentTimeMillis(); final CardContentLoaderCallbacks cardContentLoaderCallbacks = new CardContentLoaderCallbacks(mContext); cardContentLoaderCallbacks.setListener(this); // Use the cached data when navigating back to the first page and upon screen rotation. - LoaderManager.getInstance(fragment).initLoader(CARD_CONTENT_LOADER_ID, null /* bundle */, + loaderManager.initLoader(CARD_CONTENT_LOADER_ID, null /* bundle */, cardContentLoaderCallbacks); } diff --git a/src/com/android/settings/homepage/contextualcards/ContextualCardsFragment.java b/src/com/android/settings/homepage/contextualcards/ContextualCardsFragment.java index e598e4c6946..72ddb50eafb 100644 --- a/src/com/android/settings/homepage/contextualcards/ContextualCardsFragment.java +++ b/src/com/android/settings/homepage/contextualcards/ContextualCardsFragment.java @@ -19,16 +19,19 @@ package com.android.settings.homepage.contextualcards; import static com.android.settings.homepage.contextualcards.ContextualCardsAdapter.SPAN_COUNT; import android.app.settings.SettingsEnums; +import android.content.Context; import android.os.Bundle; import android.view.LayoutInflater; import android.view.View; import android.view.ViewGroup; +import androidx.loader.app.LoaderManager; import androidx.recyclerview.widget.GridLayoutManager; import androidx.recyclerview.widget.RecyclerView; import com.android.settings.R; import com.android.settings.core.InstrumentedFragment; +import com.android.settings.overlay.FeatureFactory; public class ContextualCardsFragment extends InstrumentedFragment { @@ -42,14 +45,19 @@ public class ContextualCardsFragment extends InstrumentedFragment { @Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); - mContextualCardManager = new ContextualCardManager(getContext(), getSettingsLifecycle(), + final Context context = getContext(); + if (savedInstanceState == null) { + FeatureFactory.getFactory(context).getSlicesFeatureProvider().newUiSession(); + } + mContextualCardManager = new ContextualCardManager(context, getSettingsLifecycle(), savedInstanceState); + } @Override public void onStart() { super.onStart(); - mContextualCardManager.loadContextualCards(this); + mContextualCardManager.loadContextualCards(LoaderManager.getInstance(this)); } @Override diff --git a/src/com/android/settings/slices/CustomSliceManager.java b/src/com/android/settings/slices/CustomSliceManager.java deleted file mode 100644 index d0a65ba6eb0..00000000000 --- a/src/com/android/settings/slices/CustomSliceManager.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright (C) 2018 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.slices; - -import android.content.Context; -import android.net.Uri; - -import java.util.Map; -import java.util.WeakHashMap; - -/** - * Manages custom {@link androidx.slice.Slice Slices}, which are all Slices not backed by - * preferences. - */ -public class CustomSliceManager { - - private final Context mContext; - private final Map mSliceableCache; - - public CustomSliceManager(Context context) { - mContext = context.getApplicationContext(); - mSliceableCache = new WeakHashMap<>(); - } - - /** - * Return a {@link CustomSliceable} associated to the Uri. - *

- * Do not change this method signature to accommodate for a special-case slicable - a context is - * the only thing that should be needed to create the object. - */ - public CustomSliceable getSliceableFromUri(Uri uri) { - final Uri newUri = CustomSliceRegistry.removeParameterFromUri(uri); - if (mSliceableCache.containsKey(newUri)) { - return mSliceableCache.get(newUri); - } - - final Class clazz = CustomSliceRegistry.getSliceClassByUri(newUri); - if (clazz == null) { - throw new IllegalArgumentException("No Slice found for uri: " + uri); - } - - final CustomSliceable sliceable = CustomSliceable.createInstance(mContext, clazz); - mSliceableCache.put(newUri, sliceable); - return sliceable; - } -} diff --git a/src/com/android/settings/slices/CustomSliceRegistry.java b/src/com/android/settings/slices/CustomSliceRegistry.java index 12e7b48ffb2..b3b353b4510 100644 --- a/src/com/android/settings/slices/CustomSliceRegistry.java +++ b/src/com/android/settings/slices/CustomSliceRegistry.java @@ -331,7 +331,7 @@ public class CustomSliceRegistry { /** * Returns {@code true} if {@param uri} is a valid Slice Uri handled by - * {@link CustomSliceManager}. + * {@link CustomSliceRegistry}. */ public static boolean isValidUri(Uri uri) { return sUriToSlice.containsKey(removeParameterFromUri(uri)); @@ -339,7 +339,7 @@ public class CustomSliceRegistry { /** * Returns {@code true} if {@param action} is a valid intent action handled by - * {@link CustomSliceManager}. + * {@link CustomSliceRegistry}. */ public static boolean isValidAction(String action) { return isValidUri(Uri.parse(action)); diff --git a/src/com/android/settings/slices/CustomSliceable.java b/src/com/android/settings/slices/CustomSliceable.java index 8393d4ca6f8..93d08a21711 100644 --- a/src/com/android/settings/slices/CustomSliceable.java +++ b/src/com/android/settings/slices/CustomSliceable.java @@ -110,7 +110,7 @@ public interface CustomSliceable extends Sliceable { try { final Constructor constructor = sliceable.getConstructor(Context.class); - final Object[] params = new Object[]{context}; + final Object[] params = new Object[]{context.getApplicationContext()}; return constructor.newInstance(params); } catch (NoSuchMethodException | InstantiationException | IllegalArgumentException | InvocationTargetException | IllegalAccessException e) { diff --git a/src/com/android/settings/slices/SettingsSliceProvider.java b/src/com/android/settings/slices/SettingsSliceProvider.java index d019368fd62..3187d109994 100644 --- a/src/com/android/settings/slices/SettingsSliceProvider.java +++ b/src/com/android/settings/slices/SettingsSliceProvider.java @@ -117,9 +117,6 @@ public class SettingsSliceProvider extends SliceProvider { private static final KeyValueListParser KEY_VALUE_LIST_PARSER = new KeyValueListParser(','); - @VisibleForTesting - CustomSliceManager mCustomSliceManager; - @VisibleForTesting SlicesDatabaseAccessor mSlicesDatabaseAccessor; @@ -140,15 +137,15 @@ public class SettingsSliceProvider extends SliceProvider { mSlicesDatabaseAccessor = new SlicesDatabaseAccessor(getContext()); mSliceDataCache = new ConcurrentHashMap<>(); mSliceWeakDataCache = new WeakHashMap<>(); - mCustomSliceManager = FeatureFactory.getFactory( - getContext()).getSlicesFeatureProvider().getCustomSliceManager(getContext()); return true; } @Override public void onSlicePinned(Uri sliceUri) { if (CustomSliceRegistry.isValidUri(sliceUri)) { - final CustomSliceable sliceable = mCustomSliceManager.getSliceableFromUri(sliceUri); + final Context context = getContext(); + final CustomSliceable sliceable = FeatureFactory.getFactory(context) + .getSlicesFeatureProvider().getSliceableFromUri(context, sliceUri); final IntentFilter filter = sliceable.getIntentFilter(); if (filter != null) { registerIntentToUri(filter, sliceUri); @@ -195,9 +192,10 @@ public class SettingsSliceProvider extends SliceProvider { // Before adding a slice to {@link CustomSliceManager}, please get approval // from the Settings team. if (CustomSliceRegistry.isValidUri(sliceUri)) { - final CustomSliceable sliceable = mCustomSliceManager.getSliceableFromUri( - sliceUri); - return sliceable.getSlice(); + final Context context = getContext(); + return FeatureFactory.getFactory(context) + .getSlicesFeatureProvider().getSliceableFromUri(context, sliceUri) + .getSlice(); } if (CustomSliceRegistry.WIFI_CALLING_URI.equals(sliceUri)) { diff --git a/src/com/android/settings/slices/SlicesFeatureProvider.java b/src/com/android/settings/slices/SlicesFeatureProvider.java index 16a7424f2a6..ae94f29bcb3 100644 --- a/src/com/android/settings/slices/SlicesFeatureProvider.java +++ b/src/com/android/settings/slices/SlicesFeatureProvider.java @@ -1,6 +1,7 @@ package com.android.settings.slices; import android.content.Context; +import android.net.Uri; import com.android.settings.network.telephony.Enhanced4gLteSliceHelper; import com.android.settings.wifi.calling.WifiCallingSliceHelper; @@ -14,6 +15,20 @@ public interface SlicesFeatureProvider { SliceDataConverter getSliceDataConverter(Context context); + /** + * Starts a new UI session for the purpose of using Slices. + * + * A UI session is defined as an duration of time when user stays in a UI screen. Screen + * rotation does not break the continuation of session, going to a sub-page and coming out does + * not break the continuation either. Leaving the page and coming back breaks it. + */ + void newUiSession(); + + /** + * Returns the token created in {@link #newUiSession}. + */ + long getUiSessionToken(); + /** * Asynchronous call to index the data used to build Slices. * If the data is already indexed, the data will not change. @@ -26,7 +41,14 @@ public interface SlicesFeatureProvider { */ void indexSliceData(Context context); - CustomSliceManager getCustomSliceManager(Context context); + + /** + * Return a {@link CustomSliceable} associated to the Uri. + *

+ * Do not change this method signature to accommodate for a special-case sliceable - a context + * is the only thing that should be needed to create the object. + */ + CustomSliceable getSliceableFromUri(Context context, Uri uri); /** * Gets new WifiCallingSliceHelper object diff --git a/src/com/android/settings/slices/SlicesFeatureProviderImpl.java b/src/com/android/settings/slices/SlicesFeatureProviderImpl.java index 44863ec2b1a..297f2c159d8 100644 --- a/src/com/android/settings/slices/SlicesFeatureProviderImpl.java +++ b/src/com/android/settings/slices/SlicesFeatureProviderImpl.java @@ -17,19 +17,23 @@ package com.android.settings.slices; import android.content.Context; +import android.net.Uri; +import android.os.SystemClock; import com.android.settings.network.telephony.Enhanced4gLteSliceHelper; import com.android.settings.wifi.calling.WifiCallingSliceHelper; import com.android.settingslib.utils.ThreadUtils; +import java.util.Random; + /** * Manages Slices in Settings. */ public class SlicesFeatureProviderImpl implements SlicesFeatureProvider { + private long mUiSessionToken; private SlicesIndexer mSlicesIndexer; private SliceDataConverter mSliceDataConverter; - private CustomSliceManager mCustomSliceManager; @Override public SliceDataConverter getSliceDataConverter(Context context) { @@ -40,11 +44,13 @@ public class SlicesFeatureProviderImpl implements SlicesFeatureProvider { } @Override - public CustomSliceManager getCustomSliceManager(Context context) { - if (mCustomSliceManager == null) { - mCustomSliceManager = new CustomSliceManager(context.getApplicationContext()); - } - return mCustomSliceManager; + public void newUiSession() { + mUiSessionToken = SystemClock.elapsedRealtime(); + } + + @Override + public long getUiSessionToken() { + return mUiSessionToken; } @Override @@ -69,6 +75,18 @@ public class SlicesFeatureProviderImpl implements SlicesFeatureProvider { return new Enhanced4gLteSliceHelper(context); } + @Override + public CustomSliceable getSliceableFromUri(Context context, Uri uri) { + final Uri newUri = CustomSliceRegistry.removeParameterFromUri(uri); + final Class clazz = CustomSliceRegistry.getSliceClassByUri(newUri); + if (clazz == null) { + throw new IllegalArgumentException("No Slice found for uri: " + uri); + } + + final CustomSliceable sliceable = CustomSliceable.createInstance(context, clazz); + return sliceable; + } + private SlicesIndexer getSliceIndexer(Context context) { if (mSlicesIndexer == null) { mSlicesIndexer = new SlicesIndexer(context.getApplicationContext()); diff --git a/src/com/android/settings/wifi/slice/ContextualWifiSlice.java b/src/com/android/settings/wifi/slice/ContextualWifiSlice.java index fa8c2678e29..4a799d15f6a 100644 --- a/src/com/android/settings/wifi/slice/ContextualWifiSlice.java +++ b/src/com/android/settings/wifi/slice/ContextualWifiSlice.java @@ -25,6 +25,7 @@ import android.util.Log; import androidx.annotation.VisibleForTesting; import androidx.slice.Slice; +import com.android.settings.overlay.FeatureFactory; import com.android.settings.slices.CustomSliceRegistry; import com.android.settings.slices.CustomSliceable; @@ -35,7 +36,9 @@ public class ContextualWifiSlice extends WifiSlice { private static final String TAG = "ContextualWifiSlice"; @VisibleForTesting - boolean mPreviouslyDisplayed; + static long sActiveUiSession = -1000; + @VisibleForTesting + static boolean sPreviouslyDisplayed; public ContextualWifiSlice(Context context) { super(context); @@ -48,13 +51,19 @@ public class ContextualWifiSlice extends WifiSlice { @Override public Slice getSlice() { - if (!mPreviouslyDisplayed && !TextUtils.equals(getActiveSSID(), WifiSsid.NONE)) { + final long currentUiSession = FeatureFactory.getFactory(mContext) + .getSlicesFeatureProvider().getUiSessionToken(); + if (currentUiSession != sActiveUiSession) { + sActiveUiSession = currentUiSession; + sPreviouslyDisplayed = false; + } + if (!sPreviouslyDisplayed && !TextUtils.equals(getActiveSSID(), WifiSsid.NONE)) { Log.d(TAG, "Wifi is connected, no point showing any suggestion."); return null; } - // Set mPreviouslyDisplayed to true - we will show *something* on the screen. So we should + // Set sPreviouslyDisplayed to true - we will show *something* on the screen. So we should // keep showing this card to keep UI stable, even if wifi connects to a network later. - mPreviouslyDisplayed = true; + sPreviouslyDisplayed = true; return super.getSlice(); } diff --git a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java index a693f349894..23025b2ebc6 100644 --- a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java +++ b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java @@ -135,7 +135,6 @@ public class SettingsSliceProviderTest { mProvider.mSliceWeakDataCache = new HashMap<>(); mProvider.mSliceDataCache = new HashMap<>(); mProvider.mSlicesDatabaseAccessor = new SlicesDatabaseAccessor(mContext); - mProvider.mCustomSliceManager = new CustomSliceManager(mContext); when(mProvider.getContext()).thenReturn(mContext); SlicesDatabaseHelper.getInstance(mContext).setIndexedState(); diff --git a/tests/robotests/src/com/android/settings/slices/SlicesDatabaseHelperTest.java b/tests/robotests/src/com/android/settings/slices/SlicesDatabaseHelperTest.java index 0e92c05c9cb..96bca0728a8 100644 --- a/tests/robotests/src/com/android/settings/slices/SlicesDatabaseHelperTest.java +++ b/tests/robotests/src/com/android/settings/slices/SlicesDatabaseHelperTest.java @@ -55,6 +55,7 @@ public class SlicesDatabaseHelperTest { @After public void cleanUp() { DatabaseTestUtils.clearDb(mContext); + mDatabase.close(); } @Test diff --git a/tests/robotests/src/com/android/settings/wifi/slice/ContextualWifiSliceTest.java b/tests/robotests/src/com/android/settings/wifi/slice/ContextualWifiSliceTest.java index d681afebf65..520d98897fc 100644 --- a/tests/robotests/src/com/android/settings/wifi/slice/ContextualWifiSliceTest.java +++ b/tests/robotests/src/com/android/settings/wifi/slice/ContextualWifiSliceTest.java @@ -36,6 +36,8 @@ import androidx.slice.widget.SliceLiveData; import com.android.settings.R; import com.android.settings.slices.CustomSliceRegistry; +import com.android.settings.slices.SlicesFeatureProviderImpl; +import com.android.settings.testutils.FakeFeatureFactory; import org.junit.Before; import org.junit.Test; @@ -52,11 +54,15 @@ public class ContextualWifiSliceTest { private ContentResolver mResolver; private WifiManager mWifiManager; private ContextualWifiSlice mWifiSlice; + private FakeFeatureFactory mFeatureFactory; @Before public void setUp() { mContext = spy(RuntimeEnvironment.application); mResolver = mock(ContentResolver.class); + mFeatureFactory = FakeFeatureFactory.setupForTest(); + mFeatureFactory.slicesFeatureProvider = new SlicesFeatureProviderImpl(); + mFeatureFactory.slicesFeatureProvider.newUiSession(); doReturn(mResolver).when(mContext).getContentResolver(); mWifiManager = mContext.getSystemService(WifiManager.class); @@ -65,10 +71,28 @@ public class ContextualWifiSliceTest { mWifiManager.setWifiEnabled(true); mWifiSlice = new ContextualWifiSlice(mContext); + mWifiSlice.sPreviouslyDisplayed = false; } @Test public void getWifiSlice_hasActiveConnection_shouldReturnNull() { + mWifiSlice.sPreviouslyDisplayed = false; + final WifiConfiguration config = new WifiConfiguration(); + config.SSID = "123"; + mWifiManager.connect(config, null /* listener */); + + final Slice wifiSlice = mWifiSlice.getSlice(); + + assertThat(wifiSlice).isNull(); + } + + @Test + public void getWifiSlice_newSession_hasActiveConnection_shouldReturnNull() { + // Session: use a non-active value + // previous displayed: yes + mWifiSlice.sPreviouslyDisplayed = true; + mWifiSlice.sActiveUiSession = ~mFeatureFactory.slicesFeatureProvider.getUiSessionToken(); + final WifiConfiguration config = new WifiConfiguration(); config.SSID = "123"; mWifiManager.connect(config, null /* listener */); @@ -80,7 +104,8 @@ public class ContextualWifiSliceTest { @Test public void getWifiSlice_previousDisplayed_hasActiveConnection_shouldHaveTitleAndToggle() { - mWifiSlice.mPreviouslyDisplayed = true; + mWifiSlice.sActiveUiSession = mFeatureFactory.slicesFeatureProvider.getUiSessionToken(); + mWifiSlice.sPreviouslyDisplayed = true; final WifiConfiguration config = new WifiConfiguration(); config.SSID = "123"; mWifiManager.connect(config, null /* listener */); @@ -101,7 +126,8 @@ public class ContextualWifiSliceTest { @Test public void getWifiSlice_contextualWifiSlice_shouldReturnContextualWifiSliceUri() { - mWifiSlice.mPreviouslyDisplayed = true; + mWifiSlice.sActiveUiSession = mFeatureFactory.slicesFeatureProvider.getUiSessionToken(); + mWifiSlice.sPreviouslyDisplayed = true; final Slice wifiSlice = mWifiSlice.getSlice();