From 91c10ea1620ee887b477375d7a804b9e5bffac3f Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 14 Dec 2023 17:42:55 +0000 Subject: Treat all inactive profiles equally Now that we've handled all the "profile-specific" special cases in ag/25599173, remaining references to a generic "inactive profile" can be adapted to apply to *all* inactive profiles, since in the future we may have more than one. These changes are as prototyped in ag/25335069 and described in go/chooser-ntab-refactoring. This CL covers the original "snapshot 20" through "snapshot 25." See below for a "by-snapshot" breakdown of the incremental changes composed in this CL. Snapshot 1: Establish `forEachPage()` and `forEachInactivePage()` helpers that we'll use to implement per-page operations throughout the changes included in this CL. Convert a first operation, the Resolver-specific `clearCheckedItemsInInactiveProfiles()`. Snapshot 2: Generalize the `shouldShowEmptyStateScreenInAnyInactiveAdapter()` implementation (so it in fact considers "any" inactive adapter). Snapshot 3: Generalize `refreshPackagesInAllTabs()` to handle any number of tabs. Snapshot 4: Generalize `rebuildInactiveTab()` -> `rebuildInactiveTabs()`. This relies on some imagination about how the legacy intention might apply to multiple tabs, but it's at least equivalent in the legacy two-tab case. Note this implementation may appear to differ from the legacy code by returning true instead of false when there are no inactive tabs, but it's a vacuous distinction because the legacy caller effectively checked for a tabbed presentation as a precondition for calling this method. Returning true by default is more appropriate if we relax that precondition, so we wouldn't end up waiting in an "incomplete" status on behalf of a tab that doesn't exist. Snapshot 5: Remove the API to access some single "inactive tab" -- there may be more than one in the future. Snapshot 6: Generalize `clearInactiveProfileCache()`. Bug: 310211468 Test: `ResolverActivityTest` & IntentResolver activity tests. Change-Id: Ifcba88bb3d678364a55771a9801f3626dfbfd25a --- .../v2/MultiProfilePagerAdapter.java | 103 ++++++++++----------- .../v2/ResolverMultiProfilePagerAdapter.java | 12 ++- 2 files changed, 57 insertions(+), 58 deletions(-) (limited to 'java/src/com') diff --git a/java/src/com/android/intentresolver/v2/MultiProfilePagerAdapter.java b/java/src/com/android/intentresolver/v2/MultiProfilePagerAdapter.java index aa161921..6a286f21 100644 --- a/java/src/com/android/intentresolver/v2/MultiProfilePagerAdapter.java +++ b/java/src/com/android/intentresolver/v2/MultiProfilePagerAdapter.java @@ -36,6 +36,8 @@ import com.google.common.collect.ImmutableList; import java.util.HashSet; import java.util.Optional; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; @@ -184,10 +186,7 @@ public class MultiProfilePagerAdapter< } public void clearInactiveProfileCache() { - if (mLoadedPages.size() == 1) { - return; - } - mLoadedPages.remove(1 - mCurrentPage); + forEachInactivePage(pageNumber -> mLoadedPages.remove(pageNumber)); } @Override @@ -317,30 +316,12 @@ public class MultiProfilePagerAdapter< * to the user. *

For example, if the user is viewing the work tab in the share sheet, this method returns * the work profile {@link ListAdapterT}. - * @see #getInactiveListAdapter() */ @VisibleForTesting public final ListAdapterT getActiveListAdapter() { return getListAdapterForPageNumber(getCurrentPage()); } - /** - * If this is a device with a work profile, returns the {@link ListAdapterT} instance - * of the profile that is not currently visible to the user. Otherwise returns - * {@code null}. - *

For example, if the user is viewing the work tab in the share sheet, this method returns - * the personal profile {@link ListAdapterT}. - * @see #getActiveListAdapter() - */ - @VisibleForTesting - @Nullable - public final ListAdapterT getInactiveListAdapter() { - if (getCount() < 2) { - return null; - } - return getListAdapterForPageNumber(1 - getCurrentPage()); - } - public final ListAdapterT getPersonalListAdapter() { return getListAdapterForPageNumber(getPageNumberForProfile(PROFILE_PERSONAL)); } @@ -366,14 +347,6 @@ public class MultiProfilePagerAdapter< return getListViewForIndex(getCurrentPage()); } - @Nullable - public final PageViewT getInactiveAdapterView() { - if (getCount() < 2) { - return null; - } - return getListViewForIndex(1 - getCurrentPage()); - } - private boolean anyAdapterHasItems() { for (int i = 0; i < mItems.size(); ++i) { ListAdapterT listAdapter = mListAdapterExtractor.apply(getAdapterForIndex(i)); @@ -385,13 +358,10 @@ public class MultiProfilePagerAdapter< } public void refreshPackagesInAllTabs() { - // TODO: handle all inactive profiles; for now we can only have at most one. It's unclear if - // this legacy logic really requires the active tab to be rebuilt first, or if we could just - // iterate over the tabs in arbitrary order. + // TODO: it's unclear if this legacy logic really requires the active tab to be rebuilt + // first, or if we could just iterate over the tabs in arbitrary order. getActiveListAdapter().handlePackagesChanged(); - if (getCount() > 1) { - getInactiveListAdapter().handlePackagesChanged(); - } + forEachInactivePage(page -> getListAdapterForPageNumber(page).handlePackagesChanged()); } /** @@ -449,9 +419,10 @@ public class MultiProfilePagerAdapter< // autolaunch conditions). boolean rebuildCompleted = rebuildActiveTab(true) || getActiveListAdapter().isTabLoaded(); if (includePartialRebuildOfInactiveTabs) { - boolean rebuildInactiveCompleted = - rebuildInactiveTab(false) || getInactiveListAdapter().isTabLoaded(); - rebuildCompleted = rebuildCompleted && rebuildInactiveCompleted; + // Per legacy logic, avoid short-circuiting (TODO: why? possibly so that we *start* + // loading the inactive tabs even if we're still waiting on the active tab to finish?). + boolean completedRebuildingInactiveTabs = rebuildInactiveTabs(false); + rebuildCompleted = rebuildCompleted && completedRebuildingInactiveTabs; } return rebuildCompleted; } @@ -468,18 +439,27 @@ public class MultiProfilePagerAdapter< } /** - * Rebuilds the tab that is not currently visible to the user, if such one exists. - *

Returns {@code true} if rebuild has completed. + * Rebuilds any tabs that are not currently visible to the user. + *

Returns {@code true} if rebuild has completed in all inactive tabs. */ - private boolean rebuildInactiveTab(boolean doPostProcessing) { + private boolean rebuildInactiveTabs(boolean doPostProcessing) { Trace.beginSection("MultiProfilePagerAdapter#rebuildInactiveTab"); - if (getItemCount() == 1) { - Trace.endSection(); - return false; - } - boolean result = rebuildTab(getInactiveListAdapter(), doPostProcessing); + AtomicBoolean allRebuildsComplete = new AtomicBoolean(true); + forEachInactivePage(pageNumber -> { + // Evaluate the rebuild for every inactive page, even if we've already seen some adapter + // return an "incomplete" status (i.e., even if `allRebuildsComplete` is already false) + // and so we already know we'll end up returning false for the batch. + // TODO: any particular reason the per-page legacy logic was set up in this order, or + // could we possibly short-circuit the rebuild if the tab is already "loaded"? + ListAdapterT inactiveAdapter = getListAdapterForPageNumber(pageNumber); + boolean rebuildInactivePageCompleted = + rebuildTab(inactiveAdapter, doPostProcessing) || inactiveAdapter.isTabLoaded(); + if (!rebuildInactivePageCompleted) { + allRebuildsComplete.set(false); + } + }); Trace.endSection(); - return result; + return allRebuildsComplete.get(); } private int userHandleToPageIndex(UserHandle userHandle) { @@ -490,6 +470,20 @@ public class MultiProfilePagerAdapter< } } + protected void forEachPage(Consumer pageNumberHandler) { + for (int pageNumber = 0; pageNumber < getItemCount(); ++pageNumber) { + pageNumberHandler.accept(pageNumber); + } + } + + protected void forEachInactivePage(Consumer inactivePageNumberHandler) { + forEachPage(pageNumber -> { + if (pageNumber != getCurrentPage()) { + inactivePageNumberHandler.accept(pageNumber); + } + }); + } + protected boolean rebuildTab(ListAdapterT activeListAdapter, boolean doPostProcessing) { if (shouldSkipRebuild(activeListAdapter)) { activeListAdapter.postListReadyRunnable(doPostProcessing, /* rebuildCompleted */ true); @@ -585,11 +579,14 @@ public class MultiProfilePagerAdapter< * application state. */ public final boolean shouldShowEmptyStateScreenInAnyInactiveAdapter() { - if (getCount() < 2) { - return false; - } - // TODO: check against *any* inactive adapter; for now we only have one. - return shouldShowEmptyStateScreen(getInactiveListAdapter()); + AtomicBoolean anyEmpty = new AtomicBoolean(false); + // TODO: The "inactive" condition is legacy logic. Could we simplify and ask "any"? + forEachInactivePage(pageNumber -> { + if (shouldShowEmptyStateScreen(getListAdapterForPageNumber(pageNumber))) { + anyEmpty.set(true); + } + }); + return anyEmpty.get(); } public boolean shouldShowEmptyStateScreen(ListAdapterT listAdapter) { diff --git a/java/src/com/android/intentresolver/v2/ResolverMultiProfilePagerAdapter.java b/java/src/com/android/intentresolver/v2/ResolverMultiProfilePagerAdapter.java index d96fd15a..21e36614 100644 --- a/java/src/com/android/intentresolver/v2/ResolverMultiProfilePagerAdapter.java +++ b/java/src/com/android/intentresolver/v2/ResolverMultiProfilePagerAdapter.java @@ -109,11 +109,13 @@ public class ResolverMultiProfilePagerAdapter extends /** Un-check any item(s) that may be checked in any of our inactive adapter(s). */ public void clearCheckedItemsInInactiveProfiles() { - // TODO: apply to all inactive adapters; for now we just have the one. - ListView inactiveListView = getInactiveAdapterView(); - if (inactiveListView.getCheckedItemCount() > 0) { - inactiveListView.setItemChecked(inactiveListView.getCheckedItemPosition(), false); - } + // TODO: The "inactive" condition is legacy logic. Could we simplify and clear-all? + forEachInactivePage(pageNumber -> { + ListView inactiveListView = getListViewForIndex(pageNumber); + if (inactiveListView.getCheckedItemCount() > 0) { + inactiveListView.setItemChecked(inactiveListView.getCheckedItemPosition(), false); + } + }); } private static class BottomPaddingOverrideSupplier implements Supplier> { -- cgit v1.2.3-59-g8ed1b