From 8c40305ad62902918015215477f81e81823fb560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Hern=C3=A1ndez?= Date: Thu, 20 Apr 2023 15:25:08 +0200 Subject: [PATCH] Tweak refresh of notification channel group preferences Previously, we were discarding and re-adding all channel preference switches when a group was toggled. This caused the group switch itself to flicker and sometimes lose the a11y focus as well. Because the group toggle is always at the top, it's not necessary to ever re-sort it (which is the main reason for doing the remove+add dance). This CL also resurrects ChannelListPreferenceControllerTest which was disabled. Fixes: 248056830 Test: atest ChannelListPreferenceControllerTest Change-Id: I5672c102d4571430e26ad6bee92d8c88794214ae --- .../app/ChannelListPreferenceController.java | 25 +++- .../ChannelListPreferenceControllerTest.java | 111 +++++++++--------- 2 files changed, 78 insertions(+), 58 deletions(-) rename tests/{legacy_unit => unit}/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java (86%) diff --git a/src/com/android/settings/notification/app/ChannelListPreferenceController.java b/src/com/android/settings/notification/app/ChannelListPreferenceController.java index 8db3b21f4be..61ee02459a6 100644 --- a/src/com/android/settings/notification/app/ChannelListPreferenceController.java +++ b/src/com/android/settings/notification/app/ChannelListPreferenceController.java @@ -236,13 +236,16 @@ public class ChannelListPreferenceController extends NotificationPreferenceContr @NonNull PreferenceGroup groupPrefGroup) { int initialPrefCount = groupPrefGroup.getPreferenceCount(); List finalOrderedPrefs = new ArrayList<>(); + Preference appDefinedGroupToggle; if (group.getId() == null) { // For the 'null' group, set the "Other" title. groupPrefGroup.setTitle(R.string.notification_channels_other); + appDefinedGroupToggle = null; } else { // For an app-defined group, set their name and create a row to toggle 'isBlocked'. groupPrefGroup.setTitle(group.getName()); - finalOrderedPrefs.add(addOrUpdateGroupToggle(groupPrefGroup, group)); + appDefinedGroupToggle = addOrUpdateGroupToggle(groupPrefGroup, group); + finalOrderedPrefs.add(appDefinedGroupToggle); } // Here "empty" means having no channel rows; the group toggle is ignored for this purpose. boolean initiallyEmpty = groupPrefGroup.getPreferenceCount() == finalOrderedPrefs.size(); @@ -268,13 +271,25 @@ public class ChannelListPreferenceController extends NotificationPreferenceContr // remove all preferences and re-add them all. // This is required to ensure proper ordering of inserted channels, and it simplifies logic // at the cost of computation in the rare case that the list is changing. + // As an optimization, keep the app-defined-group toggle. That way it doesn't "flicker" + // (due to remove+add) when toggling the group. int numFinalGroups = finalOrderedPrefs.size(); boolean hasInsertions = !initiallyEmpty && initialPrefCount != numFinalGroups; boolean requiresRemoval = postAddPrefCount != numFinalGroups; + boolean keepGroupToggle = + appDefinedGroupToggle != null && groupPrefGroup.getPreferenceCount() > 0 + && groupPrefGroup.getPreference(0) == appDefinedGroupToggle + && finalOrderedPrefs.get(0) == appDefinedGroupToggle; if (hasInsertions || requiresRemoval) { - groupPrefGroup.removeAll(); - for (Preference preference : finalOrderedPrefs) { - groupPrefGroup.addPreference(preference); + if (keepGroupToggle) { + while (groupPrefGroup.getPreferenceCount() > 1) { + groupPrefGroup.removePreference(groupPrefGroup.getPreference(1)); + } + } else { + groupPrefGroup.removeAll(); + } + for (int i = (keepGroupToggle ? 1 : 0); i < finalOrderedPrefs.size(); i++) { + groupPrefGroup.addPreference(finalOrderedPrefs.get(i)); } } } @@ -322,7 +337,7 @@ public class ChannelListPreferenceController extends NotificationPreferenceContr if (channel.getImportance() > IMPORTANCE_LOW) { channelPref.setIcon(getAlertingIcon()); } else { - channelPref.setIcon(R.drawable.empty_icon); + channelPref.setIcon(mContext.getDrawable(R.drawable.empty_icon)); } channelPref.setIconSize(PrimarySwitchPreference.ICON_SIZE_SMALL); channelPref.setTitle(channel.getName()); diff --git a/tests/legacy_unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java b/tests/unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java similarity index 86% rename from tests/legacy_unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java rename to tests/unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java index 015addc671c..4aae76cf971 100644 --- a/tests/legacy_unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java +++ b/tests/unit/src/com/android/settings/notification/app/ChannelListPreferenceControllerTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2020 The Android Open Source Project + * Copyright (C) 2023 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. @@ -22,7 +22,6 @@ import static android.app.NotificationManager.IMPORTANCE_NONE; import static junit.framework.TestCase.assertEquals; import static junit.framework.TestCase.assertFalse; -import static junit.framework.TestCase.assertNotNull; import static junit.framework.TestCase.assertNull; import static junit.framework.TestCase.assertTrue; @@ -37,6 +36,7 @@ import androidx.preference.PreferenceManager; import androidx.preference.PreferenceScreen; import androidx.preference.SwitchPreference; import androidx.test.annotation.UiThreadTest; +import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.SmallTest; import androidx.test.platform.app.InstrumentationRegistry; @@ -65,15 +65,14 @@ public class ChannelListPreferenceControllerTest { @Before public void setUp() throws Exception { + mContext = ApplicationProvider.getApplicationContext(); Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); - mContext = instrumentation.getTargetContext(); - instrumentation.runOnMainSync(() -> { mBackend = new NotificationBackend(); mAppRow = mBackend.loadAppRow(mContext, mContext.getPackageManager(), mContext.getApplicationInfo()); mController = new ChannelListPreferenceController(mContext, mBackend); - mController.onResume(mAppRow, null, null, null, null, null); + mController.onResume(mAppRow, null, null, null, null, null, null); mPreferenceManager = new PreferenceManager(mContext); mPreferenceScreen = mPreferenceManager.createPreferenceScreen(mContext); mGroupList = new PreferenceCategory(mContext); @@ -103,9 +102,10 @@ public class ChannelListPreferenceControllerTest { assertEquals("group1", group1.getKey()); assertEquals(2, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); } // Test that adding a channel works -- no dupes or omissions @@ -117,11 +117,12 @@ public class ChannelListPreferenceControllerTest { assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B", group1.getPreference(2).getTitle().toString()); } // Test that renaming a channel does in fact rename the preferences @@ -133,11 +134,12 @@ public class ChannelListPreferenceControllerTest { assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle().toString()); } // Test that adding a group works and results in the correct sorting. @@ -152,18 +154,20 @@ public class ChannelListPreferenceControllerTest { assertEquals("group0", group0.getKey()); assertEquals(2, group0.getPreferenceCount()); assertNull(group0.getPreference(0).getKey()); - assertEquals("All \"Group 0\" notifications", group0.getPreference(0).getTitle()); + assertEquals("All \"Group 0\" notifications", + group0.getPreference(0).getTitle().toString()); assertEquals("ch0b", group0.getPreference(1).getKey()); - assertEquals("Channel 0B", group0.getPreference(1).getTitle()); + assertEquals("Channel 0B", group0.getPreference(1).getTitle().toString()); PreferenceGroup group1 = (PreferenceGroup) mGroupList.getPreference(1); assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle().toString()); } // Test that adding a channel that comes before another works and has correct ordering. @@ -176,20 +180,22 @@ public class ChannelListPreferenceControllerTest { assertEquals("group0", group0.getKey()); assertEquals(3, group0.getPreferenceCount()); assertNull(group0.getPreference(0).getKey()); - assertEquals("All \"Group 0\" notifications", group0.getPreference(0).getTitle()); + assertEquals("All \"Group 0\" notifications", + group0.getPreference(0).getTitle().toString()); assertEquals("ch0a", group0.getPreference(1).getKey()); - assertEquals("Channel 0A", group0.getPreference(1).getTitle()); + assertEquals("Channel 0A", group0.getPreference(1).getTitle().toString()); assertEquals("ch0b", group0.getPreference(2).getKey()); - assertEquals("Channel 0B", group0.getPreference(2).getTitle()); + assertEquals("Channel 0B", group0.getPreference(2).getTitle().toString()); PreferenceGroup group1 = (PreferenceGroup) mGroupList.getPreference(1); assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle().toString()); } // Test that the "Other" group works. @@ -206,18 +212,19 @@ public class ChannelListPreferenceControllerTest { assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle().toString()); PreferenceGroup groupOther = (PreferenceGroup) mGroupList.getPreference(1); assertEquals("categories", groupOther.getKey()); assertEquals(2, groupOther.getPreferenceCount()); assertEquals("chXa", groupOther.getPreference(0).getKey()); - assertEquals("Other A", groupOther.getPreference(0).getTitle()); + assertEquals("Other A", groupOther.getPreference(0).getTitle().toString()); assertEquals("chXb", groupOther.getPreference(1).getKey()); - assertEquals("Other B", groupOther.getPreference(1).getTitle()); + assertEquals("Other B", groupOther.getPreference(1).getTitle().toString()); } // Test that the removal of a channel works. @@ -229,16 +236,17 @@ public class ChannelListPreferenceControllerTest { assertEquals("group1", group1.getKey()); assertEquals(3, group1.getPreferenceCount()); assertNull(group1.getPreference(0).getKey()); - assertEquals("All \"Group 1\" notifications", group1.getPreference(0).getTitle()); + assertEquals("All \"Group 1\" notifications", + group1.getPreference(0).getTitle().toString()); assertEquals("ch1a", group1.getPreference(1).getKey()); - assertEquals("Channel 1A", group1.getPreference(1).getTitle()); + assertEquals("Channel 1A", group1.getPreference(1).getTitle().toString()); assertEquals("ch1b", group1.getPreference(2).getKey()); - assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle()); + assertEquals("Channel 1B - Renamed", group1.getPreference(2).getTitle().toString()); PreferenceGroup groupOther = (PreferenceGroup) mGroupList.getPreference(1); assertEquals("categories", groupOther.getKey()); assertEquals(1, groupOther.getPreferenceCount()); assertEquals("chXb", groupOther.getPreference(0).getKey()); - assertEquals("Other B", groupOther.getPreference(0).getTitle()); + assertEquals("Other B", groupOther.getPreference(0).getTitle().toString()); } // Test that we go back to the empty state when clearing all groups and channels. @@ -269,15 +277,15 @@ public class ChannelListPreferenceControllerTest { assertEquals(3, group.getPreferenceCount()); SwitchPreference groupBlockPref = (SwitchPreference) group.getPreference(0); assertNull(groupBlockPref.getKey()); - assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle()); + assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle().toString()); assertTrue(groupBlockPref.isChecked()); PrimarySwitchPreference channelAPref = (PrimarySwitchPreference) group.getPreference(1); assertEquals("channelA", channelAPref.getKey()); - assertEquals("Channel A", channelAPref.getTitle()); + assertEquals("Channel A", channelAPref.getTitle().toString()); assertEquals(Boolean.TRUE, channelAPref.getCheckedState()); PrimarySwitchPreference channelBPref = (PrimarySwitchPreference) group.getPreference(2); assertEquals("channelB", channelBPref.getKey()); - assertEquals("Channel B", channelBPref.getTitle()); + assertEquals("Channel B", channelBPref.getTitle().toString()); assertEquals(Boolean.FALSE, channelBPref.getCheckedState()); } @@ -291,7 +299,7 @@ public class ChannelListPreferenceControllerTest { assertEquals(1, group.getPreferenceCount()); SwitchPreference groupBlockPref = (SwitchPreference) group.getPreference(0); assertNull(groupBlockPref.getKey()); - assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle()); + assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle().toString()); assertFalse(groupBlockPref.isChecked()); } @@ -305,15 +313,15 @@ public class ChannelListPreferenceControllerTest { assertEquals(3, group.getPreferenceCount()); SwitchPreference groupBlockPref = (SwitchPreference) group.getPreference(0); assertNull(groupBlockPref.getKey()); - assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle()); + assertEquals("All \"My Group\" notifications", groupBlockPref.getTitle().toString()); assertTrue(groupBlockPref.isChecked()); PrimarySwitchPreference channelAPref = (PrimarySwitchPreference) group.getPreference(1); assertEquals("channelA", channelAPref.getKey()); - assertEquals("Channel A", channelAPref.getTitle()); + assertEquals("Channel A", channelAPref.getTitle().toString()); assertEquals(Boolean.TRUE, channelAPref.getCheckedState()); PrimarySwitchPreference channelBPref = (PrimarySwitchPreference) group.getPreference(2); assertEquals("channelB", channelBPref.getKey()); - assertEquals("Channel B", channelBPref.getTitle()); + assertEquals("Channel B", channelBPref.getTitle().toString()); assertEquals(Boolean.FALSE, channelBPref.getCheckedState()); } } @@ -347,19 +355,18 @@ public class ChannelListPreferenceControllerTest { assertEquals("group", group.getKey()); assertEquals(3, group.getPreferenceCount()); assertNull(group.getPreference(0).getKey()); - assertEquals("All \"Group\" notifications", group.getPreference(0).getTitle()); + assertEquals("All \"Group\" notifications", + group.getPreference(0).getTitle().toString()); PrimarySwitchPreference channelAPref = (PrimarySwitchPreference) group.getPreference(1); assertEquals("channelA", channelAPref.getKey()); - assertEquals("Channel A", channelAPref.getTitle()); + assertEquals("Channel A", channelAPref.getTitle().toString()); assertEquals(Boolean.TRUE, channelAPref.getCheckedState()); - assertEquals("~2 notifications per day", channelAPref.getSummary()); - assertNotNull(channelAPref.getIcon()); + assertEquals("About 2 notifications per day", channelAPref.getSummary().toString()); PrimarySwitchPreference channelBPref = (PrimarySwitchPreference) group.getPreference(2); assertEquals("channelB", channelBPref.getKey()); - assertEquals("Channel B", channelBPref.getTitle()); + assertEquals("Channel B", channelBPref.getTitle().toString()); assertEquals(Boolean.FALSE, channelBPref.getCheckedState()); assertNull(channelBPref.getSummary()); - assertNull(channelBPref.getIcon()); } channelA.setImportance(IMPORTANCE_NONE); @@ -376,20 +383,18 @@ public class ChannelListPreferenceControllerTest { assertEquals("group", group.getKey()); assertEquals(3, group.getPreferenceCount()); assertNull(group.getPreference(0).getKey()); - assertEquals("All \"Group\" notifications", group.getPreference(0).getTitle()); + assertEquals("All \"Group\" notifications", + group.getPreference(0).getTitle().toString()); PrimarySwitchPreference channelAPref = (PrimarySwitchPreference) group.getPreference(1); assertEquals("channelA", channelAPref.getKey()); - assertEquals("Channel A", channelAPref.getTitle()); + assertEquals("Channel A", channelAPref.getTitle().toString()); assertEquals(Boolean.FALSE, channelAPref.getCheckedState()); assertNull(channelAPref.getSummary()); - assertNull(channelAPref.getIcon()); PrimarySwitchPreference channelBPref = (PrimarySwitchPreference) group.getPreference(2); assertEquals("channelB", channelBPref.getKey()); - assertEquals("Channel B", channelBPref.getTitle()); + assertEquals("Channel B", channelBPref.getTitle().toString()); assertEquals(Boolean.TRUE, channelBPref.getCheckedState()); - assertEquals("~2 notifications per week", channelBPref.getSummary()); - assertNotNull(channelBPref.getIcon()); + assertEquals("About 2 notifications per week", channelBPref.getSummary().toString()); } } - }