From 6e627be666fe43a9f13559856e158196d8ed7f4e Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Tue, 29 Nov 2022 13:34:21 -0500 Subject: Extract TargetPresentationGetter hierarchy These were inner classes of `ResolverListAdapter`, even though they're neither really related to the core responsibilities of that class, nor used exclusively in the implementation of that class (i.e., they're public APIs with clients in unrelated components of the app). This CL pulls the abstract base class (along with the two implementations) to a new component, and builds out comprehensive unit tests that address more edge-cases than the existing coverage in `ResolverActivityTest`. There are still opportunities to simplify the logic in the `TargetPresentationGetter` implementations, but this CL leaves the code roughly as-is so we can check in the unit test coverage in advance of any more significant refactorings. This CL: * Pulls the two implementations to static inner classes of their base class (the newly-non-inner-class) `TargetPresentationGetter`. * Introduces a new `TargetPresentationGetter.Factory` component to encapsulate the type-based overloads that were previously part of the `ResolverListAdapter` API. * Reworks client/test code to make calls in terms of the generic `TargetPresentationGetter` interface, then tightens the visibility of the subclasses (now private, instantiable only through the new `Factory` API). * Expands unit test coverage of the `TargetPresentationGetter` API, and removes the legacy tests from `ResolverActivityTest`. * Makes _some_ minor readability improvements (e.g. expanding acronyms in variable names) where convenient -- not consistently. Test: atest IntentResolverUnitTests Bug: 202167050 Change-Id: I4793f1149b698ddde8b3bbaa8bf23e0d7b962ce1 --- .../intentresolver/ChooserWrapperActivity.java | 3 +- .../android/intentresolver/IChooserWrapper.java | 3 +- .../intentresolver/ResolverActivityTest.java | 56 +----- .../intentresolver/ResolverDataProvider.java | 12 +- .../intentresolver/TargetPresentationGetterTest.kt | 204 +++++++++++++++++++++ 5 files changed, 216 insertions(+), 62 deletions(-) create mode 100644 java/tests/src/com/android/intentresolver/TargetPresentationGetterTest.kt (limited to 'java/tests') diff --git a/java/tests/src/com/android/intentresolver/ChooserWrapperActivity.java b/java/tests/src/com/android/intentresolver/ChooserWrapperActivity.java index 04e727ba..60320509 100644 --- a/java/tests/src/com/android/intentresolver/ChooserWrapperActivity.java +++ b/java/tests/src/com/android/intentresolver/ChooserWrapperActivity.java @@ -38,7 +38,6 @@ import android.util.Size; import com.android.intentresolver.AbstractMultiProfilePagerAdapter.CrossProfileIntentsChecker; import com.android.intentresolver.AbstractMultiProfilePagerAdapter.MyUserIdProvider; import com.android.intentresolver.AbstractMultiProfilePagerAdapter.QuietModeManager; -import com.android.intentresolver.ResolverListAdapter.ResolveInfoPresentationGetter; import com.android.intentresolver.chooser.DisplayResolveInfo; import com.android.intentresolver.chooser.NotSelectableTargetInfo; import com.android.intentresolver.chooser.TargetInfo; @@ -256,7 +255,7 @@ public class ChooserWrapperActivity @Override public DisplayResolveInfo createTestDisplayResolveInfo(Intent originalIntent, ResolveInfo pri, CharSequence pLabel, CharSequence pInfo, Intent replacementIntent, - @Nullable ResolveInfoPresentationGetter resolveInfoPresentationGetter) { + @Nullable TargetPresentationGetter resolveInfoPresentationGetter) { return DisplayResolveInfo.newDisplayResolveInfo( originalIntent, pri, diff --git a/java/tests/src/com/android/intentresolver/IChooserWrapper.java b/java/tests/src/com/android/intentresolver/IChooserWrapper.java index 0d44e147..af897a47 100644 --- a/java/tests/src/com/android/intentresolver/IChooserWrapper.java +++ b/java/tests/src/com/android/intentresolver/IChooserWrapper.java @@ -22,7 +22,6 @@ import android.content.Intent; import android.content.pm.ResolveInfo; import android.os.UserHandle; -import com.android.intentresolver.ResolverListAdapter.ResolveInfoPresentationGetter; import com.android.intentresolver.chooser.DisplayResolveInfo; import java.util.concurrent.Executor; @@ -40,7 +39,7 @@ public interface IChooserWrapper { UsageStatsManager getUsageStatsManager(); DisplayResolveInfo createTestDisplayResolveInfo(Intent originalIntent, ResolveInfo pri, CharSequence pLabel, CharSequence pInfo, Intent replacementIntent, - @Nullable ResolveInfoPresentationGetter resolveInfoPresentationGetter); + @Nullable TargetPresentationGetter resolveInfoPresentationGetter); UserHandle getCurrentUserHandle(); ChooserActivityLogger getChooserActivityLogger(); Executor getMainExecutor(); diff --git a/java/tests/src/com/android/intentresolver/ResolverActivityTest.java b/java/tests/src/com/android/intentresolver/ResolverActivityTest.java index 07cbd6a4..62c16ff5 100644 --- a/java/tests/src/com/android/intentresolver/ResolverActivityTest.java +++ b/java/tests/src/com/android/intentresolver/ResolverActivityTest.java @@ -27,7 +27,6 @@ import static androidx.test.espresso.matcher.ViewMatchers.withId; import static androidx.test.espresso.matcher.ViewMatchers.withText; import static com.android.intentresolver.MatcherUtils.first; -import static com.android.intentresolver.ResolverDataProvider.createPackageManagerMockedInfo; import static com.android.intentresolver.ResolverWrapperActivity.sOverrides; import static org.hamcrest.CoreMatchers.allOf; @@ -39,34 +38,25 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertFalse; import static org.testng.Assert.fail; -import android.content.Context; import android.content.Intent; import android.content.pm.ResolveInfo; import android.net.Uri; -import android.os.Bundle; -import android.os.Parcelable; import android.os.RemoteException; import android.os.UserHandle; import android.text.TextUtils; -import android.util.Log; import android.view.View; import android.widget.RelativeLayout; import android.widget.TextView; import androidx.test.InstrumentationRegistry; -import androidx.test.core.app.ActivityScenario; import androidx.test.espresso.Espresso; import androidx.test.espresso.NoMatchingViewException; -import androidx.test.ext.junit.rules.ActivityScenarioRule; import androidx.test.rule.ActivityTestRule; import androidx.test.runner.AndroidJUnit4; -import com.android.internal.R; import com.android.intentresolver.ResolverActivity.ResolvedComponentInfo; -import com.android.intentresolver.ResolverDataProvider.PackageManagerMockedInfo; -import com.android.intentresolver.ResolverListAdapter.ActivityInfoPresentationGetter; -import com.android.intentresolver.ResolverListAdapter.ResolveInfoPresentationGetter; import com.android.intentresolver.widget.ResolverDrawerLayout; +import com.android.internal.R; import org.junit.Before; import org.junit.Ignore; @@ -363,50 +353,6 @@ public class ResolverActivityTest { assertThat(chosen[0], is(toChoose)); } - @Test - public void getActivityLabelAndSubLabel() throws Exception { - ActivityInfoPresentationGetter pg; - PackageManagerMockedInfo info; - - info = createPackageManagerMockedInfo(false); - pg = new ActivityInfoPresentationGetter( - info.ctx, 0, info.activityInfo); - assertThat("Label should match app label", pg.getLabel().equals( - info.setAppLabel)); - assertThat("Sublabel should match activity label if set", - pg.getSubLabel().equals(info.setActivityLabel)); - - info = createPackageManagerMockedInfo(true); - pg = new ActivityInfoPresentationGetter( - info.ctx, 0, info.activityInfo); - assertThat("With override permission label should match activity label if set", - pg.getLabel().equals(info.setActivityLabel)); - assertThat("With override permission sublabel should be empty", - TextUtils.isEmpty(pg.getSubLabel())); - } - - @Test - public void getResolveInfoLabelAndSubLabel() throws Exception { - ResolveInfoPresentationGetter pg; - PackageManagerMockedInfo info; - - info = createPackageManagerMockedInfo(false); - pg = new ResolveInfoPresentationGetter( - info.ctx, 0, info.resolveInfo); - assertThat("Label should match app label", pg.getLabel().equals( - info.setAppLabel)); - assertThat("Sublabel should match resolve info label if set", - pg.getSubLabel().equals(info.setResolveInfoLabel)); - - info = createPackageManagerMockedInfo(true); - pg = new ResolveInfoPresentationGetter( - info.ctx, 0, info.resolveInfo); - assertThat("With override permission label should match activity label if set", - pg.getLabel().equals(info.setActivityLabel)); - assertThat("With override permission the sublabel should be the resolve info label", - pg.getSubLabel().equals(info.setResolveInfoLabel)); - } - @Test public void testWorkTab_displayedWhenWorkProfileUserAvailable() { Intent sendIntent = createSendImageIntent(); diff --git a/java/tests/src/com/android/intentresolver/ResolverDataProvider.java b/java/tests/src/com/android/intentresolver/ResolverDataProvider.java index 01d07639..fb928e09 100644 --- a/java/tests/src/com/android/intentresolver/ResolverDataProvider.java +++ b/java/tests/src/com/android/intentresolver/ResolverDataProvider.java @@ -93,11 +93,17 @@ public class ResolverDataProvider { public String setResolveInfoLabel; } + /** Create a {@link PackageManagerMockedInfo} with all distinct labels. */ static PackageManagerMockedInfo createPackageManagerMockedInfo(boolean hasOverridePermission) { - final String appLabel = "app_label"; - final String activityLabel = "activity_label"; - final String resolveInfoLabel = "resolve_info_label"; + return createPackageManagerMockedInfo( + hasOverridePermission, "app_label", "activity_label", "resolve_info_label"); + } + static PackageManagerMockedInfo createPackageManagerMockedInfo( + boolean hasOverridePermission, + String appLabel, + String activityLabel, + String resolveInfoLabel) { MockContext ctx = new MockContext() { @Override public PackageManager getPackageManager() { diff --git a/java/tests/src/com/android/intentresolver/TargetPresentationGetterTest.kt b/java/tests/src/com/android/intentresolver/TargetPresentationGetterTest.kt new file mode 100644 index 00000000..e62672a3 --- /dev/null +++ b/java/tests/src/com/android/intentresolver/TargetPresentationGetterTest.kt @@ -0,0 +1,204 @@ +/* + * Copyright (C) 2022 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.intentresolver + +import com.android.intentresolver.ResolverDataProvider +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +/** + * Unit tests for the various implementations of {@link TargetPresentationGetter}. + * TODO: consider expanding to cover icon logic (not just labels/sublabels). + * TODO: these are conceptually "acceptance tests" that provide comprehensive coverage of the + * apparent variations in the legacy implementation. The tests probably don't have to be so + * exhaustive if we're able to impose a simpler design on the implementation. + */ +class TargetPresentationGetterTest { + fun makeResolveInfoPresentationGetter( + withSubstitutePermission: Boolean, + appLabel: String, + activityLabel: String, + resolveInfoLabel: String): TargetPresentationGetter { + val testPackageInfo = ResolverDataProvider.createPackageManagerMockedInfo( + withSubstitutePermission, appLabel, activityLabel, resolveInfoLabel) + val factory = TargetPresentationGetter.Factory(testPackageInfo.ctx, 100) + return factory.makePresentationGetter(testPackageInfo.resolveInfo) + } + + fun makeActivityInfoPresentationGetter( + withSubstitutePermission: Boolean, + appLabel: String?, + activityLabel: String?): TargetPresentationGetter { + val testPackageInfo = ResolverDataProvider.createPackageManagerMockedInfo( + withSubstitutePermission, appLabel, activityLabel, "") + val factory = TargetPresentationGetter.Factory(testPackageInfo.ctx, 100) + return factory.makePresentationGetter(testPackageInfo.activityInfo) + } + + @Test + fun testActivityInfoLabels_noSubstitutePermission_distinctRequestedLabelAndSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter( + false, "app_label", "activity_label") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + assertThat(presentationGetter.getSubLabel()).isEqualTo("activity_label") + } + + @Test + fun testActivityInfoLabels_noSubstitutePermission_sameRequestedLabelAndSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter( + false, "app_label", "app_label") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // Without the substitute permission, there's no logic to dedupe the labels. + // TODO: this matches our observations in the legacy code, but is it the right behavior? It + // seems like {@link ResolverListAdapter.ViewHolder#bindLabel()} has some logic to dedupe in + // the UI at least, but maybe that logic should be pulled back to the "presentation"? + assertThat(presentationGetter.getSubLabel()).isEqualTo("app_label") + } + + @Test + fun testActivityInfoLabels_noSubstitutePermission_nullRequestedLabel() { + val presentationGetter = makeActivityInfoPresentationGetter(false, null, "activity_label") + assertThat(presentationGetter.getLabel()).isNull() + assertThat(presentationGetter.getSubLabel()).isEqualTo("activity_label") + } + + @Test + fun testActivityInfoLabels_noSubstitutePermission_emptyRequestedLabel() { + val presentationGetter = makeActivityInfoPresentationGetter(false, "", "activity_label") + assertThat(presentationGetter.getLabel()).isEqualTo("") + assertThat(presentationGetter.getSubLabel()).isEqualTo("activity_label") + } + + @Test + fun testActivityInfoLabels_noSubstitutePermission_emptyRequestedSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter(false, "app_label", "") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // Without the substitute permission, empty sublabels are passed through as-is. + assertThat(presentationGetter.getSubLabel()).isEqualTo("") + } + + @Test + fun testActivityInfoLabels_withSubstitutePermission_distinctRequestedLabelAndSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter( + true, "app_label", "activity_label") + assertThat(presentationGetter.getLabel()).isEqualTo("activity_label") + // With the substitute permission, the same ("activity") label is requested as both the label + // and sublabel, even though the other value ("app_label") was distinct. Thus this behaves the + // same as a dupe. + assertThat(presentationGetter.getSubLabel()).isEqualTo(null) + } + + @Test + fun testActivityInfoLabels_withSubstitutePermission_sameRequestedLabelAndSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter( + true, "app_label", "app_label") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // With the substitute permission, duped sublabels get converted to nulls. + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testActivityInfoLabels_withSubstitutePermission_nullRequestedLabel() { + val presentationGetter = makeActivityInfoPresentationGetter(true, "app_label", null) + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // With the substitute permission, null inputs are a special case that produces null outputs + // (i.e., they're not simply passed-through from the inputs). + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testActivityInfoLabels_withSubstitutePermission_emptyRequestedLabel() { + val presentationGetter = makeActivityInfoPresentationGetter(true, "app_label", "") + // Empty "labels" are taken as-is and (unlike nulls) don't prompt a fallback to the sublabel. + // Thus (as in the previous case with substitute permission & "distinct" labels), this is + // treated as a dupe. + assertThat(presentationGetter.getLabel()).isEqualTo("") + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testActivityInfoLabels_withSubstitutePermission_emptyRequestedSublabel() { + val presentationGetter = makeActivityInfoPresentationGetter(true, "", "activity_label") + assertThat(presentationGetter.getLabel()).isEqualTo("activity_label") + // With the substitute permission, empty sublabels get converted to nulls. + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testResolveInfoLabels_noSubstitutePermission_distinctRequestedLabelAndSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + false, "app_label", "activity_label", "resolve_info_label") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + assertThat(presentationGetter.getSubLabel()).isEqualTo("resolve_info_label") + } + + @Test + fun testResolveInfoLabels_noSubstitutePermission_sameRequestedLabelAndSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + false, "app_label", "activity_label", "app_label") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // Without the substitute permission, there's no logic to dedupe the labels. + // TODO: this matches our observations in the legacy code, but is it the right behavior? It + // seems like {@link ResolverListAdapter.ViewHolder#bindLabel()} has some logic to dedupe in + // the UI at least, but maybe that logic should be pulled back to the "presentation"? + assertThat(presentationGetter.getSubLabel()).isEqualTo("app_label") + } + + @Test + fun testResolveInfoLabels_noSubstitutePermission_emptyRequestedSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + false, "app_label", "activity_label", "") + assertThat(presentationGetter.getLabel()).isEqualTo("app_label") + // Without the substitute permission, empty sublabels are passed through as-is. + assertThat(presentationGetter.getSubLabel()).isEqualTo("") + } + + @Test + fun testResolveInfoLabels_withSubstitutePermission_distinctRequestedLabelAndSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + true, "app_label", "activity_label", "resolve_info_label") + assertThat(presentationGetter.getLabel()).isEqualTo("activity_label") + assertThat(presentationGetter.getSubLabel()).isEqualTo("resolve_info_label") + } + + @Test + fun testResolveInfoLabels_withSubstitutePermission_sameRequestedLabelAndSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + true, "app_label", "activity_label", "activity_label") + assertThat(presentationGetter.getLabel()).isEqualTo("activity_label") + // With the substitute permission, duped sublabels get converted to nulls. + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testResolveInfoLabels_withSubstitutePermission_emptyRequestedSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + true, "app_label", "activity_label", "") + assertThat(presentationGetter.getLabel()).isEqualTo("activity_label") + // With the substitute permission, empty sublabels get converted to nulls. + assertThat(presentationGetter.getSubLabel()).isNull() + } + + @Test + fun testResolveInfoLabels_withSubstitutePermission_emptyRequestedLabelAndSublabel() { + val presentationGetter = makeResolveInfoPresentationGetter( + true, "app_label", "", "") + assertThat(presentationGetter.getLabel()).isEqualTo("") + // With the substitute permission, empty sublabels get converted to nulls. + assertThat(presentationGetter.getSubLabel()).isNull() + } +} -- cgit v1.2.3-59-g8ed1b