Commit 1f53ac3c authored by sandromaggi's avatar sandromaggi Committed by Commit Bot

[Autofill Assistant] Change update method for chips.

Before, chips were replaced as a list. Now they are compared one by one,
if they are "the same", only the chip's |enabled| state is updated.

Bug: b/144075373
Change-Id: I7e9e970217faa85e66c559194fe0159f98d67a4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2027908Reviewed-by: default avatarClemens Arbesser <arbesser@google.com>
Reviewed-by: default avatarMathias Carlen <mcarlen@chromium.org>
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#737310}
parent 422ea0b7
......@@ -220,6 +220,10 @@ class AssistantBottomBarCoordinator
});
}
AssistantCarouselCoordinator getActionsCarouselCoordinator() {
return mActionsCoordinator;
}
private void setupAnimations(AssistantModel model, ViewGroup rootView) {
// Animate when the chip in the header changes.
model.getHeaderModel().addObserver((source, propertyKey) -> {
......
......@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.autofill_assistant;
import android.content.Context;
import android.support.annotation.Nullable;
import android.view.View;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.JNINamespace;
......@@ -14,6 +15,7 @@ import org.chromium.base.task.PostTask;
import org.chromium.chrome.autofill_assistant.R;
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.autofill_assistant.carousel.AssistantCarouselCoordinator;
import org.chromium.chrome.browser.autofill_assistant.carousel.AssistantCarouselModel;
import org.chromium.chrome.browser.autofill_assistant.carousel.AssistantChip;
import org.chromium.chrome.browser.autofill_assistant.carousel.AssistantChip.Type;
......@@ -25,6 +27,7 @@ import org.chromium.chrome.browser.ui.messages.snackbar.SnackbarManager.Snackbar
import org.chromium.chrome.browser.widget.bottomsheet.BottomSheetController;
import org.chromium.content_public.browser.UiThreadTaskTraits;
import org.chromium.content_public.browser.WebContents;
import org.chromium.ui.modelutil.ListModel;
import java.util.ArrayList;
import java.util.HashSet;
......@@ -300,13 +303,16 @@ class AutofillAssistantUiController {
@CalledByNative
private void setActions(List<AssistantChip> chips) {
AssistantCarouselModel model = getModel().getActionsModel();
setChips(model, chips);
// TODO(b/144075373): Move this to AssistantActionsCarouselCoordinator.
setChips(model, chips,
mCoordinator.getBottomBarCoordinator().getActionsCarouselCoordinator());
setHeaderChip(chips);
}
@CalledByNative
private void setSuggestions(List<AssistantChip> chips) {
setChips(getModel().getSuggestionsModel(), chips);
// TODO(b/144075373): Move this to AssistantSuggestionsCarouselCoordinator.
setChips(getModel().getSuggestionsModel(), chips, /* carouselCoordinator= */ null);
}
private void setHeaderChip(List<AssistantChip> chips) {
......@@ -322,13 +328,55 @@ class AutofillAssistantUiController {
getModel().getHeaderModel().set(AssistantHeaderModel.CHIP, headerChip);
}
private void setChips(AssistantCarouselModel model, List<AssistantChip> chips) {
// We apply the minimum set of operations on the current chips to transform it in the target
// list of chips. When testing for chip equivalence, we only compare their type and text but
// all substitutions will still be applied so we are sure we display the given {@code chips}
// with their associated callbacks.
EditDistance.transform(model.getChipsModel(), chips,
(a, b) -> a.getType() == b.getType() && a.getText().equals(b.getText()));
/**
* Returns |true| if all chips in the list are considered equal, meaning the same except their
* |isDisabled()| state.
*/
private boolean areChipsEqual(ListModel<AssistantChip> oldChips, List<AssistantChip> newChips) {
if (oldChips.size() != newChips.size()) {
return false;
}
for (int i = 0; i < oldChips.size(); ++i) {
AssistantChip oldChip = oldChips.get(i);
AssistantChip newChip = newChips.get(i);
if (!oldChip.equals(newChip)) {
return false;
}
}
return true;
}
private void setChips(AssistantCarouselModel model, List<AssistantChip> newChips,
@Nullable AssistantCarouselCoordinator carouselCoordinator) {
ListModel<AssistantChip> oldChips = model.getChipsModel();
if (carouselCoordinator == null || !areChipsEqual(oldChips, newChips)) {
// We apply the minimum set of operations on the current chips to transform it in the
// target list of chips. When testing for chip equivalence, we only compare their type
// and text but all substitutions will still be applied so we are sure we display the
// given {@code chips} with their associated callbacks.
EditDistance.transform(oldChips, newChips, AssistantChip::equals);
} else {
for (int i = 0; i < oldChips.size(); ++i) {
AssistantChip oldChip = oldChips.get(i);
AssistantChip newChip = newChips.get(i);
// We assume that the enabled state is the only thing that may change.
if (oldChip.isDisabled() == newChip.isDisabled()) {
continue;
}
View view = carouselCoordinator.getView().getLayoutManager().findViewByPosition(i);
if (view == null) {
oldChips.update(i, newChip);
} else {
oldChip.setDisabled(newChip.isDisabled());
view.setEnabled(!newChip.isDisabled());
}
}
}
}
@CalledByNative
......
......@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.autofill_assistant.carousel;
import static org.chromium.chrome.browser.autofill_assistant.AssistantTagsForTesting.RECYCLER_VIEW_TAG;
import android.content.Context;
import android.support.v7.widget.DefaultItemAnimator;
import android.support.v7.widget.OrientationHelper;
import android.support.v7.widget.RecyclerView;
import android.view.View;
......@@ -68,10 +67,6 @@ public class AssistantActionsCarouselCoordinator implements AssistantCarouselCoo
new SimpleRecyclerViewMcp<>(model.getChipsModel(),
AssistantChipViewHolder::getViewType, AssistantChipViewHolder::bind),
AssistantChipViewHolder::create));
// Disabling change animations to avoid chips that blink when setting the same, unchanged,
// set of chips.
((DefaultItemAnimator) mView.getItemAnimator()).setSupportsChangeAnimations(false);
}
@Override
......
......@@ -56,7 +56,7 @@ public class AssistantChip {
private final String mText;
/** Whether this chip is enabled or not. */
private final boolean mDisabled;
private boolean mDisabled;
/**
* Whether this chip is sticky. A sticky chip will be a candidate to be displayed in the header
......@@ -93,6 +93,15 @@ public class AssistantChip {
return mDisabled;
}
/**
* Set the disabled state of the {@link AssistantChip} object. Changing this flag will not
* affect the view this chip is bound to. Use this to keep the model and view in sync, if
* the view's state changes.
*/
public void setDisabled(boolean disabled) {
mDisabled = disabled;
}
public boolean isSticky() {
return mSticky;
}
......@@ -100,4 +109,18 @@ public class AssistantChip {
public Runnable getSelectedListener() {
return mSelectedListener;
}
@Override
public boolean equals(Object other) {
if (other == this) {
return true;
}
if (!(other instanceof AssistantChip)) {
return false;
}
AssistantChip that = (AssistantChip) other;
return this.getType() == that.getType() && this.getText().equals(that.getText())
&& this.getIcon() == that.getIcon() && this.isSticky() == that.isSticky();
}
}
......@@ -18,7 +18,6 @@ import static org.hamcrest.Matchers.allOf;
import android.support.test.InstrumentationRegistry;
import android.support.test.filters.MediumTest;
import android.support.v7.widget.DefaultItemAnimator;
import android.widget.LinearLayout;
import org.junit.Before;
......@@ -76,11 +75,6 @@ public class AutofillAssistantActionsCarouselUiTest {
AssistantCarouselModel model = new AssistantCarouselModel();
AssistantActionsCarouselCoordinator coordinator = createCoordinator(model);
TestThreadUtils.runOnUiThreadBlocking(() -> {
assertThat(((DefaultItemAnimator) coordinator.getView().getItemAnimator())
.getSupportsChangeAnimations(),
is(false));
});
assertThat(model.getChipsModel().size(), is(0));
assertThat(coordinator.getView().getAdapter().getItemCount(), is(0));
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment