Suggestions UI - expandable item

ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm

Index: ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm
diff --git a/ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm b/ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm
index a27da97a063afef0786224ca5511cfd72ae5fbe0..0940e8db9b2fe0f8a11dfa179326090f669d3c53 100644
--- a/ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm
+++ b/ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm
@@ -4,6 +4,7 @@
 
 #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"
@@ -19,6 +20,10 @@
 
 @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
@@ -54,6 +59,16 @@
                          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 {
@@ -70,4 +85,29 @@
                             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