From cbf5ccab0ebf23a91cc3a5bb600766edb50d8089 Mon Sep 17 00:00:00 2001 From: Tsung-Mao Fang Date: Wed, 9 Jun 2021 16:09:00 +0800 Subject: [PATCH] Fix shifting problem in location services page Controller generates the injection location settings in updateState() which happens in onResume. That's the primary reason why we observed the shifting issue. In a good practice, we generate preference in displayPreference(), and then update the setting state in updateState(). In this cl, we create a base controller class to encapsulate most implementation, and developer need to inject location services in child class. Test: Add work profile, and see correct services list. Bug: 183169265 Change-Id: I5735ba974da87ad83b56791abd8a8637c2317571 --- ...jectedServiceBasePreferenceController.java | 121 ++++++++++++++++++ ...edServicesForWorkPreferenceController.java | 15 +-- ...nInjectedServicesPreferenceController.java | 89 +------------ ...ectedServicesPreferenceControllerTest.java | 30 ----- 4 files changed, 135 insertions(+), 120 deletions(-) create mode 100644 src/com/android/settings/location/LocationInjectedServiceBasePreferenceController.java diff --git a/src/com/android/settings/location/LocationInjectedServiceBasePreferenceController.java b/src/com/android/settings/location/LocationInjectedServiceBasePreferenceController.java new file mode 100644 index 00000000000..3b593b93ee9 --- /dev/null +++ b/src/com/android/settings/location/LocationInjectedServiceBasePreferenceController.java @@ -0,0 +1,121 @@ +/* + * Copyright (C) 2021 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.location; + +import android.content.BroadcastReceiver; +import android.content.Context; +import android.content.Intent; +import android.content.IntentFilter; +import android.location.SettingInjectorService; +import android.os.UserHandle; +import android.util.Log; + +import androidx.annotation.VisibleForTesting; +import androidx.lifecycle.Lifecycle; +import androidx.lifecycle.OnLifecycleEvent; +import androidx.preference.Preference; +import androidx.preference.PreferenceScreen; + +import com.android.settings.Utils; +import com.android.settings.dashboard.DashboardFragment; +import com.android.settingslib.core.lifecycle.LifecycleObserver; + +import java.util.List; +import java.util.Map; + +/** + * A abstract class which is responsible for creating the injected location services items. + * Developer needs to implement the {@link #injectLocationServices(PreferenceScreen)}. + */ +public abstract class LocationInjectedServiceBasePreferenceController + extends LocationBasePreferenceController implements LifecycleObserver{ + + private static final String TAG = "LocationPrefCtrl"; + @VisibleForTesting + static final IntentFilter INTENT_FILTER_INJECTED_SETTING_CHANGED = + new IntentFilter(SettingInjectorService.ACTION_INJECTED_SETTING_CHANGED); + + @VisibleForTesting + AppSettingsInjector mInjector; + /** Receives UPDATE_INTENT */ + @VisibleForTesting + BroadcastReceiver mInjectedSettingsReceiver; + + public LocationInjectedServiceBasePreferenceController(Context context, String key) { + super(context, key); + } + + @Override + public void init(DashboardFragment fragment) { + super.init(fragment); + mInjector = new AppSettingsInjector(mContext, getMetricsCategory()); + } + + @Override + public void displayPreference(PreferenceScreen screen) { + super.displayPreference(screen); + injectLocationServices(screen); + } + + /** + * Override this method to inject location services in preference screen. + * @param screen + */ + protected abstract void injectLocationServices(PreferenceScreen screen); + + @Override + public void onLocationModeChanged(int mode, boolean restricted) { + // As a safety measure, also reloads on location mode change to ensure the settings are + // up-to-date even if an affected app doesn't send the setting changed broadcast. + mInjector.reloadStatusMessages(); + } + + /** @OnLifecycleEvent(ON_RESUME) */ + @OnLifecycleEvent(Lifecycle.Event.ON_RESUME) + public void onResume() { + if (mInjectedSettingsReceiver == null) { + mInjectedSettingsReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "Received settings change intent: " + intent); + } + mInjector.reloadStatusMessages(); + } + }; + } + mContext.registerReceiver( + mInjectedSettingsReceiver, INTENT_FILTER_INJECTED_SETTING_CHANGED); + } + + /** @OnLifecycleEvent(ON_PAUSE) */ + @OnLifecycleEvent(Lifecycle.Event.ON_PAUSE) + public void onPause() { + mContext.unregisterReceiver(mInjectedSettingsReceiver); + } + + protected Map> getLocationServices() { + // If location access is locked down by device policy then we only show injected settings + // for the primary profile. + final int profileUserId = Utils.getManagedProfileId(mUserManager, UserHandle.myUserId()); + + return mInjector.getInjectedSettings(mFragment.getPreferenceManager().getContext(), + (profileUserId != UserHandle.USER_NULL + && mLocationEnabler.getShareLocationEnforcedAdmin(profileUserId) != null) + ? UserHandle.myUserId() : UserHandle.USER_CURRENT); + } +} diff --git a/src/com/android/settings/location/LocationInjectedServicesForWorkPreferenceController.java b/src/com/android/settings/location/LocationInjectedServicesForWorkPreferenceController.java index 571b4a79091..b7e6cf2a412 100644 --- a/src/com/android/settings/location/LocationInjectedServicesForWorkPreferenceController.java +++ b/src/com/android/settings/location/LocationInjectedServicesForWorkPreferenceController.java @@ -20,6 +20,8 @@ import android.content.Context; import android.os.UserHandle; import androidx.preference.Preference; +import androidx.preference.PreferenceCategory; +import androidx.preference.PreferenceScreen; import com.android.settings.widget.RestrictedAppPreference; @@ -30,7 +32,7 @@ import java.util.Map; * Retrieve the Location Services used in work profile user. */ public class LocationInjectedServicesForWorkPreferenceController extends - LocationInjectedServicesPreferenceController { + LocationInjectedServiceBasePreferenceController { private static final String TAG = "LocationWorkPrefCtrl"; public LocationInjectedServicesForWorkPreferenceController(Context context, String key) { @@ -38,10 +40,10 @@ public class LocationInjectedServicesForWorkPreferenceController extends } @Override - public void updateState(Preference preference) { - mCategoryLocationServices.removeAll(); + protected void injectLocationServices(PreferenceScreen screen) { + final PreferenceCategory categoryLocationServices = + screen.findPreference(getPreferenceKey()); final Map> prefs = getLocationServices(); - boolean show = false; for (Map.Entry> entry : prefs.entrySet()) { for (Preference pref : entry.getValue()) { if (pref instanceof RestrictedAppPreference) { @@ -49,11 +51,8 @@ public class LocationInjectedServicesForWorkPreferenceController extends } } if (entry.getKey() != UserHandle.myUserId()) { - LocationSettings.addPreferencesSorted(entry.getValue(), - mCategoryLocationServices); - show = true; + LocationSettings.addPreferencesSorted(entry.getValue(), categoryLocationServices); } } - mCategoryLocationServices.setVisible(show); } } diff --git a/src/com/android/settings/location/LocationInjectedServicesPreferenceController.java b/src/com/android/settings/location/LocationInjectedServicesPreferenceController.java index 85733078606..d623baef56c 100644 --- a/src/com/android/settings/location/LocationInjectedServicesPreferenceController.java +++ b/src/com/android/settings/location/LocationInjectedServicesPreferenceController.java @@ -15,25 +15,14 @@ */ package com.android.settings.location; -import android.content.BroadcastReceiver; import android.content.Context; -import android.content.Intent; -import android.content.IntentFilter; -import android.location.SettingInjectorService; import android.os.UserHandle; -import android.util.Log; -import androidx.annotation.VisibleForTesting; import androidx.preference.Preference; import androidx.preference.PreferenceCategory; import androidx.preference.PreferenceScreen; -import com.android.settings.Utils; -import com.android.settings.dashboard.DashboardFragment; import com.android.settings.widget.RestrictedAppPreference; -import com.android.settingslib.core.lifecycle.LifecycleObserver; -import com.android.settingslib.core.lifecycle.events.OnPause; -import com.android.settingslib.core.lifecycle.events.OnResume; import java.util.List; import java.util.Map; @@ -41,42 +30,20 @@ import java.util.Map; /** * Preference controller for the injected Location Services. */ -public class LocationInjectedServicesPreferenceController extends LocationBasePreferenceController - implements LifecycleObserver, OnResume, OnPause { +public class LocationInjectedServicesPreferenceController + extends LocationInjectedServiceBasePreferenceController { private static final String TAG = "LocationPrefCtrl"; - @VisibleForTesting - static final IntentFilter INTENT_FILTER_INJECTED_SETTING_CHANGED = - new IntentFilter(SettingInjectorService.ACTION_INJECTED_SETTING_CHANGED); - - protected PreferenceCategory mCategoryLocationServices; - @VisibleForTesting - AppSettingsInjector mInjector; - /** Receives UPDATE_INTENT */ - @VisibleForTesting - BroadcastReceiver mInjectedSettingsReceiver; public LocationInjectedServicesPreferenceController(Context context, String key) { super(context, key); } @Override - public void init(DashboardFragment fragment) { - super.init(fragment); - mInjector = new AppSettingsInjector(mContext, getMetricsCategory()); - } - - @Override - public void displayPreference(PreferenceScreen screen) { - super.displayPreference(screen); - mCategoryLocationServices = screen.findPreference(getPreferenceKey()); - } - - @Override - public void updateState(Preference preference) { - mCategoryLocationServices.removeAll(); + protected void injectLocationServices(PreferenceScreen screen) { + final PreferenceCategory categoryLocationServices = + screen.findPreference(getPreferenceKey()); final Map> prefs = getLocationServices(); - boolean show = false; for (Map.Entry> entry : prefs.entrySet()) { for (Preference pref : entry.getValue()) { if (pref instanceof RestrictedAppPreference) { @@ -84,53 +51,11 @@ public class LocationInjectedServicesPreferenceController extends LocationBasePr } } if (entry.getKey() == UserHandle.myUserId()) { - if (mCategoryLocationServices != null) { + if (categoryLocationServices != null) { LocationSettings.addPreferencesSorted(entry.getValue(), - mCategoryLocationServices); + categoryLocationServices); } - show = true; } } - mCategoryLocationServices.setVisible(show); - } - - @Override - public void onLocationModeChanged(int mode, boolean restricted) { - // As a safety measure, also reloads on location mode change to ensure the settings are - // up-to-date even if an affected app doesn't send the setting changed broadcast. - mInjector.reloadStatusMessages(); - } - - @Override - public void onResume() { - if (mInjectedSettingsReceiver == null) { - mInjectedSettingsReceiver = new BroadcastReceiver() { - @Override - public void onReceive(Context context, Intent intent) { - if (Log.isLoggable(TAG, Log.DEBUG)) { - Log.d(TAG, "Received settings change intent: " + intent); - } - mInjector.reloadStatusMessages(); - } - }; - } - mContext.registerReceiver( - mInjectedSettingsReceiver, INTENT_FILTER_INJECTED_SETTING_CHANGED); - } - - @Override - public void onPause() { - mContext.unregisterReceiver(mInjectedSettingsReceiver); - } - - protected Map> getLocationServices() { - // If location access is locked down by device policy then we only show injected settings - // for the primary profile. - final int profileUserId = Utils.getManagedProfileId(mUserManager, UserHandle.myUserId()); - - return mInjector.getInjectedSettings(mFragment.getPreferenceManager().getContext(), - (profileUserId != UserHandle.USER_NULL - && mLocationEnabler.getShareLocationEnforcedAdmin(profileUserId) != null) - ? UserHandle.myUserId() : UserHandle.USER_CURRENT); } } diff --git a/tests/robotests/src/com/android/settings/location/LocationInjectedServicesPreferenceControllerTest.java b/tests/robotests/src/com/android/settings/location/LocationInjectedServicesPreferenceControllerTest.java index 0370781fe77..1a7a63cd45e 100644 --- a/tests/robotests/src/com/android/settings/location/LocationInjectedServicesPreferenceControllerTest.java +++ b/tests/robotests/src/com/android/settings/location/LocationInjectedServicesPreferenceControllerTest.java @@ -67,8 +67,6 @@ public class LocationInjectedServicesPreferenceControllerTest { @Mock private PreferenceCategory mCategoryPrimary; @Mock - private PreferenceCategory mCategoryManaged; - @Mock private PreferenceScreen mScreen; @Mock private AppSettingsInjector mSettingsInjector; @@ -114,31 +112,6 @@ public class LocationInjectedServicesPreferenceControllerTest { verify(mContext).unregisterReceiver(mController.mInjectedSettingsReceiver); } - @Test - public void updateState_shouldRemoveAllAndAddInjectedSettings() { - final List preferences = new ArrayList<>(); - final Map> map = new ArrayMap<>(); - final Preference pref1 = new Preference(mContext); - pref1.setTitle("Title1"); - final Preference pref2 = new Preference(mContext); - pref2.setTitle("Title2"); - preferences.add(pref1); - preferences.add(pref2); - map.put(UserHandle.myUserId(), preferences); - doReturn(map) - .when(mSettingsInjector).getInjectedSettings(any(Context.class), anyInt()); - when(mFragment.getPreferenceManager().getContext()).thenReturn(mContext); - ShadowUserManager.getShadow().setProfileIdsWithDisabled(new int[]{UserHandle.myUserId()}); - - mController.displayPreference(mScreen); - - mController.updateState(mCategoryPrimary); - - verify(mCategoryPrimary).removeAll(); - verify(mCategoryPrimary).addPreference(pref1); - verify(mCategoryPrimary).addPreference(pref2); - } - @Test public void workProfileDisallowShareLocationOn_getParentUserLocationServicesOnly() { final int fakeWorkProfileId = 123; @@ -158,7 +131,6 @@ public class LocationInjectedServicesPreferenceControllerTest { when(mDevicePolicyManager.getDeviceOwnerComponentOnAnyUser()).thenReturn(componentName); mController.displayPreference(mScreen); - mController.updateState(mCategoryPrimary); verify(mSettingsInjector).getInjectedSettings( any(Context.class), eq(UserHandle.myUserId())); } @@ -178,7 +150,6 @@ public class LocationInjectedServicesPreferenceControllerTest { enforcingUsers); mController.displayPreference(mScreen); - mController.updateState(mCategoryPrimary); verify(mSettingsInjector).getInjectedSettings( any(Context.class), eq(UserHandle.USER_CURRENT)); } @@ -216,7 +187,6 @@ public class LocationInjectedServicesPreferenceControllerTest { when(mDevicePolicyManager.getDeviceOwnerComponentOnAnyUser()).thenReturn(componentName); mController.displayPreference(mScreen); - mController.updateState(mCategoryPrimary); assertThat(pref.isEnabled()).isFalse(); assertThat(pref.isDisabledByAdmin()).isTrue();