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

chrome/browser/ui/cocoa/one_click_signin_view_controller.mm

 // Copyright (c) 2012 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 "chrome/browser/ui/cocoa/one_click_signin_view_controller.h"
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/mac/bundle_locations.h"
 #import "chrome/browser/ui/chrome_style.h"
 #import "chrome/browser/ui/cocoa/hyperlink_text_view.h"
 #include "chrome/browser/ui/sync/one_click_signin_helper.h"
 #include "chrome/browser/ui/sync/one_click_signin_histogram.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "skia/ext/skia_utils_mac.h"
 #import "third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
 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) {
   NSPoint origin = [view frame].origin;
   origin.y += amount;
   [view setFrameOrigin:origin];
 }
 
 }  // namespace
 
@@ skipping 111 matching lines @@
     // Learn More link above.
     float advancedLinkHeightPlusPadding =
         [informativePlaceholderTextField_ frame].origin.y -
         [advancedLink_ frame].origin.y;
 
     totalYOffset -= advancedLinkHeightPlusPadding;
     [advancedLink_ removeFromSuperview];
   }
 
   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];
   }
 
   ShiftOriginY(messageTextField_, totalYOffset);
   totalYOffset +=
       [GTMUILocalizerAndLayoutTweaker
           sizeToFitFixedWidthTextField:messageTextField_];
 
   ShiftOriginY(titleTextField_, totalYOffset);
   totalYOffset +=
       [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.
   [GTMUILocalizerAndLayoutTweaker
       resizeViewWithoutAutoResizingSubViews:[self view]
                                       delta:delta];
 
   if (isSyncDialog_) {
     OneClickSigninHelper::LogConfirmHistogramValue(
         one_click_signin::HISTOGRAM_CONFIRM_SHOWN);
@@ skipping 79 matching lines @@
                                 content::PAGE_TRANSITION_LINK, false);
   webContents_->OpenURL(params);
   return YES;
 }
 
 - (void)close {
   base::ResetAndReturn(&closeCallback_).Run();
 }
 
 @end