[Payment Request] Generic edit form

ios/chrome/browser/payments/payment_request_edit_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_edit_view_controller.h"
+
+#include "base/logging.h"
+#import "base/mac/foundation_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "components/strings/grit/components_strings.h"
+#import "ios/chrome/browser/payments/cells/payments_text_item.h"
+#import "ios/chrome/browser/ui/collection_view/cells/MDCCollectionViewCell+Chrome.h"
+#import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h"
+#import "ios/chrome/browser/ui/settings/autofill_edit_accessory_view.h"
+#import "ios/chrome/browser/ui/settings/cells/autofill_edit_item.h"
+#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#include "ios/chrome/grit/ios_theme_resources.h"
+#import "ios/third_party/material_components_ios/src/components/Typography/src/MaterialTypography.h"
+#include "ui/base/l10n/l10n_util.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace {
+
+const CGFloat kSeparatorEdgeInset = 14;
+
+AutofillEditCell* AutofillEditCellForTextField(UITextField* textField) {
+  for (UIView* view = textField; view; view = [view superview]) {
+    AutofillEditCell* cell = base::mac::ObjCCast<AutofillEditCell>(view);
+    if (cell)
+      return cell;
+  }
+
+  // There has to be a cell associated with this text field.
+  NOTREACHED();
+  return nil;
+}
+
+}  // namespace
+
+@interface PaymentRequestEditViewController ()<AutofillEditAccessoryDelegate> {
+  // The currently focused cell. May be nil.
+  __weak AutofillEditCell* _currentEditingCell;
+
+  AutofillEditAccessoryView* _accessoryView;
+}
+
+// Returns the index path for the cell associated with the currently focused
+// text field.
+- (NSIndexPath*)indexPathForCurrentTextField;
+
+// Returns the validation error string for |value| for the autofill type
+// |autofillType|. |required| indicates wether this is a required field or not.

[lpromero, 2017/03/16 13:01:07]

s/wether/whether. I just found out "wether" exists too!
https://www.google.fr/search?q=wether

[Moe, 2017/03/16 15:44:23]

Done! definitely whether and not wether here :)

+// If there are no validation errors an empty string is returned.

[lpromero, 2017/03/16 13:01:07]

Comma after "errors".

[Moe, 2017/03/16 15:44:23]

Done.

+- (NSString*)validateValue:(NSString*)value
+              autofillType:(autofill::ServerFieldType)autofillType
+                  required:(BOOL)required;
+
+// Adds an error message item in the section |sectionIdentifier| if
+// |errorMessage| is non-empty. Otherwise removes such an item if one exists.
+- (void)addOrRemoveErrorMessage:(NSString*)errorMessage
+        inSectionWithIdentifier:(NSInteger)sectionIdentifier;
+
+// Returns the indexPath for the same row as that of |indexPath| in a section
+// with the given offset relative to that of |indexPath|. It may return nil.
+- (NSIndexPath*)indexPathWithSectionOffset:(NSInteger)offset
+                                  fromPath:(NSIndexPath*)indexPath;
+
+// Returns the text field with the given offset relative to the currently
+// focused text field. May return nil.
+- (AutofillEditCell*)nextTextFieldWithOffset:(NSInteger)offset;
+
+// Enables or Disables the accessory view's previous and next buttons depending

[lpromero, 2017/03/16 13:01:06]

s/Disables/disables

[Moe, 2017/03/16 15:44:22]

Done.

+// on wehther there is a text field before and after the currently focused text

[lpromero, 2017/03/16 13:01:06]

whether

[Moe, 2017/03/16 15:44:23]

Done. Sorry. dyslexia...

+// field.
+- (void)updateAccessoryViewButtonState;

[lpromero, 2017/03/16 13:01:06]

There should be a plural somewhere. updateAccessoryViewButtonsStates?

[Moe, 2017/03/16 15:44:23]

Done.

+
+@end
+
+@implementation PaymentRequestEditViewController
+
+- (instancetype)init {
+  self = [super initWithStyle:CollectionViewControllerStyleAppBar];
+  if (self) {
+    _accessoryView = [[AutofillEditAccessoryView alloc] initWithDelegate:self];
+  }
+  return self;
+}
+
+- (void)viewDidAppear:(BOOL)animated {
+  [super viewDidAppear:animated];
+  [[NSNotificationCenter defaultCenter]
+      addObserver:self
+         selector:@selector(keyboardDidShow)
+             name:UIKeyboardDidShowNotification
+           object:nil];
+}
+
+- (void)viewWillDisappear:(BOOL)animated {
+  [super viewWillDisappear:animated];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver:self
+                name:UIKeyboardDidShowNotification
+              object:nil];
+}
+
+#pragma mark - CollectionViewController methods
+
+- (void)loadModel {
+  [super loadModel];
+  CollectionViewModel* model = self.collectionViewModel;
+
+  // Iterate over the field definitions and add the respective sections and
+  // items.
+  for (const auto& field : [self editorFields]) {
+    [model addSectionWithIdentifier:field.section_id];
+    AutofillEditItem* item =
+        [[AutofillEditItem alloc] initWithType:field.item_type];
+    NSString* labelFormat = field.required ? @"%@*" : @"%@";

[lpromero, 2017/03/16 13:01:07]

Since AutofillEditItem knows about the "required" concept (it has a "required"
property), I think this should be handled by the associated cell. You can add an
example to material_cell_catalog to show a "required" item for example.

[Moe, 2017/03/16 15:44:23]

Done.

+    item.textFieldName = [NSString
+        stringWithFormat:labelFormat, base::SysUTF16ToNSString(field.label)];
+    item.textFieldEnabled = YES;
+    item.required = field.required;
+    item.autofillType = field.data_type;
+    [model addItem:item toSectionWithIdentifier:field.section_id];
+  }
+}
+
+- (void)viewDidLoad {
+  [super viewDidLoad];
+
+  // Customize collection view settings.
+  self.styler.cellStyle = MDCCollectionViewCellStyleCard;
+  self.styler.separatorInset =
+      UIEdgeInsetsMake(0, kSeparatorEdgeInset, 0, kSeparatorEdgeInset);
+}
+
+#pragma mark - UITextFieldDelegate
+
+- (void)textFieldDidBeginEditing:(UITextField*)textField {
+  _currentEditingCell = AutofillEditCellForTextField(textField);
+  [textField setInputAccessoryView:_accessoryView];
+  [self updateAccessoryViewButtonState];
+}
+
+- (void)textFieldDidEndEditing:(UITextField*)textField {
+  DCHECK(_currentEditingCell == AutofillEditCellForTextField(textField));
+  CollectionViewModel* model = self.collectionViewModel;
+
+  // Validate the text field. If there is a validation error, display an error
+  // message item in the same section as the field. Otherwise remove the error
+  // message item in that section.
+  NSIndexPath* indexPath = [self indexPathForCurrentTextField];
+  AutofillEditItem* item = base::mac::ObjCCastStrict<AutofillEditItem>(
+      [model itemAtIndexPath:indexPath]);
+
+  NSString* errorMessage = [self validateValue:textField.text
+                                  autofillType:item.autofillType
+                                      required:item.required];
+  NSInteger sectionIdentifier =
+      [model sectionIdentifierForSection:[indexPath section]];
+  [self addOrRemoveErrorMessage:errorMessage
+        inSectionWithIdentifier:sectionIdentifier];
+
+  [textField setInputAccessoryView:nil];
+  _currentEditingCell = nil;
+}
+
+- (BOOL)textFieldShouldReturn:(UITextField*)textField {
+  DCHECK([_currentEditingCell textField] == textField);
+  [self nextPressed];
+  return NO;
+}
+
+#pragma mark - AutofillEditAccessoryDelegate
+
+- (void)nextPressed {
+  AutofillEditCell* nextCell = [self nextTextFieldWithOffset:1];
+  if (nextCell)
+    [nextCell.textField becomeFirstResponder];
+}
+
+- (void)previousPressed {
+  AutofillEditCell* previousCell = [self nextTextFieldWithOffset:-1];
+  if (previousCell)
+    [previousCell.textField becomeFirstResponder];
+}
+
+- (void)closePressed {
+  [[_currentEditingCell textField] resignFirstResponder];
+}
+
+#pragma mark - UICollectionViewDataSource
+
+- (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView
+                 cellForItemAtIndexPath:(NSIndexPath*)indexPath {
+  UICollectionViewCell* cell =
+      [super collectionView:collectionView cellForItemAtIndexPath:indexPath];
+
+  if ([cell isMemberOfClass:[AutofillEditCell class]]) {

[lpromero, 2017/03/16 13:01:07]

Optional nit: is isKindOfClass more appropriate? We shouldn’t ever have
subclasses of AutofillEditCell, so we might not be hit, but conceptually, it
seems you want to use isKindOfClass.

-isKindOfClass: is a "class" or subclass of "class"
-isMemberOfClass: is a "class"

[Moe, 2017/03/16 15:44:23]

Done.

+    AutofillEditCell* autofillEditCell =
+        base::mac::ObjCCast<AutofillEditCell>(cell);
+    autofillEditCell.textField.delegate = self;
+  }
+
+  NSInteger itemType =
+      [self.collectionViewModel itemTypeForIndexPath:indexPath];
+  switch (itemType) {
+    case ItemTypeErrorMessage: {
+      PaymentsTextCell* errorMessageCell =
+          base::mac::ObjCCastStrict<PaymentsTextCell>(cell);
+      errorMessageCell.textLabel.font = [MDCTypography body1Font];
+      errorMessageCell.textLabel.textColor =
+          [[MDCPalette cr_redPalette] tint600];
+      break;
+    }
+    default:
+      break;
+  }
+
+  return cell;
+}
+
+#pragma mark MDCCollectionViewStylingDelegate
+
+- (CGFloat)collectionView:(UICollectionView*)collectionView
+    cellHeightAtIndexPath:(NSIndexPath*)indexPath {
+  CollectionViewItem* item =
+      [self.collectionViewModel itemAtIndexPath:indexPath];
+  switch (item.type) {
+    case ItemTypeErrorMessage:
+      return [MDCCollectionViewCell
+          cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds)
+                             forItem:item];
+    default:
+      NOTREACHED();

[lpromero, 2017/03/16 13:01:06]

How come we don't reach this section? The text fields items are not of
ItemTypeErrorMessage.

[Moe, 2017/03/16 15:44:22]

That's the subclasses who decide what the item types for the fields (and other
items they might add to the model) are and they should handle them before
calling the super method.
But I think it's worth adding a condition here to check if the item is a kind of
AutofillEditItem so that simple subclasses don't need to override this method.  

+      return MDCCellDefaultOneLineHeight;
+  }
+}
+
+- (BOOL)collectionView:(UICollectionView*)collectionView
+    hidesInkViewAtIndexPath:(NSIndexPath*)indexPath {
+  NSInteger type = [self.collectionViewModel itemTypeForIndexPath:indexPath];
+  switch (type) {
+    case ItemTypeErrorMessage:
+      return YES;
+    default:
+      return NO;
+  }
+}
+
+#pragma mark - Protected methods
+
+- (std::vector<EditorField>)editorFields {
+  return std::vector<EditorField>();
+}
+
+- (NSIndexPath*)indexPathForCurrentTextField {

[lpromero, 2017/03/16 13:01:07]

This one is not protected, it is private.

[Moe, 2017/03/16 15:44:23]

Done.

+  DCHECK(_currentEditingCell);
+  NSIndexPath* indexPath =
+      [[self collectionView] indexPathForCell:_currentEditingCell];
+  DCHECK(indexPath);
+  return indexPath;
+}
+
+#pragma mark - Helper methods
+
+- (NSString*)validateValue:(NSString*)value
+              autofillType:(autofill::ServerFieldType)autofillType
+                  required:(BOOL)required {
+  if (required && value.length == 0) {
+    return l10n_util::GetNSString(
+        IDS_PAYMENTS_FIELD_REQUIRED_VALIDATION_MESSAGE);
+  }
+  return @"";
+}
+
+- (void)addOrRemoveErrorMessage:(NSString*)errorMessage
+        inSectionWithIdentifier:(NSInteger)sectionIdentifier {
+  CollectionViewModel* model = self.collectionViewModel;
+  if ([model hasItemForItemType:ItemTypeErrorMessage
+              sectionIdentifier:sectionIdentifier]) {
+    [model removeItemWithType:ItemTypeErrorMessage
+        fromSectionWithIdentifier:sectionIdentifier];
+  }
+
+  if (errorMessage.length != 0) {
+    PaymentsTextItem* errorMessageItem =
+        [[PaymentsTextItem alloc] initWithType:ItemTypeErrorMessage];
+    errorMessageItem.text = errorMessage;
+    errorMessageItem.image = NativeImage(IDR_IOS_PAYMENTS_WARNING);
+    [model addItem:errorMessageItem toSectionWithIdentifier:sectionIdentifier];
+  }
+
+  // Update the entire section.
+  NSInteger section = [model sectionForSectionIdentifier:sectionIdentifier];
+  [self.collectionView reloadSections:[NSIndexSet indexSetWithIndex:section]];

[lpromero, 2017/03/16 13:01:07]

Does that mean that the error message doesn't animate in? 

[Moe, 2017/03/16 15:44:23]

Done.

+}
+
+- (NSIndexPath*)indexPathWithSectionOffset:(NSInteger)offset
+                                  fromPath:(NSIndexPath*)indexPath {
+  DCHECK(indexPath);
+  DCHECK(offset);
+  NSInteger nextSection = [indexPath section] + offset;
+  if (nextSection >= 0 &&
+      nextSection < [[self collectionView] numberOfSections]) {
+    return [NSIndexPath indexPathForRow:[indexPath row] inSection:nextSection];
+  }
+  return nil;
+}
+
+- (AutofillEditCell*)nextTextFieldWithOffset:(NSInteger)offset {
+  UICollectionView* collectionView = [self collectionView];
+  NSIndexPath* currentCellPath = [self indexPathForCurrentTextField];
+  DCHECK(currentCellPath);
+  NSIndexPath* nextCellPath =
+      [self indexPathWithSectionOffset:offset fromPath:currentCellPath];
+  if (nextCellPath) {
+    id nextCell = [collectionView cellForItemAtIndexPath:nextCellPath];
+    if ([nextCell isMemberOfClass:[AutofillEditCell class]]) {

[lpromero, 2017/03/16 13:01:06]

Idem, would isKindOfClass be better?

[Moe, 2017/03/16 15:44:23]

Done.

+      return base::mac::ObjCCastStrict<AutofillEditCell>(
+          [collectionView cellForItemAtIndexPath:nextCellPath]);
+    }
+  }
+  return nil;
+}
+
+- (void)updateAccessoryViewButtonState {
+  AutofillEditCell* previousCell = [self nextTextFieldWithOffset:-1];
+  [[_accessoryView previousButton] setEnabled:previousCell != nil];
+
+  AutofillEditCell* nextCell = [self nextTextFieldWithOffset:1];
+  [[_accessoryView nextButton] setEnabled:nextCell != nil];
+}
+
+#pragma mark - Keyboard handling
+
+- (void)keyboardDidShow {

[lpromero, 2017/03/16 13:01:06]

Weird, I thought UICollectionViewController already did that out of the box.

+  [self.collectionView scrollRectToVisible:[_currentEditingCell frame]

[lpromero, 2017/03/16 13:01:06]

No action needed: I wonder if using [self.collectionView
scrollToItemAtIndexPath:[self.collectionView
indexPathForCell:_currentEditingCell]
atScrollPosition:UICollectionViewScrollPositionCenteredVertically animated:YES]
would be less brittle. 

[Moe, 2017/03/16 15:44:23]

I copied this from autofill_edit_collection_view_controller.mm. Wouldn't it be a
weird experience if an already visible text field moved to the middle of the
screen once focused? Also if the user is using the accessory view buttons to
move up and down between the fields, it's probably enough to just bring the text
field into view. 

[lpromero, 2017/03/16 16:08:08]

Ok. For the up/down button, it doesn't apply here, because this method is not
called again, right? Where do you scroll to have the next textfield entirely
visible? Is it automatic?

[Moe, 2017/03/21 04:22:14]

true about the up/down buttons. If I'm understanding your second question
correctly, once the user touches a partially visible textfield (which causes the
keyboard to show), this method scrolls to make the textfield entirely visible. 

+                                  animated:YES];
+}
+
+@end