[Payment Request] Selector view controller

ios/chrome/browser/payments/payment_request_selector_view_controller.mm

+// Copyright 2017 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/payments/payment_request_selector_view_controller.h"
+
+#include "base/mac/foundation_util.h"
+#include "components/strings/grit/components_strings.h"
+#import "ios/chrome/browser/payments/cells/payments_text_item.h"
+#import "ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h"
+#import "ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h"
+#import "ios/chrome/browser/ui/autofill/cells/status_item.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/cells/collection_view_text_item.h"
+#import "ios/chrome/browser/ui/collection_view/collection_view_model.h"
+#import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h"
+#import "ios/chrome/browser/ui/icons/chrome_icon.h"
+#include "ios/chrome/browser/ui/uikit_ui_util.h"
+#include "ios/chrome/grit/ios_strings.h"
+#include "ios/chrome/grit/ios_theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace {
+
+NSString* const kPaymentRequestSelectorCollectionViewAccessibilityID =
+    @"kPaymentRequestSelectorCollectionViewAccessibilityID";
+
+const CGFloat kSeparatorEdgeInset = 14;
+
+typedef NS_ENUM(NSInteger, SectionIdentifier) {
+  SectionIdentifierItems = kSectionIdentifierEnumZero,
+};
+
+typedef NS_ENUM(NSInteger, ItemType) {
+  ItemTypeHeader = kItemTypeHeaderItem,  // This is a repeated item type.

[gambard, 2017/04/10 09:47:35]

Is this item type the same as the one in the data source? If it is the case I am
not sure it is a good idea, the order would be very important. (If a new item
type is added between header and selectable item, I guess everything would be
broken.)
+lpromero.

[Moe, 2017/04/10 17:53:49]

Correct. Nothing should be added between the header and the selectable item. But
since we're writing this once and reusing it, nothing should ideally be added
here anyway.

+  ItemTypeSelectableItem,
+  ItemTypeSpinner,
+  ItemTypeAddItem,
+};
+
+}  // namespace
+
+@interface PaymentRequestSelectorViewController ()<
+    PaymentRequestSelectorViewControllerActions>
+
+@end
+
+@implementation PaymentRequestSelectorViewController
+
+@synthesize delegate = _delegate;
+@synthesize dataSource = _dataSource;
+
+- (instancetype)init {
+  if ((self = [super initWithStyle:CollectionViewControllerStyleAppBar])) {
+    // Set up leading (return) button.
+    UIBarButtonItem* returnButton =
+        [ChromeIcon templateBarButtonItemWithImage:[ChromeIcon backIcon]
+                                            target:nil
+                                            action:@selector(onReturn)];
+    returnButton.accessibilityLabel = l10n_util::GetNSString(IDS_ACCNAME_BACK);
+    self.navigationItem.leftBarButtonItem = returnButton;

[gambard, 2017/04/10 09:47:34]

I think the coordinator should set the navigation item. I guess it would be OK
if this method is used in tests only, but in this case make it more visible (put
it in a testing category, name it initForTesting).

[Moe, 2017/04/10 17:53:49]

All the view controllers in ios/chrome/browser/payments setup their own
navigation item. I'd say let's keep this consistent with the rest of the view
controllers and if we really need to this, we'll do it a sweeping stroke for all
view controllers.

[gambard, 2017/04/18 08:06:37]

Acknowledged.

+  }
+  return self;
+}
+

[gambard, 2017/04/10 09:47:34]

Add #pragma mark - PaymentRequestSelectorViewControllerActions

[Moe, 2017/04/10 17:53:49]

Done.

+- (void)onReturn {
+  [_delegate paymentRequestSelectorViewControllerDidReturn:self];

[gambard, 2017/04/10 09:47:35]

s/_delegate/self.delegate in the whole file (except if the method can be called
from init).

[Moe, 2017/04/10 17:53:49]

Done.

+}
+
+#pragma mark - CollectionViewController methods
+
+- (void)loadModel {
+  [super loadModel];
+  CollectionViewModel* model = self.collectionViewModel;
+
+  [model addSectionWithIdentifier:SectionIdentifierItems];
+
+  // If the view controller is in the pending state, only display a spinner and
+  // a message indicating the pending state.
+  if (_dataSource.state == PaymentRequestSelectorViewControllerStatePending) {

[gambard, 2017/04/10 09:47:35]

s/_dataSource/self.dataSource in the whole file (except if the method can be
called from init).

[Moe, 2017/04/10 17:53:49]

Done.

+    StatusItem* statusItem = [[StatusItem alloc] initWithType:ItemTypeSpinner];
+    statusItem.text = l10n_util::GetNSString(IDS_PAYMENTS_CHECKING_OPTION);

[gambard, 2017/04/10 09:47:35]

Don't you need to set the status?

[Moe, 2017/04/10 17:53:49]

Done.

+    [model addItem:statusItem toSectionWithIdentifier:SectionIdentifierItems];
+    return;
+  }
+
+  CollectionViewItem* headerItem = [_dataSource headerItem];

[gambard, 2017/04/10 09:47:35]

I think it is better to pass the value to be displayed in the items than to pass
directly the items.
I understand this design is taking some logic out of the UI, allowing it to only
display a bunch of item of any type.
I personally have a UI mediator, translating the data send by the model to UI
item, but it creates a lot of boilerplate.
+lpromero for opinion.

[Moe, 2017/04/10 17:53:49]

I discussed this with lpromero@ previously and he supported the idea of passing
the items directly. I personally find another abstraction level a bit of an
overkill. But could you please provide an example of your UI mediator for
context?

[gambard, 2017/04/18 08:06:37]

I am writing the ContentSuggestions classes. My mediator lives in
ios/chrome/browser/content_suggestions/

+  if (headerItem) {
+    [model addItem:headerItem toSectionWithIdentifier:SectionIdentifierItems];

[gambard, 2017/04/10 09:47:35]

Use |setHeader:forSectionWithIdentifier:|?

[Moe, 2017/04/10 17:53:49]

Unfortunately we don't have support for arbitrary sized headers just yet:
https://cs.chromium.org/webrtc/src/ios/chrome/browser/ui/collection_view/coll...

[gambard, 2017/04/18 08:06:37]

Acknowledged.

+  }
+
+  NSArray<CollectionViewItem*>* selectableItems = [_dataSource selectableItems];
+  for (NSUInteger index = 0; index < selectableItems.count; ++index) {
+    CollectionViewItem* item = [selectableItems objectAtIndex:index];

[gambard, 2017/04/10 09:47:34]

Nit: you can use: selectableItems[index]

[Moe, 2017/04/10 17:53:49]

Done.

+    DCHECK(item);

[gambard, 2017/04/10 09:47:34]

You cannot have nil items in an array. You should remove this DCHECK.

[Moe, 2017/04/10 17:53:49]

Done.

+    item.accessibilityTraits |= UIAccessibilityTraitButton;
+    item.accessoryType = (index == _dataSource.selectedItemIndex)
+                             ? MDCCollectionViewCellAccessoryCheckmark
+                             : MDCCollectionViewCellAccessoryNone;
+    [model addItem:item toSectionWithIdentifier:SectionIdentifierItems];
+  }
+
+  if ([[_dataSource addButtonTitle] length]) {

[gambard, 2017/04/10 09:47:35]

Why are you returning items for all the others methods and a string for the
button?
+lpromero

[Moe, 2017/04/10 17:53:49]

Because the add button, if any, will be the same kind and style for all
instances of this view controller. why pushing the logic to the mediator when it
can be created and configured by the view controller.

+    PaymentsTextItem* addButton =
+        [[PaymentsTextItem alloc] initWithType:ItemTypeAddItem];
+    addButton.text = [_dataSource addButtonTitle];
+    addButton.image = NativeImage(IDR_IOS_PAYMENTS_ADD);
+    addButton.accessibilityTraits |= UIAccessibilityTraitButton;
+    [model addItem:addButton toSectionWithIdentifier:SectionIdentifierItems];
+  }
+}
+
+- (void)viewDidLoad {
+  [super viewDidLoad];
+  self.collectionView.accessibilityIdentifier =
+      kPaymentRequestSelectorCollectionViewAccessibilityID;
+
+  // Customize collection view settings.
+  self.styler.cellStyle = MDCCollectionViewCellStyleCard;
+  self.styler.separatorInset =
+      UIEdgeInsetsMake(0, kSeparatorEdgeInset, 0, kSeparatorEdgeInset);
+}
+
+#pragma mark UICollectionViewDataSource
+
+- (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView

[gambard, 2017/04/10 09:47:34]

I don't think this is how the items should be used. What do you think of
modifying the PaymentsTextCell to add a text color property and removing the
|collectionView:cellForItemAtIndexPath:|?

[Moe, 2017/04/10 17:53:49]

I agree. Done.

+                 cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
+  CollectionViewModel* model = self.collectionViewModel;
+
+  UICollectionViewCell* cell =
+      [super collectionView:collectionView cellForItemAtIndexPath:indexPath];
+
+  CollectionViewItem* item = [model itemAtIndexPath:indexPath];
+  switch (item.type) {
+    case ItemTypeSelectableItem: {

[lpromero, 2017/04/11 15:59:06]

Move this right above the default case to bundle them in one break statement.

+      break;
+    }
+    case ItemTypeHeader: {
+      if ([cell isKindOfClass:[PaymentsTextCell class]]) {

[gambard, 2017/04/10 09:47:35]

I think this is a discrepancy between the idea you are trying to implement with
passing directly the item to the view controller (i.e. in my understanding the
View Controller is agnostic of the kind of item it receives), and this function
where the ViewController needs to know exactly the class of the item it
received.
+lpromero

[Moe, 2017/04/10 17:53:49]

I see your point of view. Again this was another point I discussed with
lpromero@ prior to implementing this. In fact what happening here isn't correct.
the mediator who passes the item to the view controller should also be
responsible for configuring the item and therefore the cell. Otherwise, the
configured cell that's returned by this method can be overridden by
"item.configureCell". If the item is configured in the mediator, the view
controller can be "more or less" completely agnostic of the kind of items it
displays. 

[lpromero, 2017/04/11 15:59:06]

It's the idea of an agnostic view controller that Rohit wants to avoid, though.

[Moe, 2017/04/14 06:05:49]

Brought this back as we decided the mediators shouldn't be styling the cells.

+        PaymentsTextCell* headerCell =
+            base::mac::ObjCCastStrict<PaymentsTextCell>(cell);
+        headerCell.textLabel.textColor =
+            _dataSource.state == PaymentRequestSelectorViewControllerStateError

[gambard, 2017/04/10 09:47:35]

Does the data source state is supposed to change over time?
This means the state would be updated only when the cells are displayed the
first time or when the cell is reused (not sure it is what you want).

[Moe, 2017/04/10 17:53:49]

because the state changes always involve going to/from the pending state, the
coordinator asks view controller to reload the view. So this should work just
fine.   

[gambard, 2017/04/18 08:06:37]

Acknowledged.

+                ? [[MDCPalette cr_redPalette] tint600]
+                : [[MDCPalette greyPalette] tint600];
+      }
+      break;
+    }
+    case ItemTypeAddItem: {
+      PaymentsTextCell* addItemCell =
+          base::mac::ObjCCastStrict<PaymentsTextCell>(cell);
+      addItemCell.textLabel.textColor = [[MDCPalette greyPalette] tint900];
+      break;
+    }
+    default:
+      break;
+  }
+  return cell;
+}
+
+#pragma mark UICollectionViewDelegate
+
+- (void)collectionView:(UICollectionView*)collectionView
+    didSelectItemAtIndexPath:(NSIndexPath*)indexPath {
+  [super collectionView:collectionView didSelectItemAtIndexPath:indexPath];
+
+  CollectionViewModel* model = self.collectionViewModel;
+
+  CollectionViewItem* item = [model itemAtIndexPath:indexPath];
+  switch (item.type) {
+    case ItemTypeSelectableItem: {
+      // Update the currently selected cell, if any.
+      if (_dataSource.selectedItemIndex != NSUIntegerMax) {
+        CollectionViewItem* selectedItem =
+            [_dataSource selectableItemAtIndex:_dataSource.selectedItemIndex];
+        DCHECK(selectedItem);

[gambard, 2017/04/10 09:47:35]

Same, I don't think you need this DCHECK.

[Moe, 2017/04/10 17:53:49]

Done.

+        selectedItem.accessoryType = MDCCollectionViewCellAccessoryNone;
+        [self reconfigureCellsForItems:@[ selectedItem ]
+               inSectionWithIdentifier:SectionIdentifierItems];
+      }
+
+      // Update the data source with the selection.
+      NSUInteger index =
+          [self.collectionViewModel indexInItemTypeForIndexPath:indexPath];
+      DCHECK(index < [[_dataSource selectableItems] count]);
+      [_dataSource setSelectedItemIndex:index];

[gambard, 2017/04/10 09:47:34]

I don't think the ViewController should set things directly in the data source.
Can this be routed through the delegate?

[Moe, 2017/04/10 17:53:49]

Good point. Done.

+
+      // Update the newly selected cell.
+      item.accessoryType = MDCCollectionViewCellAccessoryCheckmark;
+      [self reconfigureCellsForItems:@[ item ]
+             inSectionWithIdentifier:SectionIdentifierItems];
+
+      // Notify the delegate of the selection.
+      [_delegate paymentRequestSelectorViewController:self
+                                 didSelectItemAtIndex:index];
+      break;
+    }
+    case ItemTypeAddItem: {
+      [_delegate paymentRequestSelectorViewControllerDidSelectAddItem:self];
+      break;
+    }
+    default:
+      break;
+  }
+}
+
+#pragma mark MDCCollectionViewStylingDelegate
+
+- (CGFloat)collectionView:(UICollectionView*)collectionView
+    cellHeightAtIndexPath:(NSIndexPath*)indexPath {
+  CollectionViewItem* item =
+      [self.collectionViewModel itemAtIndexPath:indexPath];
+  switch (item.type) {

[gambard, 2017/04/10 09:47:35]

Nit: I would remove the the switch. Is there a particular reason for adding it?

[Moe, 2017/04/10 17:53:48]

Done.

+    case ItemTypeSpinner:
+    case ItemTypeHeader:
+    case ItemTypeSelectableItem:
+    case ItemTypeAddItem:
+      return [MDCCollectionViewCell
+          cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds)

[gambard, 2017/04/10 09:47:35]

I don't know if this is relevant in your case, but as you are using the card
format, the width available for the cells is smaller than collectionView.bound.
You can get the real width by doing:
  UIEdgeInsets inset = [self collectionView:collectionView
                                     layout:collectionView.collectionViewLayout
                     insetForSectionAtIndex:indexPath.section];

  return [MDCCollectionViewCell
      cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
                                 inset.left - inset.right
                         forItem:item];

[Moe, 2017/04/10 17:53:49]

Definitely relevant. Thank you.

+                             forItem:item];
+    default:
+      NOTREACHED();
+      return MDCCellDefaultOneLineHeight;
+  }
+}
+
+- (BOOL)collectionView:(UICollectionView*)collectionView
+    hidesInkViewAtIndexPath:(NSIndexPath*)indexPath {
+  NSInteger type = [self.collectionViewModel itemTypeForIndexPath:indexPath];
+  if (type == ItemTypeHeader) {

[gambard, 2017/04/10 09:47:34]

I would have say the ink does not appear on headers by default. Is it the case
now?

[Moe, 2017/04/10 17:53:49]

please see above.

+    return YES;
+  } else {
+    return NO;
+  }
+}
+
+@end