Suggestions UI - expandable item

ios/chrome/browser/ui/suggestions/suggestions_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.
 
 #import "ios/chrome/browser/ui/suggestions/suggestions_view_controller.h"
 
+#include "base/mac/foundation_util.h"
 #import "ios/chrome/browser/ui/collection_view/cells/MDCCollectionViewCell+Chrome.h"
 #import "ios/chrome/browser/ui/collection_view/cells/collection_view_item.h"
 #import "ios/chrome/browser/ui/collection_view/collection_view_model.h"
 #import "ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h"
 #import "ios/chrome/browser/ui/suggestions/suggestions_commands.h"
 #import "ios/chrome/browser/ui/suggestions/suggestions_item_actions.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
 @interface SuggestionsViewController ()<SuggestionsItemActions>
 
 @property(nonatomic, strong) SuggestionsCollectionUpdater* collectionUpdater;
 
+// Applies |interaction| to the item corresponding to |cell| if it is a

[marq (ping after 24h), 2017/01/16 17:16:42]

|interaction| isn't defined.

[gambard, 2017/01/17 09:59:00]

Done.

+// SuggestionsExpandableCell.
+- (void)expand:(BOOL)expand cell:(UICollectionViewCell*)cell;
+
 @end
 
 @implementation SuggestionsViewController
 
 @synthesize suggestionCommandHandler = _suggestionCommandHandler;
 @synthesize collectionUpdater = _collectionUpdater;
 
 #pragma mark - UIViewController
 
 - (void)viewDidLoad {
@@ skipping 15 matching lines @@
   UIEdgeInsets inset = [self collectionView:collectionView
                                      layout:collectionView.collectionViewLayout
                      insetForSectionAtIndex:indexPath.section];
 
   return [MDCCollectionViewCell
       cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
                                  inset.left - inset.right
                          forItem:item];
 }
 
+#pragma mark - SuggestionsExpandableCellDelegate
+
+- (void)retractCell:(UICollectionViewCell*)cell {
+  [self expand:NO cell:cell];
+}
+
+- (void)expandCell:(UICollectionViewCell*)cell {
+  [self expand:YES cell:cell];
+}
+
 #pragma mark - SuggestionsItemActions
 
 - (void)addNewItem:(id)sender {
   [self.suggestionCommandHandler addEmptyItem];
 }
 
 #pragma mark - SuggestionsCollectionUpdater forwarding
 
 - (void)addTextItem:(NSString*)title
            subtitle:(NSString*)subtitle
           toSection:(NSInteger)inputSection {
   [self.collectionUpdater addTextItem:title
                              subtitle:subtitle
                             toSection:inputSection];
 }
 
+#pragma mark - Private
+
+- (void)expand:(BOOL)expand cell:(UICollectionViewCell*)cell {
+  NSIndexPath* indexPath = [self.collectionView indexPathForCell:cell];
+  CollectionViewItem* item =
+      [self.collectionViewModel itemAtIndexPath:indexPath];
+  if ([item isKindOfClass:[SuggestionsExpandableItem class]]) {

[marq (ping after 24h), 2017/01/16 17:16:42]

As soon as you start checking for class membership at runtime, you should make
whatever part of the API you need into a protocol, and instead check for that.
You will still need to cast into id<ExpandableItem>, but protocols are less
brittle than explicit casts. (note that ObjCCast doesn't work with protocols).

[gambard, 2017/01/17 09:59:00]

Done.

[lpromero, 2017/01/17 10:38:49]

This should just check the item type.

[gambard, 2017/01/17 11:55:29]

As I am checking if it conforms to protocol, the itemType check is not needed
for now. If I have different classes conforming to the protocol, I would use the
itemType to discriminate.

[marq (ping after 24h), 2017/01/17 13:42:16]

Part of the benefit of the protocol is that you don't care what the class of the
conforming object is.

[gambard, 2017/01/17 14:46:56]

Yes, I was meaning different objects conforming to the same protocol but
potentially having different behavior.

+    SuggestionsExpandableItem* expandableItem =
+        base::mac::ObjCCast<SuggestionsExpandableItem>(item);
+
+    NSInteger sectionIdentifier = [self.collectionViewModel
+        sectionIdentifierForSection:indexPath.section];
+
+    expandableItem.expanded = expand;

[marq (ping after 24h), 2017/01/16 17:16:42]

lpromero@ -- The cycle of:

- Collection item changes
- CollectionViewController reconfigures cells for that item (looking up its
section in order to do so)
- CollectionViewLayout is invalidated

Seems clunky, but also one that's likely to be repeated. It seems like once the
item is part of the collection view's model, changing it should be able to
trigger a relayout. Ideally this code should just be:

[UIView animateWithDuration:duration animations:^{expandableItem.expanded =
expand;}];

And all of the lookup, cell reconfiguration, and layout invalidation follows
implicitly from there.

[lpromero, 2017/01/17 10:38:49]

It is a design decision to have the controller explicitly do this work. We don't
want the model, even less so the item, depend on the collection view, and we
don't want to construct a paradigm we will one day need to workaround (like QTM
collections).

The layout invalidation for example is required here because the cells are
resized, but is usually not necessary.

[marq (ping after 24h), 2017/01/17 13:42:16]

I was thinking more of some way for the view item to signal that its cells were
now dirty without necessarily creating a strong dependency from the model to the
controller itself, but that's all out of the scope of this CL.

+    [self reconfigureCellsForItems:@[ expandableItem ]
+           inSectionWithIdentifier:sectionIdentifier];
+
+    [UIView
+        animateWithDuration:1

[marq (ping after 24h), 2017/01/16 17:16:42]

use a constant for animation duration.

one second is a fairly long animation; 0.35 is typical for most UI transitions.

[gambard, 2017/01/17 09:59:00]

Done.

+                 animations:^{
+                   [self.collectionView.collectionViewLayout invalidateLayout];
+                 }];
+  }
+}
+
 @end