From 4ebe03cd0f7590ae333b60cee486d7ce8a1568e8 Mon Sep 17 00:00:00 2001 From: Miranda Kephart Date: Tue, 28 Feb 2023 11:07:11 -0500 Subject: Consider entity length when deciding whether to show actions Currently, we show the highest-confidence action, regardless of how much space it takes up in the total copied text. This leads to some strange behaviors like showing a calendar action if a date is shown anywhere in the text. This change adds a minimum proportion (currently 80%); if the text link takes up less than that proportion of the total copied text, it is ignored. Bug: 270445121 Bug: 270421442 Fix: 270445121 Test: manual (using an article headline+date, as in the bug); atest ClipboardOverlayUtilsTest Change-Id: I99b9c48aae121244e01f8783d9eb8068d3c12fe3 --- .../ClipboardOverlayController.java | 8 +- .../clipboardoverlay/ClipboardOverlayUtils.java | 29 ++++-- .../ClipboardOverlayControllerTest.java | 2 +- .../ClipboardOverlayUtilsTest.java | 116 +++++++++++++++++++-- 4 files changed, 131 insertions(+), 24 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayController.java b/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayController.java index c214f5341450..effcc4c1c951 100644 --- a/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayController.java +++ b/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayController.java @@ -363,15 +363,15 @@ public class ClipboardOverlayController implements ClipboardListener.ClipboardOv private void classifyText(ClipboardModel model) { mBgExecutor.execute(() -> { - Optional remoteAction = mClipboardUtils.getAction( - model.getText(), model.getTextLinks(), model.getSource()); + Optional remoteAction = + mClipboardUtils.getAction(model.getTextLinks(), model.getSource()); if (model.equals(mClipboardModel)) { remoteAction.ifPresent(action -> { mClipboardLogger.logUnguarded(CLIPBOARD_OVERLAY_ACTION_SHOWN); - mView.setActionChip(action, () -> { + mView.post(() -> mView.setActionChip(action, () -> { mClipboardLogger.logSessionComplete(CLIPBOARD_OVERLAY_ACTION_TAPPED); animateOut(); - }); + })); }); } }); diff --git a/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtils.java b/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtils.java index a85f8b9357f5..25caaeac2c38 100644 --- a/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtils.java +++ b/packages/SystemUI/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtils.java @@ -39,6 +39,9 @@ import javax.inject.Inject; class ClipboardOverlayUtils { + // minimum proportion of entire text an entity must take up, to be considered for smart actions + private static final float MINIMUM_ENTITY_PROPORTION = .8f; + private final TextClassifier mTextClassifier; @Inject @@ -65,19 +68,23 @@ class ClipboardOverlayUtils { return false; } - public Optional getAction(CharSequence text, TextLinks textLinks, String source) { - return getActions(text, textLinks).stream().filter(remoteAction -> { + public Optional getAction(TextLinks textLinks, String source) { + return getActions(textLinks).stream().filter(remoteAction -> { ComponentName component = remoteAction.getActionIntent().getIntent().getComponent(); return component != null && !TextUtils.equals(source, component.getPackageName()); }).findFirst(); } - private ArrayList getActions(CharSequence text, TextLinks textLinks) { + private ArrayList getActions(TextLinks textLinks) { ArrayList actions = new ArrayList<>(); for (TextLinks.TextLink link : textLinks.getLinks()) { - TextClassification classification = mTextClassifier.classifyText( - text, link.getStart(), link.getEnd(), null); - actions.addAll(classification.getActions()); + // skip classification for incidental entities + if (link.getEnd() - link.getStart() + >= textLinks.getText().length() * MINIMUM_ENTITY_PROPORTION) { + TextClassification classification = mTextClassifier.classifyText( + textLinks.getText(), link.getStart(), link.getEnd(), null); + actions.addAll(classification.getActions()); + } } return actions; } @@ -92,9 +99,13 @@ class ClipboardOverlayUtils { private ArrayList getActions(ClipData.Item item) { ArrayList actions = new ArrayList<>(); for (TextLinks.TextLink link : item.getTextLinks().getLinks()) { - TextClassification classification = mTextClassifier.classifyText( - item.getText(), link.getStart(), link.getEnd(), null); - actions.addAll(classification.getActions()); + // skip classification for incidental entities + if (link.getEnd() - link.getStart() + >= item.getText().length() * MINIMUM_ENTITY_PROPORTION) { + TextClassification classification = mTextClassifier.classifyText( + item.getText(), link.getStart(), link.getEnd(), null); + actions.addAll(classification.getActions()); + } } return actions; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayControllerTest.java index 2099281d694a..c2fb904f64ad 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayControllerTest.java @@ -446,7 +446,7 @@ public class ClipboardOverlayControllerTest extends SysuiTestCase { mFeatureFlags.set(CLIPBOARD_REMOTE_BEHAVIOR, true); when(mClipboardUtils.isRemoteCopy(any(Context.class), any(ClipData.class), anyString())) .thenReturn(true); - when(mClipboardUtils.getAction(any(CharSequence.class), any(TextLinks.class), anyString())) + when(mClipboardUtils.getAction(any(TextLinks.class), anyString())) .thenReturn(Optional.of(Mockito.mock(RemoteAction.class))); when(mClipboardOverlayView.post(any(Runnable.class))).thenAnswer(new Answer() { @Override diff --git a/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtilsTest.java b/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtilsTest.java index aea6be3d468b..3d8f04e08825 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtilsTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/clipboardoverlay/ClipboardOverlayUtilsTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.when; @@ -77,6 +78,74 @@ public class ClipboardOverlayUtilsTest extends SysuiTestCase { @Test public void test_getAction_noLinks_returnsEmptyOptional() { + Optional action = + mClipboardUtils.getAction(Mockito.mock(TextLinks.class), "abc"); + + assertTrue(action.isEmpty()); + } + + @Test + public void test_getAction_returnsFirstLink() { + TextLinks links = getFakeTextLinksBuilder().build(); + RemoteAction actionA = constructRemoteAction("abc"); + RemoteAction actionB = constructRemoteAction("def"); + TextClassification classificationA = Mockito.mock(TextClassification.class); + when(classificationA.getActions()).thenReturn(Lists.newArrayList(actionA)); + TextClassification classificationB = Mockito.mock(TextClassification.class); + when(classificationB.getActions()).thenReturn(Lists.newArrayList(actionB)); + when(mTextClassifier.classifyText(anyString(), anyInt(), anyInt(), isNull())).thenReturn( + classificationA, classificationB); + + RemoteAction result = mClipboardUtils.getAction(links, "test").orElse(null); + + assertEquals(actionA, result); + } + + @Test + public void test_getAction_skipsMatchingComponent() { + TextLinks links = getFakeTextLinksBuilder().build(); + RemoteAction actionA = constructRemoteAction("abc"); + RemoteAction actionB = constructRemoteAction("def"); + TextClassification classificationA = Mockito.mock(TextClassification.class); + when(classificationA.getActions()).thenReturn(Lists.newArrayList(actionA)); + TextClassification classificationB = Mockito.mock(TextClassification.class); + when(classificationB.getActions()).thenReturn(Lists.newArrayList(actionB)); + when(mTextClassifier.classifyText(anyString(), anyInt(), anyInt(), isNull())).thenReturn( + classificationA, classificationB); + + RemoteAction result = mClipboardUtils.getAction(links, "abc").orElse(null); + + assertEquals(actionB, result); + } + + @Test + public void test_getAction_skipsShortEntity() { + TextLinks.Builder textLinks = new TextLinks.Builder("test text of length 22"); + final Map scores = new ArrayMap<>(); + scores.put(TextClassifier.TYPE_EMAIL, 1f); + textLinks.addLink(20, 22, scores); + textLinks.addLink(0, 22, scores); + + RemoteAction actionA = constructRemoteAction("abc"); + RemoteAction actionB = constructRemoteAction("def"); + TextClassification classificationA = Mockito.mock(TextClassification.class); + when(classificationA.getActions()).thenReturn(Lists.newArrayList(actionA)); + TextClassification classificationB = Mockito.mock(TextClassification.class); + when(classificationB.getActions()).thenReturn(Lists.newArrayList(actionB)); + when(mTextClassifier.classifyText(anyString(), eq(20), eq(22), isNull())).thenReturn( + classificationA); + when(mTextClassifier.classifyText(anyString(), eq(0), eq(22), isNull())).thenReturn( + classificationB); + + RemoteAction result = mClipboardUtils.getAction(textLinks.build(), "test").orElse(null); + + assertEquals(actionB, result); + } + + // TODO(b/267162944): Next four tests (marked "legacy") are obsolete once + // CLIPBOARD_MINIMIZED_LAYOUT flag is released and removed + @Test + public void test_getAction_noLinks_returnsEmptyOptional_legacy() { ClipData.Item item = new ClipData.Item("no text links"); item.setTextLinks(Mockito.mock(TextLinks.class)); @@ -86,8 +155,8 @@ public class ClipboardOverlayUtilsTest extends SysuiTestCase { } @Test - public void test_getAction_returnsFirstLink() { - when(mClipDataItem.getTextLinks()).thenReturn(getFakeTextLinks()); + public void test_getAction_returnsFirstLink_legacy() { + when(mClipDataItem.getTextLinks()).thenReturn(getFakeTextLinksBuilder().build()); when(mClipDataItem.getText()).thenReturn(""); RemoteAction actionA = constructRemoteAction("abc"); RemoteAction actionB = constructRemoteAction("def"); @@ -98,14 +167,14 @@ public class ClipboardOverlayUtilsTest extends SysuiTestCase { when(mTextClassifier.classifyText(anyString(), anyInt(), anyInt(), isNull())).thenReturn( classificationA, classificationB); - RemoteAction result = mClipboardUtils.getAction(mClipDataItem, "def").orElse(null); + RemoteAction result = mClipboardUtils.getAction(mClipDataItem, "test").orElse(null); assertEquals(actionA, result); } @Test - public void test_getAction_skipsMatchingComponent() { - when(mClipDataItem.getTextLinks()).thenReturn(getFakeTextLinks()); + public void test_getAction_skipsMatchingComponent_legacy() { + when(mClipDataItem.getTextLinks()).thenReturn(getFakeTextLinksBuilder().build()); when(mClipDataItem.getText()).thenReturn(""); RemoteAction actionA = constructRemoteAction("abc"); RemoteAction actionB = constructRemoteAction("def"); @@ -121,6 +190,33 @@ public class ClipboardOverlayUtilsTest extends SysuiTestCase { assertEquals(actionB, result); } + @Test + public void test_getAction_skipsShortEntity_legacy() { + TextLinks.Builder textLinks = new TextLinks.Builder("test text of length 22"); + final Map scores = new ArrayMap<>(); + scores.put(TextClassifier.TYPE_EMAIL, 1f); + textLinks.addLink(20, 22, scores); + textLinks.addLink(0, 22, scores); + + when(mClipDataItem.getTextLinks()).thenReturn(textLinks.build()); + when(mClipDataItem.getText()).thenReturn(textLinks.build().getText()); + + RemoteAction actionA = constructRemoteAction("abc"); + RemoteAction actionB = constructRemoteAction("def"); + TextClassification classificationA = Mockito.mock(TextClassification.class); + when(classificationA.getActions()).thenReturn(Lists.newArrayList(actionA)); + TextClassification classificationB = Mockito.mock(TextClassification.class); + when(classificationB.getActions()).thenReturn(Lists.newArrayList(actionB)); + when(mTextClassifier.classifyText(anyString(), eq(20), eq(22), isNull())).thenReturn( + classificationA); + when(mTextClassifier.classifyText(anyString(), eq(0), eq(22), isNull())).thenReturn( + classificationB); + + RemoteAction result = mClipboardUtils.getAction(mClipDataItem, "test").orElse(null); + + assertEquals(actionB, result); + } + @Test public void test_extra_withPackage_returnsTrue() { PersistableBundle b = new PersistableBundle(); @@ -184,12 +280,12 @@ public class ClipboardOverlayUtilsTest extends SysuiTestCase { return action; } - private static TextLinks getFakeTextLinks() { - TextLinks.Builder textLinks = new TextLinks.Builder("test"); + private static TextLinks.Builder getFakeTextLinksBuilder() { + TextLinks.Builder textLinks = new TextLinks.Builder("test text of length 22"); final Map scores = new ArrayMap<>(); scores.put(TextClassifier.TYPE_EMAIL, 1f); - textLinks.addLink(0, 0, scores); - textLinks.addLink(0, 0, scores); - return textLinks.build(); + textLinks.addLink(0, 22, scores); + textLinks.addLink(0, 22, scores); + return textLinks; } } -- cgit v1.2.3-59-g8ed1b