Refactor media galleries dialogs on cocoa to extract common code

chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm

+// Copyright 2014 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/extensions/media_gallery_list_entry_view.h"
+
+#include "base/strings/sys_string_conversions.h"
+#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_control_utils.h"
+#include "chrome/browser/ui/chrome_style.h"
+#include "grit/theme_resources.h"
+#import "ui/base/cocoa/menu_controller.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace {
+
+const CGFloat kCheckboxMargin = 10;
+
+CGFloat MinIfNotZero(CGFloat a, CGFloat b) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

std::min(std::max(a, 0.0), std::max(b, 0.0)))

Equality comparison to 0.0 never ends well :)

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

a & b are both >= 0, so that reduces to std::min(a, b)
Removed...

+  if (a == 0)
+    return b;
+  if (b == 0)
+    return a;
+  return std::min(a, b);
+}
+
+}  // namespace
+
+const CGFloat kDetailGray = 0.625;

[groby-ooo-7-16, 2014/04/23 21:57:58]

Please keep consts together (and outside anon ns)

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+
+ui::MenuModel* MediaGalleryListEntryController::GetContextMenu(
+    MediaGalleryPrefId pref_id) {
+  return NULL;
+}
+
+@interface MediaGalleryListEntry ()
+- (void)onCheckboxToggled:(id)sender;
+- (void)onFolderViewerClicked:(id)sender;
+- (ui::MenuModel*)getContextMenu;
+- (void)layoutSubViews;
+@end
+
+
+@interface MediaGalleryButton : NSButton {
+ @private
+  MediaGalleryListEntry* controller_;  // |controller_| owns |self|.
+  base::scoped_nsobject<MenuController> menuController_;
+}
+
+- (id)initWithFrame:(NSRect)frameRect
+         controller:(MediaGalleryListEntry*)controller;
+- (NSMenu*)menuForEvent:(NSEvent*)theEvent;
+
+@end
+
+@implementation MediaGalleryButton
+
+- (id)initWithFrame:(NSRect)frameRect
+         controller:(MediaGalleryListEntry*)controller {
+  if ((self = [super initWithFrame:frameRect])) {
+    controller_ = controller;
+  }
+  return self;
+}
+
+- (NSMenu*)menuForEvent:(NSEvent*)theEvent {
+  menuController_.reset(
+    [[MenuController alloc] initWithModel:[controller_ getContextMenu]
+                   useWithPopUpButtonCell:NO]);
+  return [menuController_ menu];
+}
+
+@end
+
+
+@implementation MediaGalleryListEntry
+
+- (id)initWithFrame:(NSRect)frameRect
+         controller:(MediaGalleryListEntryController*)controller
+             prefId:(MediaGalleryPrefId)prefId
+        galleryName:(base::string16)galleryName
+          subscript:(base::string16)subscript
+            tooltip:(base::string16)tooltip
+   showFolderViewer:(bool)showFolderViewer {
+  if ((self = [super initWithFrame:frameRect])) {
+    controller_ = controller;
+    prefId_ = prefId;
+
+    NSString* nsTooltip = base::SysUTF16ToNSString(tooltip);
+
+    // Set a auto resize mask so that resizeWithOldSuperviewSize() is called.

[groby-ooo-7-16, 2014/04/23 21:57:58]

nit: -resizeWithOldSuperviewSize:

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+    // It is overridden so the particular mask doesn't matter.
+    [self setAutoresizingMask:NSViewWidthSizable];
+    checkbox_.reset(
+        [[MediaGalleryButton alloc] initWithFrame:NSZeroRect
+                                       controller:self]);
+    [[checkbox_ cell] setLineBreakMode:NSLineBreakByTruncatingMiddle];
+    [checkbox_ setButtonType:NSSwitchButton];
+    [checkbox_ setTarget:self];
+    [checkbox_ setAction:@selector(onCheckboxToggled:)];
+
+    [checkbox_ setTitle:base::SysUTF16ToNSString(galleryName)];
+    [checkbox_ setToolTip:nsTooltip];
+
+    // Folder viewer button.
+    if (showFolderViewer) {
+      folderViewer_.reset(
+          [[MediaGalleryButton alloc] initWithFrame:NSZeroRect
+                                         controller:self]);
+      [folderViewer_ setButtonType:NSMomentaryChangeButton];
+      [folderViewer_ setTarget:self];
+      [folderViewer_ setAction:@selector(onFolderViewerClicked:)];
+
+      ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+      [folderViewer_ setImage:rb.GetNativeImageNamed(
+          IDR_FILE_FOLDER).ToNSImage()];
+      [folderViewer_ setTitle:nil];
+      [folderViewer_ setBordered:false];
+      [folderViewer_ setToolTip:nsTooltip];
+    }
+
+    // Additional details text.
+    if (!subscript.empty()) {
+      details_.reset([[NSTextField alloc] initWithFrame:NSZeroRect]);
+      [[details_ cell] setLineBreakMode:NSLineBreakByTruncatingHead];
+      [details_ setEditable:NO];
+      [details_ setSelectable:NO];
+      [details_ setBezeled:NO];
+      [details_ setAttributedStringValue:
+          constrained_window::GetAttributedLabelString(
+              base::SysUTF16ToNSString(subscript),
+              chrome_style::kTextFontStyle,
+              NSNaturalTextAlignment,
+              NSLineBreakByClipping
+          )];
+      [details_ setTextColor:[NSColor colorWithCalibratedRed:kDetailGray

[groby-ooo-7-16, 2014/04/23 21:57:58]

+colorWithCalibratedWhite:alpha: if you must specify a specific value.

I'd suggest +disabledControlTextColor, though. (It's slightly darker than .625,
but fits into Apple color schemes.)

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+                                                       green:kDetailGray
+                                                        blue:kDetailGray
+                                                       alpha:1.0]];
+    }
+
+    [self layoutSubViews];
+
+    [self addSubview:checkbox_];
+    if (showFolderViewer)
+      [self addSubview:folderViewer_];
+    if (!subscript.empty())
+      [self addSubview:details_];
+  }
+  return self;
+}
+
+- (void)setFrame:(NSRect)frameRect {
+  [super setFrame:frameRect];

[groby-ooo-7-16, 2014/04/23 21:57:58]

Does this actually work? It seems that [super setFrame:] will invoke
-resizeWithOldSuperviewSize, which invokes -setFrame: which cycles around. Only
the equality check in the default implementation saves you here :)

What layout behavior are you trying to achieve? A layout on size change? Why
then resize the super?

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Adding some printf suggests that resize is not called on setFrame.  It also
seems that setFrame calls setFrameSize, so I remove the override of setFrame.

I've updated a few things so that layoutSubviews is called exactly once from
init and once from setFrame/setFrameSize
This isn't resizing the parent view, it's the calling the parent class
implementation to do most of the work.

[groby-ooo-7-16, 2014/04/25 20:29:54]

Sigh. No idea why I confused super and superview :(

+  [self layoutSubViews];
+}
+
+- (void)setFrameSize:(NSSize)frameSize {
+  [super setFrameSize:frameSize];
+  [self layoutSubViews];
+}
+
+- (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize {

[groby-ooo-7-16, 2014/04/23 21:57:58]

why not just set the autoresizingMask to NSViewWidthSizeable - that ensures the
size will be changing exactly equal to the amount the superview changed.


No need to override this.

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

I first tried setting the resize mask to NSViewWidthSizeable, but it didn't work
the way I wanted.
Is there a better way to run custom layout on resize?

[groby-ooo-7-16, 2014/04/25 20:29:54]

Yes, see NSView's -setPostsFrameChangedNotifications:

[vandebo (ex-Chrome), 2014/04/28 19:04:51]

I tried registered for this notification, it does not trigger for all cases that
resizeWithOldSuperviewSize does.

[groby-ooo-7-16, 2014/04/28 22:17:12]

That's correct. -resizeWithOldSuperviewSize: triggers when the superview changes
size. _If_ that means [self view] changes size, the notification should trigger.
Do you have a concrete case we can look at?

[vandebo (ex-Chrome), 2014/04/29 18:40:29]

I've uploaded a full CL that adds the Notification to this one.  The print outs
show that the notification is fired once for the internal auto fitting, but not
either time the parent view is resized.
https://codereview.chromium.org/257233002

+  NSRect superviewFrame = [[self superview] frame];
+  NSRect frame = [self frame];
+  if (NSWidth(frame) != NSWidth(superviewFrame)) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

If you *must* implement this for a reason I'm missing, no need to test. Just set
the width and call -setFrameSize:

-setFrame: already has a test that the frame hasn't changed.

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+    frame.size.width = NSWidth(superviewFrame);
+    [self setFrame:frame];
+  }
+}
+
+- (void)setState:(bool)selected {
+  [checkbox_ setState:selected ? NSOnState : NSOffState];
+}
+
+- (void)onCheckboxToggled:(id)sender {
+  controller_->OnCheckboxToggled(prefId_, [sender state] == NSOnState);
+}
+
+- (void)onFolderViewerClicked:(id)sender {
+  controller_->OnFolderViewerClicked(prefId_);
+}
+
+- (ui::MenuModel*)getContextMenu {
+ return controller_->GetContextMenu(prefId_);
+}
+
+- (void)layoutSubViews {
+  NSRect frame = [self frame];

[groby-ooo-7-16, 2014/04/23 21:57:58]

Technically, layout for subviews needs to happen in [self bounds]

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+  [checkbox_ sizeToFit];
+  NSRect checkboxRect = [checkbox_ frame];
+  checkboxRect.size.height =

[groby-ooo-7-16, 2014/04/23 21:57:58]

Why exactly the MinIfNotZero dance? checkboxRect can't be zero after sizeToFit,
so I assume that's due to self having an empty frame? Do you really care about
layout in that case?

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Right, the case where this matter is self having an empty frame.  Feel free to
tell me the right way to do this...

Multiple instance of this class (MediaGalleryListEntry) are going into a
scrollview. The content width of the scroll view is dependent on total height of
the contents.  The way I've done this now is pass in an empty frame to the this
class and let it size itself, use the preferred sizes of all the instances to
compute the total height, compute the scrollview content width, and then size
all the list entries to the correct width.

[groby-ooo-7-16, 2014/04/25 20:29:54]

Does what we discussed not work? (empty frame just uses large bounds, then
shrinks at the end)
What's the contentWidth based on?

[vandebo (ex-Chrome), 2014/04/28 19:04:51]

The windows of the scrollview is fixed, but the width of the content inside the
scrollview can change depending on if there is a scrollbar or not.

[groby-ooo-7-16, 2014/04/28 22:17:12]

Ah. Still, that should work without MinIfZero, unless the approach we discussed
is somehow broken?

[vandebo (ex-Chrome), 2014/04/29 18:40:29]

Yup, MinIfNotZero is gone.

+      MinIfNotZero(NSHeight(checkboxRect), NSHeight(frame));
+  NSRect folderViewerRect = NSZeroRect;
+  if (folderViewer_.get()) {
+    [folderViewer_ sizeToFit];
+    folderViewerRect = [folderViewer_ frame];
+  } else {
+    folderViewerRect.size.width = -kCheckboxMargin;

[groby-ooo-7-16, 2014/04/23 21:57:58]

Having a negative size for non-existent elements _still_ makes me queasy :)

If you simply compute layout rects for all elements first (including position),
I think we could simplify that.

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

So layout all the elements at their natural size, positioning them correctly,
taking into account non-existent elements, then move elements around relatively
when resizing them?  So if I resize the checkbox, I have to change the position
of both of the elements to the right?  That seems more complicated/harder to
keep correct.  I've done it in patch set 7, let me know what you think.

I could put each element into an NSBox with an appropriate sized border and then
stack the the boxes right next to each other.... but I'd rather not.

[groby-ooo-7-16, 2014/04/25 20:29:54]

Will look there, but yes, I'd suggest just updating all element's positions with
the loop mentioned below.

[vandebo (ex-Chrome), 2014/04/28 19:04:51]

I've updated the code to lay everything out and then resize in place if needed.

+  }
+  folderViewerRect.size.height =

[groby-ooo-7-16, 2014/04/23 21:57:58]

Same here

+      MinIfNotZero(NSHeight(folderViewerRect), NSHeight(frame));
+  NSRect detailsRect = NSZeroRect;
+  if (details_.get()) {
+    [details_ sizeToFit];
+    detailsRect = [details_ frame];
+  } else {
+    detailsRect.size.width = -kCheckboxMargin;
+  }
+  detailsRect.size.height =
+      MinIfNotZero(NSHeight(detailsRect), NSHeight(frame));
+
+  // Size the views. If all the elements don't naturally fit, the checkbox
+  // should get squished and will elide in the middle.  However, it shouldn't
+  // squish too much so it gets at least half of the max width and the details
+  // text should elide as well in that case.
+  int naturalWidth = NSWidth(checkboxRect) + NSWidth(folderViewerRect) +

[groby-ooo-7-16, 2014/04/23 21:57:58]

If you computed all rects, this would simply be NSMaxX(detailsRect). (Possibly
+kCheckboxMargin)

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Done.

+      NSWidth(detailsRect) + 4 * kCheckboxMargin;
+  if (NSWidth(frame) && naturalWidth > NSWidth(frame)) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

Again, I think. Can you just early out if NSIsEmptyRect([self frame])?

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Only if I compute size and position for each element before this and then I
still need to deal with the size to fit case.

+    int maxContent = NSWidth(frame) - 4 * kCheckboxMargin;
+    if (NSWidth(folderViewerRect) + NSWidth(detailsRect) > maxContent / 2) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

This is not quite what the comment above says. Or gets there in a roundabout
way. I'd translate the comment as follows:

checkboxRect.size.width = std::max(maxContent / 2, checkboxRect.size.width);
detailsRect.size.width = maxContent - NSWidth(folderViewRect) -
NSWidth(checkboxRect)

I.e. checkbox always gets at least half, folder stays constant, details is
squished to accomodate the rest. Am I missing something?

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

I think you're missing the case where the checkbox is naturally less than half
the content width and the details + folder are bigger than half the content
width.

+      detailsRect.size.width = std::max(
+          maxContent / 2 - NSWidth(folderViewerRect),
+          maxContent - NSWidth(checkboxRect) - NSWidth(folderViewerRect));
+    }
+    checkboxRect.size.width =
+        maxContent - NSWidth(folderViewerRect) - NSWidth(detailsRect);
+  }
+
+  checkboxRect.origin = NSMakePoint(kCheckboxMargin, 0);
+  [checkbox_ setFrame:checkboxRect];
+
+  folderViewerRect.origin =
+      NSMakePoint(NSMaxX(checkboxRect) + kCheckboxMargin, 0);
+  if (folderViewer_.get()) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

What if you just reflowed the subviews? Set the frame sizes above, and then:

CGFloat xPos = kCheckboxMargin;
for(NSView* view in [self subviews]) {
  [view setFrameOrigin:NSMakePoint(xPos, 0)]
  xPos = NSMaxX([view frame]) + kCheckboxMargin;
}

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

If the folder viewer is not present, that will add two kCheckboxMargin's between
the the checkbox and the details.

[groby-ooo-7-16, 2014/04/25 20:29:54]

How so? If the folderView is not present, it won't be in the list of subviews?

[vandebo (ex-Chrome), 2014/04/28 19:04:51]

Oops, you're right...

+    [folderViewer_ setFrame:folderViewerRect];
+    detailsRect.origin =
+        NSMakePoint(NSMaxX(folderViewerRect) + kCheckboxMargin, 0);
+  } else {
+    detailsRect.origin = folderViewerRect.origin;
+  }
+
+  if (details_.get()) {
+    [details_ setFrame:detailsRect];
+  }
+
+  if (NSIsEmptyRect(frame)) {

[groby-ooo-7-16, 2014/04/23 21:57:58]

Oh. I see. Empty rect is essentially -sizeToFit?

Also, don't you want to take NSMaxX for the width? This will only size to the
widest element, not to all of them?

In that case,, I'd simply do this at the top:
NSRect bounds = [self bounds];
if (NSIsEmptyRect(bounds))
  bounds.size = NSMakeSize(1.0e6, 1.0e6);

and then at the bottom:
if (NSIsEmptyRect([self frame]) {
  NSRect frame = NSMakeRect(0, 0, 1, 1); // Or use kWindowSizeDeterminedLater
  for(NSView* view in [self subviews]) {
    frame = NSUnionRect(frame, [view frame]);
  }
  [self setFrameSize:frame.size];
}

That will save you MinIfNotZero and several other tests above.

[vandebo (ex-Chrome), 2014/04/25 20:10:56]

Yes.
Yes, bug.
Done + adding final margin.

+    frame.size.width =
+        std::max(NSWidth(checkboxRect),
+                 std::max(NSWidth(folderViewerRect), NSWidth(detailsRect)));
+    frame.size.height =
+        std::max(NSHeight(checkboxRect),
+                 std::max(NSHeight(folderViewerRect), NSHeight(detailsRect)));
+    [super setFrame:frame];
+  }
+}
+
+@end