From 7b77052941ba21afe1b0b73d7bcfe26695c392b8 Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 13 Oct 2022 11:07:33 -0400 Subject: Remove "instanceof" checks in TargetInfo clients. Clients of the various TargetInfo subclasses use runtime type checks to dispatch different behavior depending on the target type. This CL makes minimal changes to replace these checks with explicit methods on the TargetInfo API to discourage clients from making inferences based on the specific types (while we're refactoring this hierarchy). The alternative provided here doesn't really improve the quality of the existing code (because the new query methods don't convey any application semantics other than the identity of the subtype like we were already using; and because the clients still use the results of these query methods to gate downcasting to the different concrete types, so they still remain "as aware" of the subclass design as ever). Arguably it even makes the code worse (because `instanceof` is a language feature, while these new APIs are hypothetically more prone to abuse that could result in unsafe downcasts). We expect to continue addressing all of these issues in followup CLs, but it's hard for maintainers to reason about the changes we can make as long as these kinds of type-checks are spread throughout client code. Once clients are generally only implemented in terms of the base TargetInfo API, we may choose to retain the overall inheritance model as an implementation convenience. Then the requirements of clients who currently type-check and downcast for type-specific dispatch would be better addressed either by moving their behavior into polymorphic methods in the TargetInfo API, or else perhaps by some sort of Visitor pattern that restructures the API while generally leaving the clients responsible for any type-specific logic they currently own. (Tentatively we might prefer an even simpler option where all the subclass APIs are collapsed into the base TargetInfo, and then there would be no need for "type-specific" behavior; more info can be found at go/chooser-targetinfo-cleanup). Test: atest IntentResolverUnitTests Bug: 202167050 Change-Id: Ib3b3d7c4730a62243eb290a440bc0f14e4e31491 --- .../android/intentresolver/ChooserActivity.java | 32 ++++--- .../android/intentresolver/ChooserListAdapter.java | 17 ++-- .../android/intentresolver/ResolverActivity.java | 3 +- .../intentresolver/ResolverListAdapter.java | 2 +- .../intentresolver/chooser/ChooserTargetInfo.java | 33 ++++++- .../intentresolver/chooser/DisplayResolveInfo.java | 5 ++ .../chooser/MultiDisplayResolveInfo.java | 5 ++ .../chooser/NotSelectableTargetInfo.java | 5 +- .../chooser/SelectableTargetInfo.java | 7 +- .../android/intentresolver/chooser/TargetInfo.java | 100 +++++++++++++++++++++ 10 files changed, 180 insertions(+), 29 deletions(-) (limited to 'java') diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java index 36b32f6a..83d4309c 100644 --- a/java/src/com/android/intentresolver/ChooserActivity.java +++ b/java/src/com/android/intentresolver/ChooserActivity.java @@ -1726,7 +1726,7 @@ public class ChooserActivity extends ResolverActivity implements ChooserTargetActionsDialogFragment fragment = new ChooserTargetActionsDialogFragment(); Bundle bundle = new Bundle(); - if (targetInfo instanceof SelectableTargetInfo) { + if (targetInfo.isSelectableTargetInfo()) { SelectableTargetInfo selectableTargetInfo = (SelectableTargetInfo) targetInfo; if (selectableTargetInfo.getDisplayResolveInfo() == null || selectableTargetInfo.getChooserTarget() == null) { @@ -1746,7 +1746,7 @@ public class ChooserActivity extends ResolverActivity implements bundle.putString(ChooserTargetActionsDialogFragment.SHORTCUT_TITLE_KEY, selectableTargetInfo.getDisplayLabel().toString()); } - } else if (targetInfo instanceof MultiDisplayResolveInfo) { + } else if (targetInfo.isMultiDisplayResolveInfo()) { // For multiple targets, include info on all targets MultiDisplayResolveInfo mti = (MultiDisplayResolveInfo) targetInfo; targetList = mti.getTargets(); @@ -1808,13 +1808,13 @@ public class ChooserActivity extends ResolverActivity implements mChooserMultiProfilePagerAdapter.getActiveListAdapter(); TargetInfo targetInfo = currentListAdapter .targetInfoForPosition(which, filtered); - if (targetInfo != null && targetInfo instanceof NotSelectableTargetInfo) { + if (targetInfo != null && targetInfo.isNotSelectableTargetInfo()) { return; } final long selectionCost = System.currentTimeMillis() - mChooserShownTime; - if (targetInfo instanceof MultiDisplayResolveInfo) { + if (targetInfo.isMultiDisplayResolveInfo()) { MultiDisplayResolveInfo mti = (MultiDisplayResolveInfo) targetInfo; if (!mti.hasSelected()) { ChooserStackedAppDialogFragment f = new ChooserStackedAppDialogFragment(); @@ -2137,7 +2137,7 @@ public class ChooserActivity extends ResolverActivity implements } void updateModelAndChooserCounts(TargetInfo info) { - if (info != null && info instanceof MultiDisplayResolveInfo) { + if (info != null && info.isMultiDisplayResolveInfo()) { info = ((MultiDisplayResolveInfo) info).getSelectedTarget(); } if (info != null) { @@ -2171,7 +2171,7 @@ public class ChooserActivity extends ResolverActivity implements return; } // Send DS target impression info to AppPredictor, only when user chooses app share. - if (targetInfo instanceof ChooserTargetInfo) { + if (targetInfo.isChooserTargetInfo()) { return; } List surfacedTargetInfo = adapter.getSurfacedTargetInfo(); @@ -2195,7 +2195,7 @@ public class ChooserActivity extends ResolverActivity implements if (directShareAppPredictor == null) { return; } - if (!(targetInfo instanceof ChooserTargetInfo)) { + if (!targetInfo.isChooserTargetInfo()) { return; } ChooserTarget chooserTarget = ((ChooserTargetInfo) targetInfo).getChooserTarget(); @@ -2448,6 +2448,11 @@ public class ChooserActivity extends ResolverActivity implements } static final class PlaceHolderTargetInfo extends NotSelectableTargetInfo { + @Override + public boolean isPlaceHolderTargetInfo() { + return true; + } + public Drawable getDisplayIcon(Context context) { AnimatedVectorDrawable avd = (AnimatedVectorDrawable) context.getDrawable(R.drawable.chooser_direct_share_icon_placeholder); @@ -2459,6 +2464,11 @@ public class ChooserActivity extends ResolverActivity implements protected static final class EmptyTargetInfo extends NotSelectableTargetInfo { public EmptyTargetInfo() {} + @Override + public boolean isEmptyTargetInfo() { + return true; + } + public Drawable getDisplayIcon(Context context) { return null; } @@ -2956,7 +2966,7 @@ public class ChooserActivity extends ResolverActivity implements .targetInfoForPosition(mListPosition, /* filtered */ true); // This should always be the case for ItemViewHolder, check for validity - if (ti instanceof DisplayResolveInfo && shouldShowTargetDetails(ti)) { + if (ti.isDisplayResolveInfo() && shouldShowTargetDetails(ti)) { showTargetDetails((DisplayResolveInfo) ti); } return true; @@ -2970,8 +2980,8 @@ public class ChooserActivity extends ResolverActivity implements // Suppress target details for nearby share to hide pin/unpin action boolean isNearbyShare = nearbyShare != null && nearbyShare.equals( ti.getResolvedComponentName()) && shouldNearbyShareBeFirstInRankedRow(); - return ti instanceof SelectableTargetInfo - || (ti instanceof DisplayResolveInfo && !isNearbyShare); + return ti.isSelectableTargetInfo() + || (ti.isDisplayResolveInfo() && !isNearbyShare); } /** @@ -3454,7 +3464,7 @@ public class ChooserActivity extends ResolverActivity implements end--; } - if (end == start && mChooserListAdapter.getItem(start) instanceof EmptyTargetInfo) { + if (end == start && mChooserListAdapter.getItem(start).isEmptyTargetInfo()) { final TextView textView = viewGroup.findViewById(com.android.internal.R.id.chooser_row_text_option); if (textView.getVisibility() != View.VISIBLE) { diff --git a/java/src/com/android/intentresolver/ChooserListAdapter.java b/java/src/com/android/intentresolver/ChooserListAdapter.java index 5e508ed8..0a8b3890 100644 --- a/java/src/com/android/intentresolver/ChooserListAdapter.java +++ b/java/src/com/android/intentresolver/ChooserListAdapter.java @@ -268,7 +268,7 @@ public class ChooserListAdapter extends ResolverListAdapter { holder.bindLabel(info.getDisplayLabel(), info.getExtendedInfo(), alwaysShowSubLabel()); holder.bindIcon(info); - if (info instanceof SelectableTargetInfo) { + if (info.isSelectableTargetInfo()) { // direct share targets should append the application name for a better readout DisplayResolveInfo rInfo = ((SelectableTargetInfo) info).getDisplayResolveInfo(); CharSequence appName = rInfo != null ? rInfo.getDisplayLabel() : ""; @@ -276,7 +276,7 @@ public class ChooserListAdapter extends ResolverListAdapter { String contentDescription = String.join(" ", info.getDisplayLabel(), extendedInfo != null ? extendedInfo : "", appName); holder.updateContentDescription(contentDescription); - } else if (info instanceof DisplayResolveInfo) { + } else if (info.isDisplayResolveInfo()) { DisplayResolveInfo dri = (DisplayResolveInfo) info; if (!dri.hasDisplayIcon()) { loadIcon(dri); @@ -284,7 +284,7 @@ public class ChooserListAdapter extends ResolverListAdapter { } // If target is loading, show a special placeholder shape in the label, make unclickable - if (info instanceof ChooserActivity.PlaceHolderTargetInfo) { + if (info.isPlaceHolderTargetInfo()) { final int maxWidth = mContext.getResources().getDimensionPixelSize( R.dimen.chooser_direct_share_label_placeholder_max_width); holder.text.setMaxWidth(maxWidth); @@ -301,7 +301,7 @@ public class ChooserListAdapter extends ResolverListAdapter { // Always remove the spacing listener, attach as needed to direct share targets below. holder.text.removeOnLayoutChangeListener(mPinTextSpacingListener); - if (info instanceof MultiDisplayResolveInfo) { + if (info.isMultiDisplayResolveInfo()) { // If the target is grouped show an indicator Drawable bkg = mContext.getDrawable(R.drawable.chooser_group_background); holder.text.setPaddingRelative(0, 0, bkg.getIntrinsicWidth() /* end */, 0); @@ -338,7 +338,7 @@ public class ChooserListAdapter extends ResolverListAdapter { DisplayResolveInfo multiDri = consolidated.get(resolvedTarget); if (multiDri == null) { consolidated.put(resolvedTarget, info); - } else if (multiDri instanceof MultiDisplayResolveInfo) { + } else if (multiDri.isMultiDisplayResolveInfo()) { ((MultiDisplayResolveInfo) multiDri).addTarget(info); } else { // create consolidated target from the single DisplayResolveInfo @@ -387,7 +387,7 @@ public class ChooserListAdapter extends ResolverListAdapter { public int getSelectableServiceTargetCount() { int count = 0; for (ChooserTargetInfo info : mServiceTargets) { - if (info instanceof SelectableTargetInfo) { + if (info.isSelectableTargetInfo()) { count++; } } @@ -530,8 +530,7 @@ public class ChooserListAdapter extends ResolverListAdapter { @ChooserActivity.ShareTargetType int targetType, Map directShareToShortcutInfos) { // Avoid inserting any potentially late results. - if ((mServiceTargets.size() == 1) - && (mServiceTargets.get(0) instanceof ChooserActivity.EmptyTargetInfo)) { + if ((mServiceTargets.size() == 1) && mServiceTargets.get(0).isEmptyTargetInfo()) { return; } boolean isShortcutResult = targetType == TARGET_TYPE_SHORTCUTS_FROM_SHORTCUT_MANAGER @@ -579,7 +578,7 @@ public class ChooserListAdapter extends ResolverListAdapter { * update the direct share area. */ public void completeServiceTargetLoading() { - mServiceTargets.removeIf(o -> o instanceof ChooserActivity.PlaceHolderTargetInfo); + mServiceTargets.removeIf(o -> o.isPlaceHolderTargetInfo()); if (mServiceTargets.isEmpty()) { mServiceTargets.add(new ChooserActivity.EmptyTargetInfo()); mChooserActivityLogger.logSharesheetEmptyDirectShareRow(); diff --git a/java/src/com/android/intentresolver/ResolverActivity.java b/java/src/com/android/intentresolver/ResolverActivity.java index ea140dcb..a0ece9d9 100644 --- a/java/src/com/android/intentresolver/ResolverActivity.java +++ b/java/src/com/android/intentresolver/ResolverActivity.java @@ -94,7 +94,6 @@ import android.widget.Toast; import androidx.viewpager.widget.ViewPager; import com.android.intentresolver.AbstractMultiProfilePagerAdapter.Profile; -import com.android.intentresolver.chooser.ChooserTargetInfo; import com.android.intentresolver.chooser.DisplayResolveInfo; import com.android.intentresolver.chooser.TargetInfo; import com.android.intentresolver.widget.ResolverDrawerLayout; @@ -1376,7 +1375,7 @@ public class ResolverActivity extends Activity implements .createEvent(DevicePolicyEnums.RESOLVER_CROSS_PROFILE_TARGET_OPENED) .setBoolean(currentUserHandle.equals(getPersonalProfileUserHandle())) .setStrings(getMetricsCategory(), - cti instanceof ChooserTargetInfo ? "direct_share" : "other_target") + cti.isInDirectShareMetricsCategory() ? "direct_share" : "other_target") .write(); } diff --git a/java/src/com/android/intentresolver/ResolverListAdapter.java b/java/src/com/android/intentresolver/ResolverListAdapter.java index 251b157b..3c8eae3a 100644 --- a/java/src/com/android/intentresolver/ResolverListAdapter.java +++ b/java/src/com/android/intentresolver/ResolverListAdapter.java @@ -644,7 +644,7 @@ public class ResolverListAdapter extends BaseAdapter { return; } - if (info instanceof DisplayResolveInfo) { + if (info.isDisplayResolveInfo()) { DisplayResolveInfo dri = (DisplayResolveInfo) info; boolean hasLabel = dri.hasDisplayLabel(); holder.bindLabel( diff --git a/java/src/com/android/intentresolver/chooser/ChooserTargetInfo.java b/java/src/com/android/intentresolver/chooser/ChooserTargetInfo.java index 1c763071..77c30102 100644 --- a/java/src/com/android/intentresolver/chooser/ChooserTargetInfo.java +++ b/java/src/com/android/intentresolver/chooser/ChooserTargetInfo.java @@ -16,6 +16,7 @@ package com.android.intentresolver.chooser; +import android.annotation.Nullable; import android.service.chooser.ChooserTarget; import android.text.TextUtils; @@ -23,16 +24,40 @@ import android.text.TextUtils; * A TargetInfo for Direct Share. Includes a {@link ChooserTarget} representing the * Direct Share deep link into an application. */ -public interface ChooserTargetInfo extends TargetInfo { - float getModifiedScore(); +public abstract class ChooserTargetInfo implements TargetInfo { + /** + * @return the target score, including any Chooser-specific modifications that may have been + * applied (either overriding by special-case for "non-selectable" targets, or by twiddling the + * scores of "selectable" targets in {@link ChooserListAdapter}). Higher scores are "better." + */ + public abstract float getModifiedScore(); + + /** + * @return the {@link ChooserTarget} record that contains additional data about this target, if + * any. This is only non-null for selectable targets (and probably only Direct Share targets?). + * + * @deprecated {@link ChooserTarget} (and any other related {@code ChooserTargetService} APIs) + * got deprecated as part of sunsetting that old system design, but for historical reasons + * Chooser continues to shoehorn data from other sources into this representation to maintain + * compatibility with legacy internal APIs. New clients should avoid taking any further + * dependencies on the {@link ChooserTarget} type; any data they want to query from those + * records should instead be pulled up to new query methods directly on this class (or on the + * root {@link TargetInfo}). + */ + @Deprecated + @Nullable + public abstract ChooserTarget getChooserTarget(); - ChooserTarget getChooserTarget(); + @Override + public final boolean isChooserTargetInfo() { + return true; + } /** * Do not label as 'equals', since this doesn't quite work * as intended with java 8. */ - default boolean isSimilar(ChooserTargetInfo other) { + public boolean isSimilar(ChooserTargetInfo other) { if (other == null) return false; ChooserTarget ct1 = getChooserTarget(); diff --git a/java/src/com/android/intentresolver/chooser/DisplayResolveInfo.java b/java/src/com/android/intentresolver/chooser/DisplayResolveInfo.java index c4bca266..8f950323 100644 --- a/java/src/com/android/intentresolver/chooser/DisplayResolveInfo.java +++ b/java/src/com/android/intentresolver/chooser/DisplayResolveInfo.java @@ -100,6 +100,11 @@ public class DisplayResolveInfo implements TargetInfo, Parcelable { mResolveInfoPresentationGetter = other.mResolveInfoPresentationGetter; } + @Override + public final boolean isDisplayResolveInfo() { + return true; + } + public ResolveInfo getResolveInfo() { return mResolveInfo; } diff --git a/java/src/com/android/intentresolver/chooser/MultiDisplayResolveInfo.java b/java/src/com/android/intentresolver/chooser/MultiDisplayResolveInfo.java index 5133d997..e1fe58fd 100644 --- a/java/src/com/android/intentresolver/chooser/MultiDisplayResolveInfo.java +++ b/java/src/com/android/intentresolver/chooser/MultiDisplayResolveInfo.java @@ -44,6 +44,11 @@ public class MultiDisplayResolveInfo extends DisplayResolveInfo { mTargetInfos.add(firstInfo); } + @Override + public final boolean isMultiDisplayResolveInfo() { + return true; + } + @Override public CharSequence getExtendedInfo() { // Never show subtitle for stacked apps diff --git a/java/src/com/android/intentresolver/chooser/NotSelectableTargetInfo.java b/java/src/com/android/intentresolver/chooser/NotSelectableTargetInfo.java index 220870f2..db03b785 100644 --- a/java/src/com/android/intentresolver/chooser/NotSelectableTargetInfo.java +++ b/java/src/com/android/intentresolver/chooser/NotSelectableTargetInfo.java @@ -32,7 +32,10 @@ import java.util.List; * Distinguish between targets that selectable by the user, vs those that are * placeholders for the system while information is loading in an async manner. */ -public abstract class NotSelectableTargetInfo implements ChooserTargetInfo { +public abstract class NotSelectableTargetInfo extends ChooserTargetInfo { + public final boolean isNotSelectableTargetInfo() { + return true; + } public Intent getResolvedIntent() { return null; diff --git a/java/src/com/android/intentresolver/chooser/SelectableTargetInfo.java b/java/src/com/android/intentresolver/chooser/SelectableTargetInfo.java index 179966ad..0f48cfd1 100644 --- a/java/src/com/android/intentresolver/chooser/SelectableTargetInfo.java +++ b/java/src/com/android/intentresolver/chooser/SelectableTargetInfo.java @@ -50,7 +50,7 @@ import java.util.List; * Live target, currently selectable by the user. * @see NotSelectableTargetInfo */ -public final class SelectableTargetInfo implements ChooserTargetInfo { +public final class SelectableTargetInfo extends ChooserTargetInfo { private static final String TAG = "SelectableTargetInfo"; private final Context mContext; @@ -134,6 +134,11 @@ public final class SelectableTargetInfo implements ChooserTargetInfo { mDisplayLabel = sanitizeDisplayLabel(mChooserTarget.getTitle()); } + @Override + public boolean isSelectableTargetInfo() { + return true; + } + private String sanitizeDisplayLabel(CharSequence label) { SpannableStringBuilder sb = new SpannableStringBuilder(label); sb.clearSpans(); diff --git a/java/src/com/android/intentresolver/chooser/TargetInfo.java b/java/src/com/android/intentresolver/chooser/TargetInfo.java index e1970354..502a3342 100644 --- a/java/src/com/android/intentresolver/chooser/TargetInfo.java +++ b/java/src/com/android/intentresolver/chooser/TargetInfo.java @@ -131,6 +131,106 @@ public interface TargetInfo { */ boolean isPinned(); + /** + * @return true if this target represents a legacy {@code ChooserTargetInfo}. These objects were + * historically documented as representing "[a] TargetInfo for Direct Share." However, not all + * of these targets are actually *valid* for direct share; e.g. some represent "empty" items + * (although perhaps only for display in the Direct Share UI?). {@link #getChooserTarget()} will + * return null for any of these "invalid" items. In even earlier versions, these targets may + * also have been results from (now-deprecated/unsupported) {@code ChooserTargetService} peers; + * even though we no longer use these services, we're still shoehorning other target data into + * the deprecated {@link ChooserTarget} structure for compatibility with some internal APIs. + * TODO: refactor to clarify the semantics of any target for which this method returns true + * (e.g., are they characterized by their application in the Direct Share UI?), and to remove + * the scaffolding that adapts to and from the {@link ChooserTarget} structure. Eventually, we + * expect to remove this method (and others that strictly indicate legacy subclass roles) in + * favor of a more semantic design that expresses the purpose and distinctions in those roles. + */ + default boolean isChooserTargetInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code DisplayResolveInfo}. These objects + * were historically documented as an augmented "TargetInfo plus additional information needed + * to render it (such as icon and label) and resolve it to an activity." That description in no + * way distinguishes from the base {@code TargetInfo} API. At the time of writing, these objects + * are most-clearly defined by their opposite; this returns true for exactly those instances of + * {@code TargetInfo} where {@link #isChooserTargetInfo()} returns false (these conditions are + * complementary because they correspond to the immediate {@code TargetInfo} child types that + * historically partitioned all concrete {@code TargetInfo} implementations). These may(?) + * represent any target displayed somewhere other than the Direct Share UI. + */ + default boolean isDisplayResolveInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code MultiDisplayResolveInfo}. These + * objects were historically documented as representing "a 'stack' of chooser targets for + * various activities within the same component." For historical reasons this currently can + * return true only if {@link #isDisplayResolveInfo()} returns true (because the legacy classes + * shared an inheritance relationship), but new code should avoid relying on that relationship + * since these APIs are "in transition." + */ + default boolean isMultiDisplayResolveInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code SelectableTargetInfo}. Note that this + * is defined for legacy compatibility and may not conform to other notions of a "selectable" + * target. For historical reasons, this method and {@link #isNotSelectableTargetInfo()} only + * partition the {@code TargetInfo} instances for which {@link #isChooserTargetInfo()} returns + * true; otherwise both methods return false. + * TODO: define selectability for targets not historically from {@code ChooserTargetInfo}, + * then attempt to replace this with a new method like {@code TargetInfo#isSelectable()} that + * actually partitions all target types (after updating client usage as needed). + */ + default boolean isSelectableTargetInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code NotSelectableTargetInfo} (i.e., a + * target where {@link #isChooserTargetInfo()} is true but {@link #isSelectableTargetInfo()} is + * false). For more information on how this divides the space of targets, see the Javadoc for + * {@link #isSelectableTargetInfo()}. + */ + default boolean isNotSelectableTargetInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code ChooserActivity#EmptyTargetInfo}. Note + * that this is defined for legacy compatibility and may not conform to other notions of an + * "empty" target. + */ + default boolean isEmptyTargetInfo() { + return false; + } + + /** + * @return true if this target represents a legacy {@code ChooserActivity#PlaceHolderTargetInfo} + * (defined only for compatibility with historic use in {@link ChooserListAdapter}). For + * historic reasons (owing to a legacy subclass relationship) this can return true only if + * {@link #isNotSelectableTargetInfo()} also returns true. + */ + default boolean isPlaceHolderTargetInfo() { + return false; + } + + /** + * @return true if this target should be logged with the "direct_share" metrics category in + * {@link ResolverActivity#maybeLogCrossProfileTargetLaunch()}. This is defined for legacy + * compatibility and is not likely to be a good indicator of whether this is actually a + * "direct share" target (e.g. because it historically also applies to "empty" and "placeholder" + * targets). + */ + default boolean isInDirectShareMetricsCategory() { + return isChooserTargetInfo(); + } + /** * Fix the URIs in {@code intent} if cross-profile sharing is required. This should be called * before launching the intent as another user. -- cgit v1.2.3-59-g8ed1b