Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c132a228 authored by Abodunrinwa Toki's avatar Abodunrinwa Toki
Browse files

Ensure the toolbar refreshes its PendingIntents when it changes. 

A bug in FloatingToolbar.isCurrentlyShowing(menuItems) caused the
toolbar not to update the menu item action in certain cases where the
text selection changed but the toolbar menu items did not change.
e.g. Fires a Translate PendingIntent with the wrong text selection.
This CL fixes that.

isCurrentlyShowing(menuItems) aims to help avoid the re-building of
toolbar views when unnecessary. This fix still maintains this idea
but ensures that the menuItem references are updated so that the
correct click listener is triggered when the menu item button is
clicked.

Bug: 150973392
Test: Manually tested. See bug for how to reproduce.
Test: atest android.widget.TextViewActivityTest
Test: atest android.view.textclassifier.cts.TextViewIntegrationTest
Test: atest android.widget.cts.TextViewTest

Change-Id: Iefedf40e1ea6f9499401918ab34d3ebac53fc385
parent d9e62672

core/java/com/android/internal/widget/FloatingToolbar.java

+125 −34
Original line number Diff line number Diff line
@@ -61,12 +61,17 @@ import android.widget.PopupWindow; 
import android.widget.TextView;



import com.android.internal.R;

import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.util.Preconditions;



import java.util.ArrayList;

import java.util.Collection;

import java.util.Comparator;

import java.util.Iterator;

import java.util.LinkedHashMap;

import java.util.LinkedList;

import java.util.List;

import java.util.Map;

import java.util.Objects;



/**
@@ -93,7 +98,6 @@ public final class FloatingToolbar { 
    private final Rect mPreviousContentRect = new Rect();



    private Menu mMenu;

    private List<MenuItem> mShowingMenuItems = new ArrayList<>();

    private MenuItem.OnMenuItemClickListener mMenuItemClickListener = NO_OP_MENUITEM_CLICK_LISTENER;



    private int mSuggestedWidth;
@@ -274,10 +278,11 @@ public final class FloatingToolbar { 
    private void doShow() {

        List<MenuItem> menuItems = getVisibleAndEnabledMenuItems(mMenu);

        menuItems.sort(mMenuItemComparator);

        if (!isCurrentlyShowing(menuItems) || mWidthChanged) {

        if (mPopup.isLayoutRequired(menuItems) || mWidthChanged) {

            mPopup.dismiss();

            mPopup.layoutMenuItems(menuItems, mMenuItemClickListener, mSuggestedWidth);

            mShowingMenuItems = menuItems;

        } else {

            mPopup.updateMenuItems(menuItems, mMenuItemClickListener);

        }

        if (!mPopup.isShowing()) {

            mPopup.show(mContentRect);
@@ -288,34 +293,11 @@ public final class FloatingToolbar { 
        mPreviousContentRect.set(mContentRect);

    }



    /**

     * Returns true if this floating toolbar is currently showing the specified menu items.

     */

    private boolean isCurrentlyShowing(List<MenuItem> menuItems) {

        if (mShowingMenuItems == null || menuItems.size() != mShowingMenuItems.size()) {

            return false;

        }



        final int size = menuItems.size();

        for (int i = 0; i < size; i++) {

            final MenuItem menuItem = menuItems.get(i);

            final MenuItem showingItem = mShowingMenuItems.get(i);

            if (menuItem.getItemId() != showingItem.getItemId()

                    || !TextUtils.equals(menuItem.getTitle(), showingItem.getTitle())

                    || !Objects.equals(menuItem.getIcon(), showingItem.getIcon())

                    || menuItem.getGroupId() != showingItem.getGroupId()) {

                return false;

            }

        }



        return true;

    }



    /**

     * Returns the visible and enabled menu items in the specified menu.

     * This method is recursive.

     */

    private List<MenuItem> getVisibleAndEnabledMenuItems(Menu menu) {

    private static List<MenuItem> getVisibleAndEnabledMenuItems(Menu menu) {

        List<MenuItem> menuItems = new ArrayList<>();

        for (int i = 0; (menu != null) && (i < menu.size()); i++) {

            MenuItem menuItem = menu.getItem(i);
@@ -427,17 +409,25 @@ public final class FloatingToolbar { 
        private Size mOverflowPanelSize;  // Should be null when there is no overflow.

        private Size mMainPanelSize;



        /* Item click listeners */

        /* Menu items and click listeners */

        private final Map<MenuItemRepr, MenuItem> mMenuItems = new LinkedHashMap<>();

        private MenuItem.OnMenuItemClickListener mOnMenuItemClickListener;

        private final View.OnClickListener mMenuItemButtonOnClickListener =

                new View.OnClickListener() {

                    @Override

                    public void onClick(View v) {

                        if (v.getTag() instanceof MenuItem) {

                            if (mOnMenuItemClickListener != null) {

                                mOnMenuItemClickListener.onMenuItemClick((MenuItem) v.getTag());

                        if (mOnMenuItemClickListener == null) {

                            return;

                        }

                        final Object tag = v.getTag();

                        if (!(tag instanceof MenuItemRepr)) {

                            return;

                        }

                        final MenuItem menuItem = mMenuItems.get((MenuItemRepr) tag);

                        if (menuItem == null) {

                            return;

                        }

                        mOnMenuItemClickListener.onMenuItemClick(menuItem);

                    }

                };
@@ -558,9 +548,9 @@ public final class FloatingToolbar { 
                List<MenuItem> menuItems,

                MenuItem.OnMenuItemClickListener menuItemClickListener,

                int suggestedWidth) {

            mOnMenuItemClickListener = menuItemClickListener;

            cancelOverflowAnimations();

            clearPanels();

            updateMenuItems(menuItems, menuItemClickListener);

            menuItems = layoutMainPanelItems(menuItems, getAdjustedToolbarWidth(suggestedWidth));

            if (!menuItems.isEmpty()) {

                // Add remaining items to the overflow.
@@ -569,6 +559,28 @@ public final class FloatingToolbar { 
            updatePopupSize();

        }



        /**

         * Updates the popup's menu items without rebuilding the widget.

         * Use in place of layoutMenuItems() when the popup's views need not be reconstructed.

         *

         * @see isLayoutRequired(List<MenuItem>)

         */

        public void updateMenuItems(

                List<MenuItem> menuItems, MenuItem.OnMenuItemClickListener menuItemClickListener) {

            mMenuItems.clear();

            for (MenuItem menuItem : menuItems) {

                mMenuItems.put(MenuItemRepr.of(menuItem), menuItem);

            }

            mOnMenuItemClickListener = menuItemClickListener;

        }



        /**

         * Returns true if this popup needs a relayout to properly render the specified menu items.

         */

        public boolean isLayoutRequired(List<MenuItem> menuItems) {

            return !MenuItemRepr.reprEquals(menuItems, mMenuItems.values());

        }



        /**

         * Shows this popup at the specified coordinates.

         * The specified coordinates may be adjusted to make sure the popup is entirely on-screen.
@@ -1374,7 +1386,7 @@ public final class FloatingToolbar { 
        }



        private void setButtonTagAndClickListener(View menuItemButton, MenuItem menuItem) {

            menuItemButton.setTag(menuItem);

            menuItemButton.setTag(MenuItemRepr.of(menuItem));

            menuItemButton.setOnClickListener(mMenuItemButtonOnClickListener);

        }
@@ -1655,6 +1667,85 @@ public final class FloatingToolbar { 
        }

    }



    /**

     * Represents the identity of a MenuItem that is rendered in a FloatingToolbarPopup.

     */

    @VisibleForTesting

    public static final class MenuItemRepr {



        public final int itemId;

        public final int groupId;

        @Nullable public final String title;

        @Nullable private final Drawable mIcon;



        private MenuItemRepr(

                int itemId, int groupId, @Nullable CharSequence title, @Nullable Drawable icon) {

            this.itemId = itemId;

            this.groupId = groupId;

            this.title = (title == null) ? null : title.toString();

            mIcon = icon;

        }



        /**

         * Creates an instance of MenuItemRepr for the specified menu item.

         */

        public static MenuItemRepr of(MenuItem menuItem) {

            return new MenuItemRepr(

                    menuItem.getItemId(),

                    menuItem.getGroupId(),

                    menuItem.getTitle(),

                    menuItem.getIcon());

        }



        /**

         * Returns this object's hashcode.

         */

        @Override

        public int hashCode() {

            return Objects.hash(itemId, groupId, title, mIcon);

        }



        /**

         * Returns true if this object is the same as the specified object.

         */

        @Override

        public boolean equals(Object o) {

            if (o == this) {

                return true;

            }

            if (!(o instanceof MenuItemRepr)) {

                return false;

            }

            final MenuItemRepr other = (MenuItemRepr) o;

            return itemId == other.itemId

                    && groupId == other.groupId

                    && TextUtils.equals(title, other.title)

                    // Many Drawables (icons) do not implement equals(). Using equals() here instead

                    // of reference comparisons in case a Drawable subclass implements equals().

                    && Objects.equals(mIcon, other.mIcon);

        }



        /**

         * Returns true if the two menu item collections are the same based on MenuItemRepr.

         */

        public static boolean reprEquals(

                Collection<MenuItem> menuItems1, Collection<MenuItem> menuItems2) {

            if (menuItems1.size() != menuItems2.size()) {

                return false;

            }



            final Iterator<MenuItem> menuItems2Iter = menuItems2.iterator();

            for (MenuItem menuItem1 : menuItems1) {

                final MenuItem menuItem2 = menuItems2Iter.next();

                if (!MenuItemRepr.of(menuItem1).equals(MenuItemRepr.of(menuItem2))) {

                    return false;

                }

            }



            return true;

        }

    }



    /**

     * Creates and returns a menu button for the specified menu item.

     */

core/tests/coretests/src/android/widget/TextViewActivityTest.java

+50 −0
Original line number Diff line number Diff line
@@ -375,6 +375,56 @@ public class TextViewActivityTest { 
        assertFloatingToolbarIsDisplayed();

    }



    @Test

    public void testToolbarMenuItemClickAfterSelectionChange() throws Throwable {

        final MenuItem[] latestItem = new MenuItem[1];

        final MenuItem[] clickedItem = new MenuItem[1];

        final String text = "abcd efg hijk";

        mActivityRule.runOnUiThread(() -> {

            final TextView textView = mActivity.findViewById(R.id.textview);

            textView.setText(text);

            textView.setCustomSelectionActionModeCallback(

                    new ActionMode.Callback() {

                        @Override

                        public boolean onPrepareActionMode(ActionMode mode, Menu menu) {

                            menu.clear();

                            latestItem[0] = menu.add("Item");

                            return true;

                        }



                        @Override

                        public boolean onActionItemClicked(ActionMode mode, MenuItem item) {

                            clickedItem[0] = item;

                            return true;

                        }



                        @Override

                        public boolean onCreateActionMode(ActionMode mode, Menu menu) {

                            return true;

                        }



                        @Override

                        public void onDestroyActionMode(ActionMode mode) {}

                    });

        });

        mInstrumentation.waitForIdleSync();



        onView(withId(R.id.textview)).perform(longPressOnTextAtIndex(text.indexOf("f")));

        sleepForFloatingToolbarPopup();



        // Change the selection so that the menu items are refreshed.

        final TextView textView = mActivity.findViewById(R.id.textview);

        onHandleView(com.android.internal.R.id.selection_start_handle)

                .perform(dragHandle(textView, Handle.SELECTION_START, 0));

        sleepForFloatingToolbarPopup();

        assertFloatingToolbarIsDisplayed();



        clickFloatingToolbarItem("Item");

        mInstrumentation.waitForIdleSync();



        assertEquals(latestItem[0], clickedItem[0]);

    }



    @Test

    public void testSelectionRemovedWhenNonselectableTextLosesFocus() throws Throwable {

        final TextLinks.TextLink textLink = addLinkifiedTextToTextView(R.id.nonselectable_textview);

core/tests/coretests/src/android/widget/espresso/FloatingToolbarEspressoUtils.java

+8 −7
Original line number Diff line number Diff line
@@ -28,10 +28,11 @@ import static androidx.test.espresso.matcher.ViewMatchers.withId; 
import static androidx.test.espresso.matcher.ViewMatchers.withTagValue;

import static androidx.test.espresso.matcher.ViewMatchers.withText;



import static com.android.internal.widget.FloatingToolbar.MenuItemRepr;



import static org.hamcrest.Matchers.allOf;

import static org.hamcrest.Matchers.is;



import android.view.MenuItem;

import android.view.View;

import android.view.ViewGroup;
@@ -158,8 +159,8 @@ public class FloatingToolbarEspressoUtils { 
            public void describeTo(Description description) {}



            private void collectMenuItemIds(View view) {

                if (view.getTag() instanceof MenuItem) {

                    menuItemIds.add(((MenuItem) view.getTag()).getItemId());

                if (view.getTag() instanceof MenuItemRepr) {

                    menuItemIds.add(((MenuItemRepr) view.getTag()).itemId);

                } else if (view instanceof ViewGroup) {

                    ViewGroup viewGroup = (ViewGroup) view;

                    for (int i = 0; i < viewGroup.getChildCount(); i++) {
@@ -178,8 +179,8 @@ public class FloatingToolbarEspressoUtils { 
     */

    public static void assertFloatingToolbarDoesNotContainItem(String itemLabel) {

        final Predicate<View> hasMenuItemLabel = view ->

                view.getTag() instanceof MenuItem

                        && itemLabel.equals(((MenuItem) view.getTag()).getTitle().toString());

                view.getTag() instanceof MenuItemRepr

                        && itemLabel.equals(((MenuItemRepr) view.getTag()).title);

        assertFloatingToolbarMenuItem(hasMenuItemLabel, false);

    }
@@ -191,8 +192,8 @@ public class FloatingToolbarEspressoUtils { 
     */

    public static void assertFloatingToolbarDoesNotContainItem(final int menuItemId) {

        final Predicate<View> hasMenuItemId = view ->

                view.getTag() instanceof MenuItem

                        && ((MenuItem) view.getTag()).getItemId() == menuItemId;

                view.getTag() instanceof MenuItemRepr

                        && ((MenuItemRepr) view.getTag()).itemId == menuItemId;

        assertFloatingToolbarMenuItem(hasMenuItemId, false);

    }