[Payments] Update autofill/payments spacings

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
index 72b617829117135860efed83b064436208c06335..e46020bdbfc1641e54f972ab09b92ba2a63c99c2 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
@@ -58,10 +58,13 @@ public class EditorView extends AlwaysDismissedDialog
     private final Handler mHandler;
     private final AsyncTask<Void, Void, PhoneNumberFormattingTextWatcher> mPhoneFormatterTask;
     private final TextView.OnEditorActionListener mEditorActionListener;
+    private final int mHalfRowMargin;
 
     private ViewGroup mLayout;
     private EditorModel mEditorModel;
     private Button mDoneButton;
+    private ViewGroup mDataView;
+    private View mFooter;
     @Nullable private AutoCompleteTextView mPhoneInput;
 
     /**
@@ -98,6 +101,9 @@ public class EditorView extends AlwaysDismissedDialog
                 return false;
             }
         };
+
+        mHalfRowMargin = activity.getResources().getDimensionPixelSize(

[Ian Wen, 2016/07/01 19:19:03]

Aha yes! I am always amazed at how Android can't handle such simplest spacing
matters in listviews and recycler views.

[gone, 2016/07/01 23:00:49]

Lol well... the simplest case is 0 margin...

+                R.dimen.payments_section_large_spacing);
     }
 
     /** Launches the Autofill help page on top of the current Context. */
@@ -157,21 +163,26 @@ public class EditorView extends AlwaysDismissedDialog
      */
     private boolean validateForm() {
         final List<EditorTextField> invalidViews = getViewsWithInvalidInformation();
-        if (invalidViews.isEmpty()) return true;
 
         // Focus the first field that's invalid.
         if (!invalidViews.contains(getCurrentFocus())) focusInputField(invalidViews.get(0));

[please use gerrit instead, 2016/07/01 06:14:12]

fyi, we should check for empty invalidViews here.

[gone, 2016/07/01 18:31:39]

Should be fine; contains would fail wouldn't it?

[gone, 2016/07/01 23:00:49]

D'oh, I get it.  Added an empty check.

 
         // Iterate over all the fields to update what errors are displayed, which is necessary to
         // to clear existing errors on any newly valid fields.
-        ViewGroup dataView = (ViewGroup) mLayout.findViewById(R.id.contents);
-        for (int i = 0; i < dataView.getChildCount(); i++) {
-            if (!(dataView.getChildAt(i) instanceof EditorTextField)) continue;
-            EditorTextField fieldView = (EditorTextField) dataView.getChildAt(i);
-            fieldView.updateDisplayedError(invalidViews.contains(fieldView));
-        }
+        validateForm(invalidViews, mDataView);
+        return invalidViews.isEmpty();
+    }
 
-        return false;
+    private void validateForm(List<EditorTextField> invalidViews, ViewGroup root) {

[Ian Wen, 2016/07/01 19:19:03]

This is essentially traversing the view hierarchy. Could we create a list of all
EditorFields, and traverse the list instead?

[gone, 2016/07/01 23:00:49]

Huh.  Yeah, that makes a ton more sense.

+        for (int i = 0; i < root.getChildCount(); i++) {
+            View child = root.getChildAt(i);
+            if (child instanceof EditorTextField) {
+                EditorTextField fieldView = (EditorTextField) child;
+                fieldView.updateDisplayedError(invalidViews.contains(fieldView));
+            } else if (child instanceof ViewGroup) {
+                validateForm(invalidViews, (ViewGroup) child);
+            }
+        }
     }
 
     @Override
@@ -210,47 +221,86 @@ public class EditorView extends AlwaysDismissedDialog
     /**
      * Create the visual representation of the EditorModel.
      *
-     * Fields are added to the layout at position |getChildCount() - 1| to account for the
-     * additional TextView that says "* indicates required field" at the bottom of the layout.
-     *
-     * TODO(rouslan): Put views side by side if !fieldModel.isFullLine();
+     * This would be more optimal as a RelativeLayout, but because it's dynamically generated, it's
+     * much more human-parsable with inefficient LinearLayouts for half-width controls sharing rows.
      */
     private void prepareEditor() {
-        final ViewGroup dataView = (ViewGroup) mLayout.findViewById(R.id.contents);
+        // Ensure the layout is empty.
+        mDataView = (ViewGroup) mLayout.findViewById(R.id.contents);
+        mDataView.removeAllViews();
+
+        // Add Views for each of the {@link EditorFields}.
         for (int i = 0; i < mEditorModel.getFields().size(); i++) {
-            final EditorFieldModel fieldModel = mEditorModel.getFields().get(i);
-
-            if (fieldModel.getInputTypeHint() == EditorFieldModel.INPUT_TYPE_HINT_DROPDOWN) {
-                EditorDropdownField dropdownView = new EditorDropdownField(mContext, fieldModel,
-                        new Runnable() {
-                            @Override
-                            public void run() {
-                                removeTextChangedListenerFromPhoneInputField();
-                                // Do not remove the "* indicates required field" label at the
-                                // bottom.
-                                dataView.removeViews(0, dataView.getChildCount() - 1);
-                                prepareEditor();
-                                if (mObserverForTest != null) {
-                                    mObserverForTest.onPaymentRequestReadyToEdit();
-                                }
-                            }
-                        });
-
-                dataView.addView(dropdownView.getLabel(), dataView.getChildCount() - 1);
-                dataView.addView(dropdownView.getDropdown(), dataView.getChildCount() - 1);
+            EditorFieldModel fieldModel = mEditorModel.getFields().get(i);
+            EditorFieldModel nextFieldModel = null;
+
+            boolean isLastField = i == mEditorModel.getFields().size() - 1;
+            boolean useFullLine = fieldModel.isFullLine();
+            if (!isLastField && !useFullLine) {
+                // If the next field isn't full, stretch it out.
+                nextFieldModel = mEditorModel.getFields().get(i + 1);
+                if (nextFieldModel.isFullLine()) useFullLine = true;
+            }
+
+            if (useFullLine) {
+                addFieldViewToEditor(mDataView, fieldModel);
             } else {
-                EditorTextField inputLayout = new EditorTextField(mLayout.getContext(), fieldModel,
-                        mEditorActionListener, getPhoneFormatter(), mObserverForTest);
+                // Create a LinearLayout to put it and the next view side by side.
+                LinearLayout rowLayout = new LinearLayout(mContext);
+                mDataView.addView(rowLayout);
+
+                View firstView = addFieldViewToEditor(rowLayout, fieldModel);
+                View lastView = addFieldViewToEditor(rowLayout, nextFieldModel);
+
+                LinearLayout.LayoutParams firstParams =
+                        (LinearLayout.LayoutParams) firstView.getLayoutParams();
+                LinearLayout.LayoutParams lastParams =
+                        (LinearLayout.LayoutParams) lastView.getLayoutParams();
+
+                firstParams.width = 0;
+                firstParams.weight = 1;
+                firstParams.setMarginEnd(mHalfRowMargin);
+                lastParams.width = 0;
+                lastParams.weight = 1;
+                i = i + 1;
+            }
+        }
 
-                final AutoCompleteTextView input = inputLayout.getEditText();
-                if (fieldModel.getInputTypeHint() == EditorFieldModel.INPUT_TYPE_HINT_PHONE) {
-                    assert mPhoneInput == null;
-                    mPhoneInput = input;
-                }
+        // Add the footer.
+        mDataView.addView(mFooter);
+    }
 
-                dataView.addView(inputLayout, dataView.getChildCount() - 1);
+    private View addFieldViewToEditor(ViewGroup parent, EditorFieldModel fieldModel) {
+        View childView = null;
+
+        if (fieldModel.getInputTypeHint() == EditorFieldModel.INPUT_TYPE_HINT_DROPDOWN) {
+            Runnable prepareEditorRunnable = new Runnable() {
+                @Override
+                public void run() {
+                    // The fields may have changed.
+                    prepareEditor();
+                    if (mObserverForTest != null) mObserverForTest.onPaymentRequestReadyToEdit();
+                }
+            };
+            EditorDropdownField dropdownView =
+                    new EditorDropdownField(mContext, fieldModel, prepareEditorRunnable);
+
+            childView = dropdownView.getLayout();
+        } else {
+            EditorTextField inputLayout = new EditorTextField(mLayout.getContext(), fieldModel,
+                    mEditorActionListener, getPhoneFormatter(), mObserverForTest);
+
+            final AutoCompleteTextView input = inputLayout.getEditText();
+            if (fieldModel.getInputTypeHint() == EditorFieldModel.INPUT_TYPE_HINT_PHONE) {
+                assert mPhoneInput == null;
+                mPhoneInput = input;
             }
+
+            childView = inputLayout;
         }
+
+        parent.addView(childView);
+        return childView;
     }
 
     /**
@@ -265,6 +315,10 @@ public class EditorView extends AlwaysDismissedDialog
         mLayout = (LinearLayout) LayoutInflater.from(mContext).inflate(
                 R.layout.payment_request_editor, null);
         setContentView(mLayout);
+
+        mFooter = LayoutInflater.from(mContext).inflate(
+                R.layout.payment_request_editor_footer, null, false);

[Ian Wen, 2016/07/01 19:19:03]

Use mLayout here as parent instead of null? If you specify parent in inflate(),
layout params will be correct and it might save you some trouble.

[gone, 2016/07/01 23:00:49]

Problem is that we don't want to add it to the hierarchy, yet.  I doubt that the
View would keep the right kind of LayoutParams when it gets removed from its
parent.

+
         prepareToolbar();
         prepareButtons();
         prepareEditor();
@@ -305,17 +359,22 @@ public class EditorView extends AlwaysDismissedDialog
     }
 
     private List<EditorTextField> getViewsWithInvalidInformation() {
-        ViewGroup container = (ViewGroup) findViewById(R.id.contents);
-
         List<EditorTextField> invalidViews = new ArrayList<>();
-        for (int i = 0; i < container.getChildCount(); i++) {
-            View layout = container.getChildAt(i);
-            if (!(layout instanceof EditorTextField)) continue;
+        getViewsWithInvalidInformation(invalidViews, mDataView);
+        return invalidViews;
+    }
 
-            EditorTextField fieldView = (EditorTextField) layout;
-            if (!fieldView.getFieldModel().isValid()) invalidViews.add(fieldView);
+    private void getViewsWithInvalidInformation(
+            List<EditorTextField> invalidViews, ViewGroup root) {
+        for (int i = 0; i < root.getChildCount(); i++) {
+            View child = root.getChildAt(i);
+            if (child instanceof EditorTextField) {
+                EditorTextField fieldView = (EditorTextField) child;
+                if (!fieldView.getFieldModel().isValid()) invalidViews.add(fieldView);
+            } else if (child instanceof ViewGroup) {
+                getViewsWithInvalidInformation(invalidViews, (ViewGroup) child);
+            }
         }
-        return invalidViews;
     }
 
     private void focusInputField(View view) {