From 2edcb8de0cd067fe43e32e30ee3c128443509989 Mon Sep 17 00:00:00 2001 From: Matthew DeVore Date: Tue, 11 Feb 2025 21:09:48 +0000 Subject: [PATCH] Show per-displays pref groups after built-in When the built-in display settings as well as CD settings are both present in the fragment, show external display settings category after built-in. This applies to the per-display fragment and the initial display list fragment. Stop showing per-display settings nested in a parent list, as this was causing extra spacing and complicating the code. Flag: com.android.settings.flags.display_topology_pane_in_display_list Test: ExternalDisplayPreferenceFragmentTest Test: manually check topology mode for single display, multiple displays, in both fragments Test: manually check v1 UI for single display, multiple displays, in both fragments Bug: b/352648432 Bug: b/396116157 Change-Id: I7fdf72d198988feb1e7559f96a54f7680cf5b8a6 --- .../ExternalDisplayPreferenceFragment.java | 82 ++++++++++++------- ...ExternalDisplayPreferenceFragmentTest.java | 76 +++++++++++------ .../display/ExternalDisplayTestBase.java | 7 +- 3 files changed, 110 insertions(+), 55 deletions(-) diff --git a/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragment.java b/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragment.java index af03bab7579..5de96b33c80 100644 --- a/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragment.java +++ b/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragment.java @@ -59,6 +59,7 @@ import com.android.settingslib.widget.TwoTargetPreference; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import java.util.function.Consumer; /** * The Settings screen for External Displays configuration and connection management. @@ -85,8 +86,6 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen BUILTIN_DISPLAY_LIST(70, "builtin_display_list_preference", R.string.builtin_display_settings_category), - DISPLAYS_LIST(80, "displays_list_preference", null), - // If shown, footer should appear below everything. FOOTER(90, "footer_preference", null); @@ -333,15 +332,6 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen return args != null ? args.getInt(DISPLAY_ID_ARG, INVALID_DISPLAY) : INVALID_DISPLAY; } - @NonNull - private PreferenceCategory getDisplaysListPreference(@NonNull Context context) { - if (mDisplaysPreference == null) { - mDisplaysPreference = new PreferenceCategory(context); - PrefBasics.DISPLAYS_LIST.apply(mDisplaysPreference); - } - return mDisplaysPreference; - } - @NonNull private PreferenceCategory getBuiltinDisplayListPreference(@NonNull Context context) { if (mBuiltinDisplayPreference == null) { @@ -455,6 +445,26 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen EXTERNAL_DISPLAY_NOT_FOUND_FOOTER_RESOURCE)); } + private static PreferenceCategory getCategoryForDisplay(@NonNull Display display, + @NonNull PrefRefresh screen, @NonNull Context context) { + // The rest of the settings are in a category with the display name as the title. + String categoryKey = "expanded_display_items_" + display.getDisplayId(); + var category = (PreferenceCategory) screen.findUnusedPreference(categoryKey); + + if (category != null) { + screen.addPreference(category); + } else { + category = new PreferenceCategory(context); + screen.addPreference(category); + category.setPersistent(false); + category.setKey(categoryKey); + category.setTitle(display.getName()); + category.setOrder(PrefBasics.BUILTIN_DISPLAY_LIST.order + 1); + } + + return category; + } + private void showDisplaySettings(@NonNull Display display, @NonNull PrefRefresh screen, @NonNull Context context) { final var isEnabled = mInjector != null && mInjector.isDisplayEnabled(display); @@ -469,8 +479,18 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen if (!isTopologyPaneEnabled(mInjector)) { screen.addPreference(updateIllustrationImage(context, displayRotation)); } - screen.addPreference(updateResolutionPreference(context, display)); - screen.addPreference(updateRotationPreference(context, display, displayRotation)); + + Consumer adder; + if (isTopologyPaneEnabled(mInjector)) { + adder = getCategoryForDisplay(display, screen, context)::addPreference; + // The category may have already been populated if it was retrieved from the PrefRefresh + // backup, but we still need to update resolution and rotation items. + } else { + adder = screen::addPreference; + } + + adder.accept(updateResolutionPreference(context, display)); + adder.accept(updateRotationPreference(context, display, displayRotation)); if (isResolutionSettingEnabled(mInjector)) { // Do not show the footer about changing resolution affecting apps. This is not in the // UX design for v2, and there is no good place to put it, since (a) if it is on the @@ -483,12 +503,12 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen // TODO(b/352648432): probably remove footer once the pane and rest of v2 UI is in // place. if (!isTopologyPaneEnabled(mInjector)) { - screen.addPreference(updateFooterPreference(context, + adder.accept(updateFooterPreference(context, EXTERNAL_DISPLAY_CHANGE_RESOLUTION_FOOTER_RESOURCE)); } } if (isDisplaySizeSettingEnabled(mInjector)) { - screen.addPreference(updateSizePreference(context)); + adder.accept(updateSizePreference(context)); } } @@ -508,23 +528,28 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen private void showDisplaysList(@NonNull List displaysToShow, @NonNull PrefRefresh screen, @NonNull Context context) { maybeAddV2Components(context, screen); - var displayGroupPref = getDisplaysListPreference(context); - if (!displaysToShow.isEmpty()) { - screen.addPreference(displayGroupPref); - } - try (var groupCleanable = new PrefRefresh(displayGroupPref)) { - for (var display : displaysToShow) { - var pref = getDisplayPreference(context, display, groupCleanable); - pref.setSummary(display.getMode().getPhysicalWidth() + " x " - + display.getMode().getPhysicalHeight()); - } + int order = PrefBasics.BUILTIN_DISPLAY_LIST.order; + for (var display : displaysToShow) { + var pref = getDisplayPreference(context, display, screen, ++order); + pref.setSummary(display.getMode().getPhysicalWidth() + " x " + + display.getMode().getPhysicalHeight()); } } + @VisibleForTesting + static String displayListDisplayCategoryKey(int displayId) { + return "display_list_display_category_" + displayId; + } + + @VisibleForTesting + static String resolutionRotationPreferenceKey(int displayId) { + return "display_id_" + displayId; + } + private Preference getDisplayPreference(@NonNull Context context, - @NonNull Display display, @NonNull PrefRefresh groupCleanable) { - var itemKey = "display_id_" + display.getDisplayId(); - var categoryKey = itemKey + "_category"; + @NonNull Display display, @NonNull PrefRefresh groupCleanable, int categoryOrder) { + var itemKey = resolutionRotationPreferenceKey(display.getDisplayId()); + var categoryKey = displayListDisplayCategoryKey(display.getDisplayId()); var category = (PreferenceCategory) groupCleanable.findUnusedPreference(categoryKey); if (category != null) { @@ -534,6 +559,7 @@ public class ExternalDisplayPreferenceFragment extends SettingsPreferenceFragmen category = new PreferenceCategory(context); category.setPersistent(false); category.setKey(categoryKey); + category.setOrder(categoryOrder); // Must add the category to the hierarchy before adding its descendants. Otherwise // the category will not have a preference manager, which causes an exception when a // child is added to it. diff --git a/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragmentTest.java b/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragmentTest.java index 12af7726a42..623b20947cf 100644 --- a/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragmentTest.java +++ b/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayPreferenceFragmentTest.java @@ -22,6 +22,8 @@ import static com.android.settings.connecteddevice.display.ExternalDisplayPrefer import static com.android.settings.connecteddevice.display.ExternalDisplayPreferenceFragment.EXTERNAL_DISPLAY_SETTINGS_RESOURCE; import static com.android.settings.connecteddevice.display.ExternalDisplayPreferenceFragment.EXTERNAL_DISPLAY_SIZE_SUMMARY_RESOURCE; import static com.android.settings.connecteddevice.display.ExternalDisplayPreferenceFragment.PREVIOUSLY_SHOWN_LIST_KEY; +import static com.android.settings.connecteddevice.display.ExternalDisplayPreferenceFragment.displayListDisplayCategoryKey; +import static com.android.settings.connecteddevice.display.ExternalDisplayPreferenceFragment.resolutionRotationPreferenceKey; import static com.android.settings.flags.Flags.FLAG_DISPLAY_SIZE_CONNECTED_DISPLAY_SETTING; import static com.android.settings.flags.Flags.FLAG_DISPLAY_TOPOLOGY_PANE_IN_DISPLAY_LIST; import static com.android.settingslib.widget.FooterPreference.KEY_FOOTER; @@ -79,6 +81,19 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa assertThat(mPreferenceIdFromResource).isEqualTo(EXTERNAL_DISPLAY_SETTINGS_RESOURCE); } + private void assertDisplayList(boolean present, int displayId) { + // In display list fragment, there is a combined resolution/rotation preference key. + var category = mPreferenceScreen.findPreference(displayListDisplayCategoryKey(displayId)); + var pref = mPreferenceScreen.findPreference(resolutionRotationPreferenceKey(displayId)); + if (present) { + assertThat(category).isNotNull(); + assertThat(pref).isNotNull(); + } else { + assertThat(category).isNull(); + assertThat(pref).isNull(); + } + } + @Test @UiThreadTest public void testShowDisplayList() { @@ -89,19 +104,26 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa fragment.onSaveInstanceStateCallback(outState); assertThat(outState.getBoolean(PREVIOUSLY_SHOWN_LIST_KEY)).isFalse(); assertThat(mHandler.getPendingMessages().size()).isEqualTo(1); - PreferenceCategory pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(pref).isNull(); + + // Combined resolution/refresh rate are not available in displays list because the pane is + // disabled (v1 UI). + assertDisplayList(false, EXTERNAL_DISPLAY_ID); + assertDisplayList(false, OVERLAY_DISPLAY_ID); + // Individual resolution preference is not available in displays list. + assertThat(mPreferenceScreen.findPreference( + PrefBasics.EXTERNAL_DISPLAY_RESOLUTION.key)) + .isNull(); + verify(mMockedInjector, never()).getAllDisplays(); mHandler.flush(); assertThat(mHandler.getPendingMessages().size()).isEqualTo(0); verify(mMockedInjector).getAllDisplays(); - pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(pref).isNotNull(); - assertThat(pref.getPreferenceCount()).isEqualTo(2); + assertDisplayList(true, EXTERNAL_DISPLAY_ID); + assertDisplayList(true, OVERLAY_DISPLAY_ID); fragment.onSaveInstanceStateCallback(outState); assertThat(outState.getBoolean(PREVIOUSLY_SHOWN_LIST_KEY)).isTrue(); - pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAY_TOPOLOGY.key); + Preference pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAY_TOPOLOGY.key); assertThat(pref).isNull(); pref = mPreferenceScreen.findPreference(PrefBasics.BUILTIN_DISPLAY_LIST.key); @@ -122,8 +144,7 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa pref = mPreferenceScreen.findPreference(PrefBasics.MIRROR.key); assertThat(pref).isNotNull(); - pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(pref).isNull(); + assertDisplayList(false, mDisplays[1].getDisplayId()); PreferenceCategory listPref = mPreferenceScreen.findPreference(PrefBasics.BUILTIN_DISPLAY_LIST.key); @@ -148,11 +169,10 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa pref = mPreferenceScreen.findPreference(PrefBasics.MIRROR.key); assertThat(pref).isNull(); - PreferenceCategory listPref = - mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(listPref).isNull(); + assertDisplayList(false, EXTERNAL_DISPLAY_ID); + assertDisplayList(false, OVERLAY_DISPLAY_ID); - listPref = mPreferenceScreen.findPreference(PrefBasics.BUILTIN_DISPLAY_LIST.key); + var listPref = mPreferenceScreen.findPreference(PrefBasics.BUILTIN_DISPLAY_LIST.key); assertThat(listPref).isNull(); } @@ -161,19 +181,23 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa public void testLaunchDisplaySettingFromList() { initFragment(); mHandler.flush(); - PreferenceCategory pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(pref).isNotNull(); - var display1Category = (PreferenceCategory) pref.getPreference(0); + assertDisplayList(true, EXTERNAL_DISPLAY_ID); + assertDisplayList(true, OVERLAY_DISPLAY_ID); + PreferenceCategory display1Category = mPreferenceScreen.findPreference( + displayListDisplayCategoryKey(EXTERNAL_DISPLAY_ID)); var display1Pref = (DisplayPreference) display1Category.getPreference(0); - var display2Category = (PreferenceCategory) pref.getPreference(1); + PreferenceCategory display2Category = mPreferenceScreen.findPreference( + displayListDisplayCategoryKey(OVERLAY_DISPLAY_ID)); var display2Pref = (DisplayPreference) display2Category.getPreference(0); - assertThat(display1Pref.getKey()).isEqualTo("display_id_" + 1); + assertThat(display1Pref.getKey()).isEqualTo( + resolutionRotationPreferenceKey(EXTERNAL_DISPLAY_ID)); assertThat("" + display1Category.getTitle()).isEqualTo("HDMI"); assertThat("" + display1Pref.getSummary()).isEqualTo("1920 x 1080"); display1Pref.onPreferenceClick(display1Pref); assertThat(mDisplayIdArg).isEqualTo(1); verify(mMockedMetricsLogger).writePreferenceClickMetric(display1Pref); - assertThat(display2Pref.getKey()).isEqualTo("display_id_" + 2); + assertThat(display2Pref.getKey()).isEqualTo( + resolutionRotationPreferenceKey(OVERLAY_DISPLAY_ID)); assertThat("" + display2Category.getTitle()).isEqualTo("Overlay #1"); assertThat("" + display2Pref.getSummary()).isEqualTo("1240 x 780"); display2Pref.onPreferenceClick(display2Pref); @@ -190,9 +214,12 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa // Only one display available doReturn(new Display[] {mDisplays[1]}).when(mMockedInjector).getAllDisplays(); mHandler.flush(); - PreferenceCategory pref = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(pref).isNotNull(); - assertThat(pref.getPreferenceCount()).isEqualTo(1); + int attachedId = mDisplays[1].getDisplayId(); + assertDisplayList(true, attachedId); + assertThat(mPreferenceScreen.findPreference( + resolutionRotationPreferenceKey(attachedId))) + .isNotNull(); + assertDisplayList(false, mDisplays[2].getDisplayId()); } @Test @@ -205,8 +232,7 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa // Init initFragment(); mHandler.flush(); - PreferenceCategory list = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(list).isNull(); + assertDisplayList(false, mDisplays[1].getDisplayId()); var pref = mPreferenceScreen.findPreference(PrefBasics.EXTERNAL_DISPLAY_RESOLUTION.key); assertThat(pref).isNotNull(); pref = mPreferenceScreen.findPreference(PrefBasics.EXTERNAL_DISPLAY_ROTATION.key); @@ -227,8 +253,8 @@ public class ExternalDisplayPreferenceFragmentTest extends ExternalDisplayTestBa // Init initFragment(); mHandler.flush(); - PreferenceCategory list = mPreferenceScreen.findPreference(PrefBasics.DISPLAYS_LIST.key); - assertThat(list).isNull(); + assertDisplayList(false, mDisplays[1].getDisplayId()); + assertDisplayList(false, mDisplays[2].getDisplayId()); var pref = mPreferenceScreen.findPreference(PrefBasics.EXTERNAL_DISPLAY_RESOLUTION.key); assertThat(pref).isNotNull(); pref = mPreferenceScreen.findPreference(PrefBasics.EXTERNAL_DISPLAY_ROTATION.key); diff --git a/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayTestBase.java b/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayTestBase.java index ea76118c5a2..fcc3daa4efc 100644 --- a/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayTestBase.java +++ b/tests/unit/src/com/android/settings/connecteddevice/display/ExternalDisplayTestBase.java @@ -49,6 +49,9 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; public class ExternalDisplayTestBase { + static final int EXTERNAL_DISPLAY_ID = 1; + static final int OVERLAY_DISPLAY_ID = 2; + @Mock ExternalDisplaySettingsConfiguration.Injector mMockedInjector; @Mock @@ -115,7 +118,7 @@ public class ExternalDisplayTestBase { } Display createExternalDisplay() throws RemoteException { - int displayId = 1; + int displayId = EXTERNAL_DISPLAY_ID; var displayInfo = new DisplayInfo(); doReturn(displayInfo).when(mMockedIDisplayManager).getDisplayInfo(displayId); displayInfo.displayId = displayId; @@ -134,7 +137,7 @@ public class ExternalDisplayTestBase { } Display createOverlayDisplay() throws RemoteException { - int displayId = 2; + int displayId = OVERLAY_DISPLAY_ID; var displayInfo = new DisplayInfo(); doReturn(displayInfo).when(mMockedIDisplayManager).getDisplayInfo(displayId); displayInfo.displayId = displayId;