Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
231873003:
Refactor media galleries dialogs on cocoa to extract common code (Closed)
|
Created:
6 years, 8 months ago by vandebo (ex-Chrome) Modified:
6 years, 7 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor media galleries dialogs on cocoa to extract common code
BUG=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267021
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Checkpoint #Patch Set 4 : gallery entry controller to C++ #Patch Set 5 : nit #
Total comments: 55
Patch Set 6 : Address comments #Patch Set 7 : position all views up front #Patch Set 8 : Update test #Patch Set 9 : Layout in place #
Total comments: 17
Patch Set 10 : Address comments #Messages
Total messages: 10 (0 generated)
Awesome for factoring this out! I'm not entirely clear on the resize & layout flow, so a bunch of questions there. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:159: NSRect main_frame = [main_container_ frame]; [main_container_ setFrameOrigin:NSZeroPoint]; https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:185: prefId:permission.gallery_id I'd suggest just initializing it with the permission, and teasing it apart in init - avoids endless parameter lists :) https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:191: [checkbox_entry setState:permission.allowed]; ... which'd also allow to roll the setState: into init. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:195: [checkbox_entry setFrame:entry_rect]; setFrameOrigin: does just set the origin. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm:20: namespace { const implies internal linkage - no need for anon namespace. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm:139: NSRect main_frame = [main_container_ frame]; setFrameOrigin: https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:18: CGFloat MinIfNotZero(CGFloat a, CGFloat b) { std::min(std::max(a, 0.0), std::max(b, 0.0))) Equality comparison to 0.0 never ends well :) https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:28: const CGFloat kDetailGray = 0.625; Please keep consts together (and outside anon ns) https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:90: // Set a auto resize mask so that resizeWithOldSuperviewSize() is called. nit: -resizeWithOldSuperviewSize: https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:135: [details_ setTextColor:[NSColor colorWithCalibratedRed:kDetailGray +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.) https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:153: [super setFrame:frameRect]; 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? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:165: if (NSWidth(frame) != NSWidth(superviewFrame)) { 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:188: NSRect frame = [self frame]; Technically, layout for subviews needs to happen in [self bounds] https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = 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? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: folderViewerRect.size.width = -kCheckboxMargin; 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:200: folderViewerRect.size.height = Same here https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:216: int naturalWidth = NSWidth(checkboxRect) + NSWidth(folderViewerRect) + If you computed all rects, this would simply be NSMaxX(detailsRect). (Possibly +kCheckboxMargin) https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:218: if (NSWidth(frame) && naturalWidth > NSWidth(frame)) { Again, I think. Can you just early out if NSIsEmptyRect([self frame])? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:220: if (NSWidth(folderViewerRect) + NSWidth(detailsRect) > maxContent / 2) { 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? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:234: if (folderViewer_.get()) { 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; } https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:246: if (NSIsEmptyRect(frame)) { 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.
https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:159: NSRect main_frame = [main_container_ frame]; On 2014/04/23 21:57:58, groby wrote: > [main_container_ setFrameOrigin:NSZeroPoint]; Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:185: prefId:permission.gallery_id On 2014/04/23 21:57:58, groby wrote: > I'd suggest just initializing it with the permission, and teasing it apart in > init - avoids endless parameter lists :) For the two call sites, there are two of these parameters that are handled differently, prefId and showFolderViewer, so I could pass in the pref_info in addition to those two things, which saves two parameters. The reason I haven't already done that is that pref_info has a prefId field (matches in one instance, but not in the other). For that reason my preference would be to not pass in pref_info. I could move some of the parameters to setters, but it would increase the code length a bit in order to properly handle all combinations. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:195: [checkbox_entry setFrame:entry_rect]; On 2014/04/23 21:57:58, groby wrote: > setFrameOrigin: does just set the origin. Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm:20: namespace { On 2014/04/23 21:57:58, groby wrote: > const implies internal linkage - no need for anon namespace. Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm:139: NSRect main_frame = [main_container_ frame]; On 2014/04/23 21:57:58, groby wrote: > setFrameOrigin: Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:18: CGFloat MinIfNotZero(CGFloat a, CGFloat b) { On 2014/04/23 21:57:58, groby wrote: > std::min(std::max(a, 0.0), std::max(b, 0.0))) a & b are both >= 0, so that reduces to std::min(a, b) > Equality comparison to 0.0 never ends well :) Removed... https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:28: const CGFloat kDetailGray = 0.625; On 2014/04/23 21:57:58, groby wrote: > Please keep consts together (and outside anon ns) Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:90: // Set a auto resize mask so that resizeWithOldSuperviewSize() is called. On 2014/04/23 21:57:58, groby wrote: > nit: -resizeWithOldSuperviewSize: Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:135: [details_ setTextColor:[NSColor colorWithCalibratedRed:kDetailGray On 2014/04/23 21:57:58, groby wrote: > +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.) Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:153: [super setFrame:frameRect]; On 2014/04/23 21:57:58, groby wrote: > 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 :) 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 > What layout behavior are you trying to achieve? A layout on size change? Why > then resize the super? This isn't resizing the parent view, it's the calling the parent class implementation to do most of the work. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { On 2014/04/23 21:57:58, groby wrote: > why not just set the autoresizingMask to NSViewWidthSizeable - that ensures the > size will be changing exactly equal to the amount the superview changed. I first tried setting the resize mask to NSViewWidthSizeable, but it didn't work the way I wanted. > No need to override this. Is there a better way to run custom layout on resize? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:165: if (NSWidth(frame) != NSWidth(superviewFrame)) { On 2014/04/23 21:57:58, groby wrote: > 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. Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:188: NSRect frame = [self frame]; On 2014/04/23 21:57:58, groby wrote: > Technically, layout for subviews needs to happen in [self bounds] Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = On 2014/04/23 21:57:58, groby wrote: > 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? 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: folderViewerRect.size.width = -kCheckboxMargin; On 2014/04/23 21:57:58, groby wrote: > 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. 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:216: int naturalWidth = NSWidth(checkboxRect) + NSWidth(folderViewerRect) + On 2014/04/23 21:57:58, groby wrote: > If you computed all rects, this would simply be NSMaxX(detailsRect). (Possibly > +kCheckboxMargin) Done. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:218: if (NSWidth(frame) && naturalWidth > NSWidth(frame)) { On 2014/04/23 21:57:58, groby wrote: > Again, I think. Can you just early out if NSIsEmptyRect([self frame])? 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:220: if (NSWidth(folderViewerRect) + NSWidth(detailsRect) > maxContent / 2) { On 2014/04/23 21:57:58, groby wrote: > 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? 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:234: if (folderViewer_.get()) { On 2014/04/23 21:57:58, groby wrote: > 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; > } If the folder viewer is not present, that will add two kCheckboxMargin's between the the checkbox and the details. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:246: if (NSIsEmptyRect(frame)) { On 2014/04/23 21:57:58, groby wrote: > Oh. I see. Empty rect is essentially -sizeToFit? Yes. > Also, don't you want to take NSMaxX for the width? This will only size to the > widest element, not to all of them? Yes, bug. > 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. Done + adding final margin.
https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:153: [super setFrame:frameRect]; On 2014/04/25 20:10:56, vandebo wrote: > On 2014/04/23 21:57:58, groby wrote: > > 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 :) > > 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 > > > What layout behavior are you trying to achieve? A layout on size change? Why > > then resize the super? > > This isn't resizing the parent view, it's the calling the parent class > implementation to do most of the work. Sigh. No idea why I confused super and superview :( https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { On 2014/04/25 20:10:56, vandebo wrote: > On 2014/04/23 21:57:58, groby wrote: > > why not just set the autoresizingMask to NSViewWidthSizeable - that ensures > the > > size will be changing exactly equal to the amount the superview changed. > > I first tried setting the resize mask to NSViewWidthSizeable, but it didn't work > the way I wanted. > > > No need to override this. > > Is there a better way to run custom layout on resize? Yes, see NSView's -setPostsFrameChangedNotifications: https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = On 2014/04/25 20:10:56, vandebo wrote: > On 2014/04/23 21:57:58, groby wrote: > > 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? > > Right, the case where this matter is self having an empty frame. Feel free to > tell me the right way to do this... Does what we discussed not work? (empty frame just uses large bounds, then shrinks at the end) > > 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. What's the contentWidth based on? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: folderViewerRect.size.width = -kCheckboxMargin; On 2014/04/25 20:10:56, vandebo wrote: > On 2014/04/23 21:57:58, groby wrote: > > 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. > > 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. Will look there, but yes, I'd suggest just updating all element's positions with the loop mentioned below. > > 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:234: if (folderViewer_.get()) { On 2014/04/25 20:10:56, vandebo wrote: > On 2014/04/23 21:57:58, groby wrote: > > 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; > > } > > If the folder viewer is not present, that will add two kCheckboxMargin's between > the the checkbox and the details. How so? If the folderView is not present, it won't be in the list of subviews?
https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { On 2014/04/25 20:29:54, groby wrote: > On 2014/04/25 20:10:56, vandebo wrote: > > On 2014/04/23 21:57:58, groby wrote: > > > why not just set the autoresizingMask to NSViewWidthSizeable - that ensures > > the > > > size will be changing exactly equal to the amount the superview changed. > > > > I first tried setting the resize mask to NSViewWidthSizeable, but it didn't > work > > the way I wanted. > > > > > No need to override this. > > > > Is there a better way to run custom layout on resize? > > Yes, see NSView's -setPostsFrameChangedNotifications: I tried registered for this notification, it does not trigger for all cases that resizeWithOldSuperviewSize does. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = On 2014/04/25 20:29:54, groby wrote: > On 2014/04/25 20:10:56, vandebo wrote: > > On 2014/04/23 21:57:58, groby wrote: > > > 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? > > > > Right, the case where this matter is self having an empty frame. Feel free to > > tell me the right way to do this... > > Does what we discussed not work? (empty frame just uses large bounds, then > shrinks at the end) > > > > 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. > > What's the contentWidth based on? 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. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: folderViewerRect.size.width = -kCheckboxMargin; On 2014/04/25 20:29:54, groby wrote: > On 2014/04/25 20:10:56, vandebo wrote: > > On 2014/04/23 21:57:58, groby wrote: > > > 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. > > > > 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. > > Will look there, but yes, I'd suggest just updating all element's positions with > the loop mentioned below. > > > > 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. > I've updated the code to lay everything out and then resize in place if needed. https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:234: if (folderViewer_.get()) { On 2014/04/25 20:29:54, groby wrote: > On 2014/04/25 20:10:56, vandebo wrote: > > On 2014/04/23 21:57:58, groby wrote: > > > 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; > > > } > > > > If the folder viewer is not present, that will add two kCheckboxMargin's > between > > the the checkbox and the details. > > How so? If the folderView is not present, it won't be in the list of subviews? Oops, you're right...
LGTM % nits https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { On 2014/04/28 19:04:51, vandebo wrote: > On 2014/04/25 20:29:54, groby wrote: > > On 2014/04/25 20:10:56, vandebo wrote: > > > On 2014/04/23 21:57:58, groby wrote: > > > > why not just set the autoresizingMask to NSViewWidthSizeable - that > ensures > > > the > > > > size will be changing exactly equal to the amount the superview changed. > > > > > > I first tried setting the resize mask to NSViewWidthSizeable, but it didn't > > work > > > the way I wanted. > > > > > > > No need to override this. > > > > > > Is there a better way to run custom layout on resize? > > > > Yes, see NSView's -setPostsFrameChangedNotifications: > > I tried registered for this notification, it does not trigger for all cases that > resizeWithOldSuperviewSize does. 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? https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = On 2014/04/28 19:04:51, vandebo wrote: > On 2014/04/25 20:29:54, groby wrote: > > On 2014/04/25 20:10:56, vandebo wrote: > > > On 2014/04/23 21:57:58, groby wrote: > > > > 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? > > > > > > Right, the case where this matter is self having an empty frame. Feel free > to > > > tell me the right way to do this... > > > > Does what we discussed not work? (empty frame just uses large bounds, then > > shrinks at the end) > > > > > > 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. > > > > What's the contentWidth based on? > > 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. > Ah. Still, that should work without MinIfZero, unless the approach we discussed is somehow broken? https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:157: [main_container_ setFrameOrigin:NSZeroPoint]; Not sure you need that - it defaults to NSZeroPoint. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_galleries_scan_result_dialog_cocoa.mm:114: CGFloat height = CreateCheckboxes(); Not suggesting you change this, just as an FYI: In general cocoa code separates between creating a view and adding it somewhere. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:125: if (showFolderViewer) Personal nit: You might want to roll this into the corresponding if() blocks above - saves two ifs. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:170: if (folderViewer_.get()) Skip the if - it's fine to send messages to nil :) https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:180: if (NSHeight(bounds) < NSHeight(viewFrame)) viewFrame.size.height = std::min(NSHeight(bounds), NSHeight(viewFrame)); https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:186: NSRect folderViewerFrame = NSZeroRect; Just folderViewFrame = [folderView_ frame]; Messages to nil return nil. If the return value is a known ABI struct (and NSRect is), it will return a zeroed out struct. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: CGFloat maxContent = NSWidth(bounds) - 4 * kCheckboxMargin; the 4* is probably not correct. ([[self subviews] count] + 1) * kCheckboxMargin? https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:201: detailsFrame.size.width = std::max( Caveat: Technically, you can have non-existing details but exceed the maxContent/2 limit. Is that an actual concern, or can that be ignored? https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:212: if (folderViewer_.get()) { You can probably save a lot of this logic if you just reflow again - the for-view-in-views loop at the top.
https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:162: - (void)resizeWithOldSuperviewSize:(NSSize)oldBoundsSize { On 2014/04/28 22:17:12, groby wrote: > On 2014/04/28 19:04:51, vandebo wrote: > > On 2014/04/25 20:29:54, groby wrote: > > > On 2014/04/25 20:10:56, vandebo wrote: > > > > On 2014/04/23 21:57:58, groby wrote: > > > > > why not just set the autoresizingMask to NSViewWidthSizeable - that > > ensures > > > > the > > > > > size will be changing exactly equal to the amount the superview changed. > > > > > > > > I first tried setting the resize mask to NSViewWidthSizeable, but it > didn't > > > work > > > > the way I wanted. > > > > > > > > > No need to override this. > > > > > > > > Is there a better way to run custom layout on resize? > > > > > > Yes, see NSView's -setPostsFrameChangedNotifications: > > > > I tried registered for this notification, it does not trigger for all cases > that > > resizeWithOldSuperviewSize does. > > 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? 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 https://codereview.chromium.org/231873003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:191: checkboxRect.size.height = On 2014/04/28 22:17:12, groby wrote: > On 2014/04/28 19:04:51, vandebo wrote: > > On 2014/04/25 20:29:54, groby wrote: > > > On 2014/04/25 20:10:56, vandebo wrote: > > > > On 2014/04/23 21:57:58, groby wrote: > > > > > 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? > > > > > > > > Right, the case where this matter is self having an empty frame. Feel > free > > to > > > > tell me the right way to do this... > > > > > > Does what we discussed not work? (empty frame just uses large bounds, then > > > shrinks at the end) > > > > > > > > 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. > > > > > > What's the contentWidth based on? > > > > 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. > > > Ah. Still, that should work without MinIfZero, unless the approach we discussed > is somehow broken? Yup, MinIfNotZero is gone. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right): https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:157: [main_container_ setFrameOrigin:NSZeroPoint]; On 2014/04/28 22:17:12, groby wrote: > Not sure you need that - it defaults to NSZeroPoint. I do - setFrameFromContentFrame adjusts origin so that the content origin is at 0,0. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm (right): https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:125: if (showFolderViewer) On 2014/04/28 22:17:12, groby wrote: > Personal nit: You might want to roll this into the corresponding if() blocks > above - saves two ifs. Done. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:170: if (folderViewer_.get()) On 2014/04/28 22:17:12, groby wrote: > Skip the if - it's fine to send messages to nil :) Done. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:180: if (NSHeight(bounds) < NSHeight(viewFrame)) On 2014/04/28 22:17:12, groby wrote: > viewFrame.size.height = std::min(NSHeight(bounds), NSHeight(viewFrame)); Done. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:186: NSRect folderViewerFrame = NSZeroRect; On 2014/04/28 22:17:12, groby wrote: > Just folderViewFrame = [folderView_ frame]; > > Messages to nil return nil. If the return value is a known ABI struct (and > NSRect is), it will return a zeroed out struct. Done. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:198: CGFloat maxContent = NSWidth(bounds) - 4 * kCheckboxMargin; On 2014/04/28 22:17:12, groby wrote: > the 4* is probably not correct. ([[self subviews] count] + 1) * kCheckboxMargin? Fixed up differently. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:201: detailsFrame.size.width = std::max( On 2014/04/28 22:17:12, groby wrote: > Caveat: Technically, you can have non-existing details but exceed the > maxContent/2 limit. Is that an actual concern, or can that be ignored? I haven't coded for that contingency/not a concern. The folder viewer icon is 16 pixels wide. https://codereview.chromium.org/231873003/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/media_gallery_list_entry_view.mm:212: if (folderViewer_.get()) { On 2014/04/28 22:17:12, groby wrote: > You can probably save a lot of this logic if you just reflow again - the > for-view-in-views loop at the top. I did try that code here. It was the same number of lines, and I thought this code made it clearer why something needed to happen. Another element would certainly swing things the other way though.
The CQ bit was checked by vandebo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/231873003/180001
Message was sent while issue was closed.
Change committed as 267021 |
