From cd7c00c36e198afee5d4db1c3d354d6b997b3c4b Mon Sep 17 00:00:00 2001 From: Andrey Epin Date: Wed, 14 Sep 2022 21:48:41 -0700 Subject: [PATCH] Load icons and labels only once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current implementation triggers icon loading each time a view gets bound and, as binding happens multiple times, icon and label loadings are unnecessarily duplicated. In addition, as a loading task never gets canceled, there’re cases (like in the associated ticket) when a view gets rapidly bound to multiple positions and all the icon loadings eventually catches up updating it, ending up in icons rapidly changing on the screen for the view. This change addresses this by simplifying the icon loading/binding logic: a view is always bound to the current data and the task is only updates the data with the missing piece of info once (and applied for both icons and label loading tasks). There is one extra change in ResolverActivity due to LoadIconTask interface change and cancellation logic for the tasks. Fix: 245934835 Test: Manual tests on a list which does not fit into the screen. Test: Manual tests with arficial delays added into the tasks. Test: atest FrameworksCoreTests:ResolverActivityTest Test: atest FrameworksCoreTests:ChooserActivityTest Change-Id: I76fe5663523232a74d4f505c535cd6dbcf7f8759 --- .../internal/app/ChooserListAdapter.java | 38 +++------ .../internal/app/ResolverActivity.java | 18 ++-- .../internal/app/ResolverListAdapter.java | 83 ++++++++++++------- .../internal/app/ResolverWrapperAdapter.java | 8 +- 4 files changed, 85 insertions(+), 62 deletions(-) diff --git a/core/java/com/android/internal/app/ChooserListAdapter.java b/core/java/com/android/internal/app/ChooserListAdapter.java index 1ec5325623ec1..4f74ca72b4aa6 100644 --- a/core/java/com/android/internal/app/ChooserListAdapter.java +++ b/core/java/com/android/internal/app/ChooserListAdapter.java @@ -86,7 +86,6 @@ public class ChooserListAdapter extends ResolverListAdapter { private final ChooserActivityLogger mChooserActivityLogger; private int mNumShortcutResults = 0; - private Map mIconLoaders = new HashMap<>(); private boolean mApplySharingAppLimits; // Reserve spots for incoming direct share targets by adding placeholders @@ -265,31 +264,20 @@ public class ChooserListAdapter extends ResolverListAdapter { return; } - if (!(info instanceof DisplayResolveInfo)) { - holder.bindLabel(info.getDisplayLabel(), info.getExtendedInfo(), alwaysShowSubLabel()); - holder.bindIcon(info); - - if (info instanceof SelectableTargetInfo) { - // direct share targets should append the application name for a better readout - DisplayResolveInfo rInfo = ((SelectableTargetInfo) info).getDisplayResolveInfo(); - CharSequence appName = rInfo != null ? rInfo.getDisplayLabel() : ""; - CharSequence extendedInfo = info.getExtendedInfo(); - String contentDescription = String.join(" ", info.getDisplayLabel(), - extendedInfo != null ? extendedInfo : "", appName); - holder.updateContentDescription(contentDescription); - } - } else { + holder.bindLabel(info.getDisplayLabel(), info.getExtendedInfo(), alwaysShowSubLabel()); + holder.bindIcon(info); + if (info instanceof SelectableTargetInfo) { + // direct share targets should append the application name for a better readout + DisplayResolveInfo rInfo = ((SelectableTargetInfo) info).getDisplayResolveInfo(); + CharSequence appName = rInfo != null ? rInfo.getDisplayLabel() : ""; + CharSequence extendedInfo = info.getExtendedInfo(); + String contentDescription = String.join(" ", info.getDisplayLabel(), + extendedInfo != null ? extendedInfo : "", appName); + holder.updateContentDescription(contentDescription); + } else if (info instanceof DisplayResolveInfo) { DisplayResolveInfo dri = (DisplayResolveInfo) info; - holder.bindLabel(dri.getDisplayLabel(), dri.getExtendedInfo(), alwaysShowSubLabel()); - LoadIconTask task = mIconLoaders.get(dri); - if (task == null) { - task = new LoadIconTask(dri, holder); - mIconLoaders.put(dri, task); - task.execute(); - } else { - // The holder was potentially changed as the underlying items were - // reshuffled, so reset the target holder - task.setViewHolder(holder); + if (!dri.hasDisplayIcon()) { + loadIcon(dri); } } diff --git a/core/java/com/android/internal/app/ResolverActivity.java b/core/java/com/android/internal/app/ResolverActivity.java index 0e1ed7bd0550c..c70e26f65829e 100644 --- a/core/java/com/android/internal/app/ResolverActivity.java +++ b/core/java/com/android/internal/app/ResolverActivity.java @@ -55,6 +55,7 @@ import android.content.pm.UserInfo; import android.content.res.Configuration; import android.content.res.TypedArray; import android.graphics.Insets; +import android.graphics.drawable.Drawable; import android.net.Uri; import android.os.Build; import android.os.Bundle; @@ -1475,14 +1476,21 @@ public class ResolverActivity extends Activity implements mMultiProfilePagerAdapter.getActiveListAdapter().mDisplayList.get(0); boolean inWorkProfile = getCurrentProfile() == PROFILE_WORK; - ResolverListAdapter inactiveAdapter = mMultiProfilePagerAdapter.getInactiveListAdapter(); - DisplayResolveInfo otherProfileResolveInfo = inactiveAdapter.mDisplayList.get(0); + final ResolverListAdapter inactiveAdapter = + mMultiProfilePagerAdapter.getInactiveListAdapter(); + final DisplayResolveInfo otherProfileResolveInfo = inactiveAdapter.mDisplayList.get(0); // Load the icon asynchronously ImageView icon = findViewById(R.id.icon); - ResolverListAdapter.LoadIconTask iconTask = inactiveAdapter.new LoadIconTask( - otherProfileResolveInfo, new ResolverListAdapter.ViewHolder(icon)); - iconTask.execute(); + inactiveAdapter.new LoadIconTask(otherProfileResolveInfo) { + @Override + protected void onPostExecute(Drawable drawable) { + if (!isDestroyed()) { + otherProfileResolveInfo.setDisplayIcon(drawable); + new ResolverListAdapter.ViewHolder(icon).bindIcon(otherProfileResolveInfo); + } + } + }.execute(); ((TextView) findViewById(R.id.open_cross_profile)).setText( getResources().getString( diff --git a/core/java/com/android/internal/app/ResolverListAdapter.java b/core/java/com/android/internal/app/ResolverListAdapter.java index 66fff5c13ab73..f6075b008f721 100644 --- a/core/java/com/android/internal/app/ResolverListAdapter.java +++ b/core/java/com/android/internal/app/ResolverListAdapter.java @@ -58,7 +58,10 @@ import com.android.internal.app.chooser.DisplayResolveInfo; import com.android.internal.app.chooser.TargetInfo; import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class ResolverListAdapter extends BaseAdapter { private static final String TAG = "ResolverListAdapter"; @@ -87,6 +90,8 @@ public class ResolverListAdapter extends BaseAdapter { private Runnable mPostListReadyRunnable; private final boolean mIsAudioCaptureDevice; private boolean mIsTabLoaded; + private final Map mIconLoaders = new HashMap<>(); + private final Map mLabelLoaders = new HashMap<>(); public ResolverListAdapter(Context context, List payloadIntents, Intent[] initialIntents, List rList, @@ -636,26 +641,47 @@ public class ResolverListAdapter extends BaseAdapter { if (info == null) { holder.icon.setImageDrawable( mContext.getDrawable(R.drawable.resolver_icon_placeholder)); + holder.bindLabel("", "", false); return; } - if (info instanceof DisplayResolveInfo - && !((DisplayResolveInfo) info).hasDisplayLabel()) { - getLoadLabelTask((DisplayResolveInfo) info, holder).execute(); - } else { - holder.bindLabel(info.getDisplayLabel(), info.getExtendedInfo(), alwaysShowSubLabel()); - } - - if (info instanceof DisplayResolveInfo - && !((DisplayResolveInfo) info).hasDisplayIcon()) { - new LoadIconTask((DisplayResolveInfo) info, holder).execute(); - } else { + if (info instanceof DisplayResolveInfo) { + DisplayResolveInfo dri = (DisplayResolveInfo) info; + boolean hasLabel = dri.hasDisplayLabel(); + holder.bindLabel( + dri.getDisplayLabel(), + dri.getExtendedInfo(), + hasLabel && alwaysShowSubLabel()); holder.bindIcon(info); + if (!hasLabel) { + loadLabel(dri); + } + if (!dri.hasDisplayIcon()) { + loadIcon(dri); + } } } - protected LoadLabelTask getLoadLabelTask(DisplayResolveInfo info, ViewHolder holder) { - return new LoadLabelTask(info, holder); + protected final void loadIcon(DisplayResolveInfo info) { + LoadIconTask task = mIconLoaders.get(info); + if (task == null) { + task = new LoadIconTask((DisplayResolveInfo) info); + mIconLoaders.put(info, task); + task.execute(); + } + } + + private void loadLabel(DisplayResolveInfo info) { + LoadLabelTask task = mLabelLoaders.get(info); + if (task == null) { + task = createLoadLabelTask(info); + mLabelLoaders.put(info, task); + task.execute(); + } + } + + protected LoadLabelTask createLoadLabelTask(DisplayResolveInfo info) { + return new LoadLabelTask(info); } public void onDestroy() { @@ -666,6 +692,16 @@ public class ResolverListAdapter extends BaseAdapter { if (mResolverListController != null) { mResolverListController.destroy(); } + cancelTasks(mIconLoaders.values()); + cancelTasks(mLabelLoaders.values()); + mIconLoaders.clear(); + mLabelLoaders.clear(); + } + + private void cancelTasks(Collection tasks) { + for (T task: tasks) { + task.cancel(false); + } } private static ColorMatrixColorFilter getSuspendedColorMatrix() { @@ -883,11 +919,9 @@ public class ResolverListAdapter extends BaseAdapter { protected class LoadLabelTask extends AsyncTask { private final DisplayResolveInfo mDisplayResolveInfo; - private final ViewHolder mHolder; - protected LoadLabelTask(DisplayResolveInfo dri, ViewHolder holder) { + protected LoadLabelTask(DisplayResolveInfo dri) { mDisplayResolveInfo = dri; - mHolder = holder; } @Override @@ -925,21 +959,22 @@ public class ResolverListAdapter extends BaseAdapter { @Override protected void onPostExecute(CharSequence[] result) { + if (mDisplayResolveInfo.hasDisplayLabel()) { + return; + } mDisplayResolveInfo.setDisplayLabel(result[0]); mDisplayResolveInfo.setExtendedInfo(result[1]); - mHolder.bindLabel(result[0], result[1], alwaysShowSubLabel()); + notifyDataSetChanged(); } } class LoadIconTask extends AsyncTask { protected final DisplayResolveInfo mDisplayResolveInfo; private final ResolveInfo mResolveInfo; - private ViewHolder mHolder; - LoadIconTask(DisplayResolveInfo dri, ViewHolder holder) { + LoadIconTask(DisplayResolveInfo dri) { mDisplayResolveInfo = dri; mResolveInfo = dri.getResolveInfo(); - mHolder = holder; } @Override @@ -953,17 +988,9 @@ public class ResolverListAdapter extends BaseAdapter { mResolverListCommunicator.updateProfileViewButton(); } else if (!mDisplayResolveInfo.hasDisplayIcon()) { mDisplayResolveInfo.setDisplayIcon(d); - mHolder.bindIcon(mDisplayResolveInfo); - // Notify in case view is already bound to resolve the race conditions on - // low end devices notifyDataSetChanged(); } } - - public void setViewHolder(ViewHolder holder) { - mHolder = holder; - mHolder.bindIcon(mDisplayResolveInfo); - } } /** diff --git a/core/tests/coretests/src/com/android/internal/app/ResolverWrapperAdapter.java b/core/tests/coretests/src/com/android/internal/app/ResolverWrapperAdapter.java index 56a70708b6dfd..2861428ece4dd 100644 --- a/core/tests/coretests/src/com/android/internal/app/ResolverWrapperAdapter.java +++ b/core/tests/coretests/src/com/android/internal/app/ResolverWrapperAdapter.java @@ -46,14 +46,14 @@ public class ResolverWrapperAdapter extends ResolverListAdapter { } @Override - protected LoadLabelTask getLoadLabelTask(DisplayResolveInfo info, ViewHolder holder) { - return new LoadLabelWrapperTask(info, holder); + protected LoadLabelTask createLoadLabelTask(DisplayResolveInfo info) { + return new LoadLabelWrapperTask(info); } class LoadLabelWrapperTask extends LoadLabelTask { - protected LoadLabelWrapperTask(DisplayResolveInfo dri, ViewHolder holder) { - super(dri, holder); + protected LoadLabelWrapperTask(DisplayResolveInfo dri) { + super(dri); } @Override