Skip to content

Commit

Permalink
[Journeys] Remove visits in place when deleting
Browse files Browse the repository at this point in the history 
Previously, we just started from scratch with the current query. This
should be more performant and avoids losing scroll state.

(cherry picked from commit 62706b7)

Bug: 1303171, 1346692
Change-Id: Id736a304f440613bf61435b400526a98c01baa99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3781695
Reviewed-by: Gang Wu <gangwu@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1027863}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3788399
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5195@{chromium#43}
Cr-Branched-From: 7aa3f07-refs/heads/main@{#1027018}
  • Loading branch information
Patrick Noland authored and Chromium LUCI CQ committed Jul 26, 2022
1 parent 2e13494 commit 5663e6b
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 22 deletions.
32 changes: 28 additions & 4 deletions ...d/junit/src/org/chromium/chrome/browser/history_clusters/HistoryClustersMediatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,15 +545,39 @@ public void testDeleteItems() {
mMediator.setQueryState(QueryState.forQuery("query", ""));
mMediator.startQuery("query");
fulfillPromise(promise, mHistoryClustersResultWithQuery);
int initialSize = mModelList.size();

mMediator.deleteVisits(Arrays.asList(mVisit1, mVisit2, mVisit3));
assertThat(mVisitsForRemoval, Matchers.containsInAnyOrder(mVisit1, mVisit2, mVisit3));
mMediator.deleteVisits(Arrays.asList(mVisit1, mVisit3));
assertThat(mVisitsForRemoval, Matchers.containsInAnyOrder(mVisit1, mVisit3));
verify(mMetricsLogger)
.recordVisitAction(HistoryClustersMetricsLogger.VisitAction.DELETED, mVisit1);
verify(mMetricsLogger)
.recordVisitAction(HistoryClustersMetricsLogger.VisitAction.DELETED, mVisit2);
verify(mMetricsLogger)
.recordVisitAction(HistoryClustersMetricsLogger.VisitAction.DELETED, mVisit3);
// Deleting all of the visits in a cluster should also delete the ModelList entry for the
// cluster itself.
assertEquals(initialSize - 3, mModelList.size());

ListItem clusterItem = mModelList.get(0);
assertEquals(clusterItem.type, ItemType.CLUSTER);

ListItem visitItem = mModelList.get(1);
assertEquals(visitItem.type, ItemType.VISIT);
PropertyModel visitModel = visitItem.model;
assertEquals(mMediator.applyBolding(mVisit2.getTitle(), mVisit2.getTitleMatchPositions()),
visitModel.get(HistoryClustersItemProperties.TITLE));
assertEquals(
mMediator.applyBolding(mVisit2.getUrlForDisplay(), mVisit2.getUrlMatchPositions()),
visitModel.get(HistoryClustersItemProperties.URL));

ListItem relatedSearchesItem = mModelList.get(2);
assertEquals(relatedSearchesItem.type, ItemType.RELATED_SEARCHES);
PropertyModel relatedSearchesModel = relatedSearchesItem.model;
assertEquals(mCluster1.getRelatedSearches(),
relatedSearchesModel.get(HistoryClustersItemProperties.RELATED_SEARCHES));

mMediator.deleteVisits(Arrays.asList(mVisit2));
// Deleting the final visit should result in an entirely empty list.
assertEquals(0, mModelList.size());
}

@Test
Expand Down
73 changes: 55 additions & 18 deletions ...usters/java/src/org/chromium/chrome/browser/history_clusters/HistoryClustersMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,12 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

class HistoryClustersMediator extends RecyclerView.OnScrollListener implements SearchDelegate {
@VisibleForTesting
Expand All @@ -62,6 +64,21 @@ interface Clock {
long currentTimeMillis();
}

private static class VisitMetadata {
public final ListItem visitListItem;
public final ListItem clusterListItem;
public final ListItem relatedSearchesListItem;
public AtomicInteger visitCount;

private VisitMetadata(ListItem visitListItem, ListItem clusterListItem,
ListItem relatedSearchesListItem, AtomicInteger visitCount) {
this.visitListItem = visitListItem;
this.clusterListItem = clusterListItem;
this.relatedSearchesListItem = relatedSearchesListItem;
this.visitCount = visitCount;
}
}

private final HistoryClustersBridge mHistoryClustersBridge;
private final Context mContext;
private final Resources mResources;
Expand All @@ -82,6 +99,7 @@ interface Clock {
private QueryState mQueryState = QueryState.forQueryless();
private final HistoryClustersMetricsLogger mMetricsLogger;
private Map<String, PropertyModel> mLabelToModelMap = new LinkedHashMap<>();
private Map<ClusterVisit, VisitMetadata> mVisitMetadataMap = new HashMap<>();

/**
* Create a new HistoryClustersMediator.
Expand Down Expand Up @@ -267,11 +285,22 @@ void deleteVisits(List<ClusterVisit> visits) {
mDelegate.markVisitForRemoval(visit);
mMetricsLogger.recordVisitAction(
HistoryClustersMetricsLogger.VisitAction.DELETED, visit);
removeVisit(visit);
}

mDelegate.removeMarkedItems();
}

resetModel();
startQuery(mQueryState.getQuery());
private void removeVisit(ClusterVisit visit) {
VisitMetadata visitMetadata = mVisitMetadataMap.get(visit);
if (visitMetadata == null) return;
mModelList.remove(visitMetadata.visitListItem);
if (visitMetadata.visitCount.decrementAndGet() == 0) {
mModelList.remove(visitMetadata.clusterListItem);
if (visitMetadata.relatedSearchesListItem != null) {
mModelList.remove(visitMetadata.relatedSearchesListItem);
}
}
}

private Tab createNewTab(GURL gurl, boolean incognito, Tab parentTab) {
Expand Down Expand Up @@ -333,7 +362,24 @@ private void queryComplete(HistoryClustersResult result) {

List<ListItem> visitsAndRelatedSearches =
new ArrayList<>(cluster.getVisits().size() + 1);
for (ClusterVisit visit : cluster.getVisits()) {
ListItem relatedSearchesItem = null;
List<String> relatedSearches = cluster.getRelatedSearches();
if (!relatedSearches.isEmpty()) {
PropertyModel relatedSearchesModel =
new PropertyModel.Builder(HistoryClustersItemProperties.ALL_KEYS)
.with(HistoryClustersItemProperties.RELATED_SEARCHES,
relatedSearches)
.with(HistoryClustersItemProperties.CHIP_CLICK_HANDLER,
(query)
-> onRelatedSearchesChipClicked(
query, relatedSearches.indexOf(query)))
.build();
relatedSearchesItem = new ListItem(ItemType.RELATED_SEARCHES, relatedSearchesModel);
}

AtomicInteger visitCount = new AtomicInteger(cluster.getVisits().size());
for (int i = 0; i < visitCount.get(); i++) {
ClusterVisit visit = cluster.getVisits().get(i);
PropertyModel visitModel =
new PropertyModel.Builder(HistoryClustersItemProperties.ALL_KEYS)
.with(HistoryClustersItemProperties.TITLE,
Expand Down Expand Up @@ -361,27 +407,17 @@ private void queryComplete(HistoryClustersResult result) {
});
}

visitsAndRelatedSearches.add(new ListItem(ItemType.VISIT, visitModel));
ListItem listItem = new ListItem(ItemType.VISIT, visitModel);
mVisitMetadataMap.put(visit,
new VisitMetadata(listItem, clusterItem, relatedSearchesItem, visitCount));
visitsAndRelatedSearches.add(listItem);
}

List<String> relatedSearches = cluster.getRelatedSearches();
if (!relatedSearches.isEmpty()) {
PropertyModel relatedSearchesModel =
new PropertyModel.Builder(HistoryClustersItemProperties.ALL_KEYS)
.with(HistoryClustersItemProperties.RELATED_SEARCHES,
relatedSearches)
.with(HistoryClustersItemProperties.CHIP_CLICK_HANDLER,
(query)
-> onRelatedSearchesChipClicked(
query, relatedSearches.indexOf(query)))
.build();
ListItem relatedSearchesItem =
new ListItem(ItemType.RELATED_SEARCHES, relatedSearchesModel);
if (relatedSearchesItem != null) {
visitsAndRelatedSearches.add(relatedSearchesItem);
}

mModelList.addAll(visitsAndRelatedSearches);

clusterModel.set(HistoryClustersItemProperties.CLICK_HANDLER,
v -> hideCluster(clusterModel, visitsAndRelatedSearches));
Drawable chevron = UiUtils.getTintedDrawable(mContext,
Expand All @@ -395,6 +431,7 @@ private void queryComplete(HistoryClustersResult result) {
private void resetModel() {
mModelList.clear();
mLabelToModelMap.clear();
mVisitMetadataMap.clear();
}

private String getQuotedLabelFromRawLabel(String rawLabel, List<HistoryCluster> clusters) {
Expand Down

0 comments on commit 5663e6b

Please sign in to comment.