OneClickSigninBubbleView: Only show title if no error present. [Mac+Views]

chrome/browser/ui/cocoa/one_click_signin_view_controller.mm

Index: chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
diff --git a/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm b/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
index 5e4d9677656be4c8629a1f96ae0a26f03caf024c..3882b05378ee55b75d3e66d69ded6b53c57aaeb9 100644
--- a/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
+++ b/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
@@ -21,6 +21,8 @@
 
 namespace {
 
+const CGFloat kTopErrorMsgMargin = 12;

[groby-ooo-7-16, 2014/01/27 21:28:18]

Please don't abbreviate. Also, what is this constant for, and why 12?

+
 // Shift the origin of |view|'s frame by the given amount in the
 // positive y direction (up).
 void ShiftOriginY(NSView* view, CGFloat amount) {
@@ -152,6 +154,14 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
   }
 
   if (informativePlaceholderTextField_) {
+    if (!isSyncDialog_ && ([errorMessage_ length] != 0)) {
+      // Move up the "Learn more" origin in error case to account for the
+      // smaller bubble..
+      NSRect oldFrame = [informativePlaceholderTextField_ frame];
+      oldFrame.origin.y += [titleTextField_ frame].size.height;

[groby-ooo-7-16, 2014/01/27 21:28:18]

Use rectangle helpers instead:

oldFrame = NSOffsetRect(oldFrame, 0, NSHeight([titleTextField_ frame]));

[Oren Blasberg, 2014/01/29 23:52:19]

Done.

+      [informativePlaceholderTextField_ setFrame:oldFrame];
+    }
+
     ShiftOriginY(informativePlaceholderTextField_, totalYOffset);
     totalYOffset += [self initializeInformativeTextView];
   }
@@ -166,16 +176,34 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
       [GTMUILocalizerAndLayoutTweaker
           sizeToFitFixedWidthTextField:titleTextField_];
 
-  if (closeButton_)
-    ShiftOriginY(closeButton_, totalYOffset);
-
   NSSize delta = NSMakeSize(0.0, totalYOffset);
 
   if (isSyncDialog_) {
     [messageTextField_ setStringValue:l10n_util::GetNSStringWithFixup(
         IDS_ONE_CLICK_SIGNIN_DIALOG_TITLE_NEW)];
   } else if ([errorMessage_ length] != 0) {
+    [titleTextField_ setStringValue:@""];

[groby-ooo-7-16, 2014/01/27 21:28:18]

Can you set field values before doing the layout, and then just layout based on
the contents?

Also, if you want to hide the titleTextField, [titleTextField_ setHidden:YES] is
a better choice, I think.

[Oren Blasberg, 2014/01/29 23:52:19]

Could you elaborate on what you mean by "doing the layout" and "layout based on
the contents"?

[groby-ooo-7-16, 2014/01/30 23:12:41]

"Doing the layout" ==> Arranging where fields show up.
"Layout based on contents" ==> Happens after individual controls are sized
properly. You now know the size of all elements based on what they contain, and
can arrange them in one go.

     [messageTextField_ setStringValue:errorMessage_];
+
+    // Make the bubble less tall, as the title text will be hidden.
+    NSSize size = [[self view] frame].size;
+    size.height = size.height - [titleTextField_ frame].size.height;
+    [[self view] setFrameSize:size];

[groby-ooo-7-16, 2014/01/27 21:28:18]

If you do this as part of the layout flow, the change will be accounted for in
totalYOffset/delta, and the final resizeViewWithoutAutoResizingSubviews will
automatically do the right thing.

[Oren Blasberg, 2014/01/29 23:52:19]

Same here: what do you mean by layout flow?  Are you referring to the
ShiftOriginY / totalYOffset stuff above?

[groby-ooo-7-16, 2014/01/30 23:12:41]

Yep - arranging individual controls so they look nice. There, you can just skip
arranging the title field at all if it is hidden. Makes sense? (If it doesn't,
let's sit down for a sec. )

+
+    // Shift the message text up to where the title text used to be.

[groby-ooo-7-16, 2014/01/27 21:28:18]

Why shift the messageTextField at all? Won't the resizing of the outer view
already make sure it's at the top?

[Oren Blasberg, 2014/01/29 23:52:19]

That is not happening for some reason. :/ Perhaps addressing your above comments
regarding layout will help make this work?

[groby-ooo-7-16, 2014/01/30 23:12:41]

Weird. If the field is outside the view's bounds, it shouldn't render.

+    NSPoint origin = [titleTextField_ frame].origin;
+    [messageTextField_ setFrameOrigin:origin];
+    ShiftOriginY(messageTextField_, -kTopErrorMsgMargin);
+
+    // Use "OK" instead of "OK, got it" in the error case, and size the button
+    // accordingly.
+    [closeButton_ setTitle:l10n_util::GetNSStringWithFixup(
+        IDS_OK)];
+    NSRect cbFrame = [closeButton_ frame];

[groby-ooo-7-16, 2014/01/27 21:28:18]

Please don't use abbreviations. closeButtonFrame would be a good name.

[Oren Blasberg, 2014/01/29 23:52:19]

Removed.

+    [closeButton_ sizeToFit];
+    NSRect fittedCbFrame = [closeButton_ frame];
+    fittedCbFrame.origin.x += cbFrame.size.width - fittedCbFrame.size.width;
+    [closeButton_ setFrame:fittedCbFrame];

[groby-ooo-7-16, 2014/01/27 21:28:18]

Actually, since the view is already in a GTMWidthBasedTweaker, you could just
use the layout tweaker. Make sure the button/tweaker is anchored to the bottom
right.

[GTMUILocalizerAndLayoutTweaker sizeToFitView:[closeButton_ superview]];

[Oren Blasberg, 2014/01/29 23:52:19]

Done.

   }
 
   // Resize bubble and window to hold the controls.