[ios] Inserts and deletes history items in the same BatchUpdates block

ios/chrome/browser/ui/history/history_collection_view_controller.mm

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ios/chrome/browser/ui/history/history_collection_view_controller.h"
 
 #import <MobileCoreServices/MobileCoreServices.h>
 
 #include <memory>
 
@@ skipping 96 matching lines @@
 // Fetches history prior to |time| for search text |query|. If |query| is nil or
 // the empty string, all history is fetched.
 - (void)fetchHistoryForQuery:(NSString*)query
                  priorToTime:(const base::Time&)time;
 // Updates header section to provide relevant information about the currently
 // displayed history entries.
 - (void)updateEntriesStatusMessage;
 // Removes selected items from the visible collection, but does not delete them
 // from browser history.
 - (void)removeSelectedItemsFromCollection;
-// Removes all items in the collection that are not included in entries.
+// Selects all items in the collection that are not included in entries.
 - (void)filterForHistoryEntries:(NSArray*)entries;
+// Deletes all items in the collection which indexes are included in indexArray,
+// needs to be run inside a performBatchUpdates block.
+- (void)deleteItemsFromCollectionViewModelWithIndex:(NSArray*)indexArray;
 // Adds loading indicator to the top of the history collection, if one is not
 // already present.
 - (void)addLoadingIndicator;
 // Displays context menu on cell pressed with gestureRecognizer.
 - (void)displayContextMenuInvokedByGestureRecognizer:
     (UILongPressGestureRecognizer*)gestureRecognizer;
 // Opens URL in the current tab and dismisses the history view.
 - (void)openURL:(const GURL&)URL;
 // Opens URL in a new non-incognito tab and dismisses the history view.
 - (void)openURLInNewTab:(const GURL&)URL;
@@ skipping 233 matching lines @@
       HistoryEntryItem* item =
           [[HistoryEntryItem alloc] initWithType:ItemTypeHistoryEntry
                                     historyEntry:entry
                                     browserState:_browserState
                                         delegate:self];
       [self.entryInserter insertHistoryEntryItem:item];
       if ([self isSearching] || self.filterQueryResult) {
         [filterResults addObject:item];
       }
     }
     [self.delegate historyCollectionViewControllerDidChangeEntries:self];

[sczs, 2017/04/05 01:57:50]

We can clearly see now that this was being called twice, once here and once in
the completion block. I think we could remove this call, on the other hand,
since this might be Cherry picked, I want to change as less as possible. 

+    if (([self isSearching] && [searchQuery length] > 0 &&
+         [self.currentQuery isEqualToString:searchQuery]) ||
+        self.filterQueryResult) {
+      // If in search mode, filter out entries that are not
+      // part of the search result.
+      [self filterForHistoryEntries:filterResults];
+      NSArray* deletedIndexPaths =
+          self.collectionView.indexPathsForSelectedItems;
+      [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths];

[ramyasharma, 2017/04/06 06:23:27]

Why not call this inline? Especially since this method has a constraint that it
must be called from within PerformBatchUpdates, isn't it better to just call it
inline? Rather than expect the caller in future to ensure its called in
PerformBatchUpdates?
This is assuming that removeSelectedItemsFromCollection can be removed. 

[sczs, 2017/04/07 02:03:25]

Since removeSelectedItemsFromCollection can't be removed I think this is the
best we got for now, but I understand the concern. 

+      self.filterQueryResult = NO;
+    }
   }
       completion:^(BOOL) {

[lpromero, 2017/04/05 09:48:05]

This is the same completion block as in the removeSelectedItems method, please
create a method.

Before we had:

performBatchUpdate
  ... something ...
completion
  filter
    remove
      performBatchUpdates
        deleteItems
      completion
        "updateForNoEntries"

Now we have:

performBatchUpdate
  ... something ...
  filter
  deleteItems
completion
  "updateForNoEntries"

Did I get this right?

[sczs, 2017/04/07 02:03:25]

Exactly right! The exact same code is being executed, only that this time
everything happens inside one performBatchUpdates block.

Will create a method a method for the completion block.

-        if (([self isSearching] && [searchQuery length] > 0 &&
-             [self.currentQuery isEqualToString:searchQuery]) ||
-            self.filterQueryResult) {
-          // If in search mode, filter out entries that are not
-          // part of the search result.
-          [self filterForHistoryEntries:filterResults];
-          self.filterQueryResult = NO;
+        // If only the header section remains, there are no history entries.
+        if ([self.collectionViewModel numberOfSections] == 1) {
+          self.entriesType = NO_ENTRIES;
         }
+        [self updateEntriesStatusMessage];
+        [self.delegate historyCollectionViewControllerDidChangeEntries:self];
       }];
 }
 
 - (void)historyServiceFacade:(HistoryServiceFacade*)facade
     shouldShowNoticeAboutOtherFormsOfBrowsingHistory:(BOOL)shouldShowNotice {
   self.shouldShowNoticeAboutOtherFormsOfBrowsingHistory = shouldShowNotice;
   // Update the history entries status message if there is no query in progress.
   if (!self.isLoading) {
     [self updateEntriesStatusMessage];
   }
@@ skipping 215 matching lines @@
           fromSectionWithIdentifier:kEntriesStatusSectionIdentifier];
       [self.collectionView deleteItemsAtIndexPaths:@[ indexPath ]];
     }
     [self.collectionViewModel addItem:entriesStatusItem
               toSectionWithIdentifier:kEntriesStatusSectionIdentifier];
     [self.collectionView insertItemsAtIndexPaths:@[ indexPath ]];
   }
                                 completion:nil];
 }
 
 - (void)removeSelectedItemsFromCollection {

[ramyasharma, 2017/04/06 06:23:27]

Should this be removed? Is this used elsewhere?

[sczs, 2017/04/07 02:03:25]

This is also used when deleting items from History. In this case we only delete
them from the CollectionView when filtering.

   NSArray* deletedIndexPaths = self.collectionView.indexPathsForSelectedItems;
   [self.collectionView performBatchUpdates:^{
-    [self collectionView:self.collectionView
-        willDeleteItemsAtIndexPaths:deletedIndexPaths];
-    [self.collectionView deleteItemsAtIndexPaths:deletedIndexPaths];
-
-    // Remove any empty sections, except the header section.
-    for (int section = self.collectionView.numberOfSections - 1; section > 0;
-         --section) {
-      if (![self.collectionViewModel numberOfItemsInSection:section]) {
-        [self.entryInserter removeSection:section];
-      }
-    }
+    [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths];
   }
       completion:^(BOOL) {
         // If only the header section remains, there are no history entries.
         if ([self.collectionViewModel numberOfSections] == 1) {
           self.entriesType = NO_ENTRIES;
         }
         [self updateEntriesStatusMessage];
         [self.delegate historyCollectionViewControllerDidChangeEntries:self];
       }];
 }
 
+- (void)deleteItemsFromCollectionViewModelWithIndex:(NSArray*)indexArray {
+  [self collectionView:self.collectionView
+      willDeleteItemsAtIndexPaths:indexArray];
+  [self.collectionView deleteItemsAtIndexPaths:indexArray];
+
+  // Remove any empty sections, except the header section.
+  for (int section = self.collectionView.numberOfSections - 1; section > 0;
+       --section) {
+    if (![self.collectionViewModel numberOfItemsInSection:section]) {
+      [self.entryInserter removeSection:section];
+    }
+  }
+}
+
 - (void)filterForHistoryEntries:(NSArray*)entries {
   self.collectionView.allowsMultipleSelection = YES;
   for (int section = 1; section < [self.collectionViewModel numberOfSections];
        ++section) {
     NSInteger sectionIdentifier =
         [self.collectionViewModel sectionIdentifierForSection:section];
     if ([self.collectionViewModel
             hasSectionForSectionIdentifier:sectionIdentifier]) {
       NSArray* items = [self.collectionViewModel
           itemsInSectionWithIdentifier:sectionIdentifier];
       for (id item in items) {
         HistoryEntryItem* historyItem =
             base::mac::ObjCCastStrict<HistoryEntryItem>(item);
         if (![entries containsObject:historyItem]) {
           NSIndexPath* indexPath =
               [self.collectionViewModel indexPathForItem:historyItem
                                  inSectionWithIdentifier:sectionIdentifier];
           [self.collectionView
               selectItemAtIndexPath:indexPath
                            animated:NO
                      scrollPosition:UICollectionViewScrollPositionNone];
         }
       }
     }
   }
-  [self removeSelectedItemsFromCollection];
 }
 
 - (void)addLoadingIndicator {
   NSIndexPath* indexPath = [NSIndexPath indexPathForItem:0 inSection:0];
   if ([self.collectionViewModel hasItemAtIndexPath:indexPath] &&
       [self.collectionViewModel itemTypeForIndexPath:indexPath] ==
           ItemTypeActivityIndicator) {
     // Do not add indicator a second time.
     return;
   }
@@ skipping 118 matching lines @@
                        shouldCloseWithCompletion:^{
                          [self.URLLoader webPageOrderedOpen:copiedURL
                                                    referrer:web::Referrer()
                                                 inIncognito:YES
                                                inBackground:NO
                                                    appendTo:kLastTab];
                        }];
 }
 
 @end