Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
203223002:
Translate bubble for Mac OS X (Closed)
|
Created:
6 years, 9 months ago by hajimehoshi Modified:
6 years, 8 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionTranslate bubble for Mac OS X
Implements the new Bubble UX used instead of the current infobar. This UI is behind the flag --enable-translate-new-ux.
The screenshots are available at crbug.com/307352#22
BUG=307352
TBR=sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262097
Patch Set 1 #
Total comments: 22
Patch Set 2 : groby's review #
Total comments: 83
Patch Set 3 : groby's review #
Total comments: 22
Patch Set 4 : groby's review #
Total comments: 4
Patch Set 5 : groby's review #
Total comments: 2
Patch Set 6 : (rebasing) #Patch Set 7 : toyoshim's review #Patch Set 8 : (rebasing) #Patch Set 9 : (rebasing) #
Messages
Total messages: 25 (0 generated)
Almost done. The rest work is to implement the error view and write the unit test. PTAL
A couple of quick questions (sorry, I'm travelling right now) added as comments My main question though is why not just have separate NSView classes for the separate views. This might separate the code handling different views a little bit more clearly, I think. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:133: NSView* contentView = [[FlippedView alloc] initWithFrame:contentFrame]; Why not just replace contentView, if you need a FlippedView? Also, the amount of layout is trivial - is it possible to skip the FlippedView? https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:191: NSView* subview = [[[[self window] contentView] subviews] objectAtIndex:0]; It might be worth storing that object on the controller, if you don't replace contentView at least. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:192: [subview setSubviews:@[[self currentView]]]; nit - @[] has spaces after/before bracket. I.e. @[ [self currentView] ] https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:217: NSView* view = [[FlippedView alloc] initWithFrame:contentFrame]; In general, if you can avoid FlippedView, I would. It's not a huge advantage for layout. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:540: - (void)updateAdvancedView { I think it might be clearer to subclass NSView for the separate views. But maybe I miss something - is there a reason to keep everything in one class? https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:615: [button setFrame:frame]; see below for setFrameOrigin: https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:636: NSRect frame = NSMakeRect( I think the next 4 lines (until -setFrame:) might be cleaner as: [button setFrameOrigin:point]; https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:663: [button setFrame:frame]; setFrameOrigin:, as above https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:700: NSButton* button = base::mac::ObjCCastStrict<NSButton>(sender); Is there a contraint that requires a switch based on tag? I'd prefer a separate routine for each action. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:743: // button is pressed. Can you simply not have an action for the checkbox, then? https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:751: - (void)handlePopUpButtonSelectedItemChanged:(id)sender { As for the button delegate, I'd much prefer a separate delegate per button.
Thank you! https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:133: NSView* contentView = [[FlippedView alloc] initWithFrame:contentFrame]; On 2014/03/18 17:58:18, groby wrote: > Why not just replace contentView, if you need a FlippedView? Done (Removed). https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:191: NSView* subview = [[[[self window] contentView] subviews] objectAtIndex:0]; On 2014/03/18 17:58:18, groby wrote: > It might be worth storing that object on the controller, if you don't replace > contentView at least. Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:192: [subview setSubviews:@[[self currentView]]]; On 2014/03/18 17:58:18, groby wrote: > nit - @[] has spaces after/before bracket. I.e. @[ [self currentView] ] Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:217: NSView* view = [[FlippedView alloc] initWithFrame:contentFrame]; On 2014/03/18 17:58:18, groby wrote: > In general, if you can avoid FlippedView, I would. It's not a huge advantage for > layout. Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:540: - (void)updateAdvancedView { On 2014/03/18 17:58:18, groby wrote: > I think it might be clearer to subclass NSView for the separate views. But maybe > I miss something - is there a reason to keep everything in one class? Nothing special. I wasn't able to find such bubble classes as it switches multiple NSView classes. I guess Translate bubble is the first bubble which has multiple modes. I'll do that in another patch later. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:615: [button setFrame:frame]; On 2014/03/18 17:58:18, groby wrote: > see below for setFrameOrigin: Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:636: NSRect frame = NSMakeRect( On 2014/03/18 17:58:18, groby wrote: > I think the next 4 lines (until -setFrame:) might be cleaner as: > [button setFrameOrigin:point]; Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:663: [button setFrame:frame]; On 2014/03/18 17:58:18, groby wrote: > setFrameOrigin:, as above Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:700: NSButton* button = base::mac::ObjCCastStrict<NSButton>(sender); On 2014/03/18 17:58:18, groby wrote: > Is there a contraint that requires a switch based on tag? I'd prefer a separate > routine for each action. Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:743: // button is pressed. On 2014/03/18 17:58:18, groby wrote: > Can you simply not have an action for the checkbox, then? Done. https://codereview.chromium.org/203223002/diff/1/chrome/browser/ui/cocoa/tran... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:751: - (void)handlePopUpButtonSelectedItemChanged:(id)sender { On 2014/03/18 17:58:18, groby wrote: > As for the button delegate, I'd much prefer a separate delegate per button. > Done.
Sorry this took so long - travel interfered. I'm back at my desk, so I should be faster to respond. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/translate... File chrome/browser/translate/translate_manager.h (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/translate... chrome/browser/translate/translate_manager.h:58: // Sets the pair languages for translation to |source| and |target|. It doesn't really set the languages, or am I misunderstanding? Maybe "Gets |source| and |target| language for translation" as a comment instead? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:25: class TranslateDenialComboboxModel : public ui::ComboboxModel { This seems identical to the Views combobox model - can this be shared? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:77: const CGFloat kRelatedControlHorizontalSpacing = -2; Why is this negative? Intuitively, I would read that as "related controls overlap horizontally" - which probably means I read this wrong :) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:120: - (NSView*)currentView { I'm wondering if it would be nicer to simply have a ScopedPtrHashMap of ViewState->NSView. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:169: [[[self window] contentView] setFrame:contentViewFrame]; If you don't need to animate the window, you probably want [[self window] setContentSize:contentViewFrame.size]; This will automatically adjust the window's size, so you only need to comput windowFrame.origin, and call [[self window] setFrameOrigin:windowFrameOrigin]; If you DO want to animate, you want to compute the window's frame size via -frameRectForContentRect: - this takes potential window decorations into accoumt. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:177: animate:[[self window] isVisible]]; I assume you want the window to just pop up if it wasn't visible yet, no animation? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:181: CGFloat contentWidth = kWindowWidth - 2 * kFramePadding; That seems to always be the contentWidth - maybe just add CGFloat kContentWidth = kWindowWidth - 2 * kFramePadding; to the constants? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:186: 1); It's OK to use 0 for the height - only NSWindow doesn't like zero width or height. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: // Adjust width for the first item. A shorter version would be to just copy the popup, remove all but the first item, sizeToFit, and copy over the size. I.e.: scoped_nsobject <NSPopUpButton> helper([denyPopUpButton copy]); [helper removeAllItems]; [helper addItemWithTitle:[[denyPopUpButton itemAtIndex:0] title]]; [helper sizeToFit]; [denyPopUpButton setFrameSize:[helper frame].size]; (I can't believe we don't have a helper function that does that... Odd!) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:227: for (int i = 1; i < [denyPopUpButton numberOfItems]; i++) { If the above solution works, you don't need this, but as general info, it's easier to iterate over Objective C arrays this way: for (NSMenuItem* item in [denyPopupButton itemArray]) { ... https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:246: titleRectForBounds:[denyPopUpButton bounds]].origin.y; Usually, we don't base layout computations on cell size, since the size of control decorations could conceivable change across SDKs. Can you change the computation to base this on the button only? Or maybe I don't quite understand the computation - where is the button supposed to be? It seems "almost at the bottom, with a small offset"? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:252: yPos += NSHeight([translateButton frame]); You can roll these two into one line :) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( Are you sure you don't want any horizontal spacing between the label and the button? And tiny nit: In this case it's identical, but you probably want NSMaxX, not NSWidth. It is a little bit cleaner in case the label is ever not at origin.x == 0./ https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:259: yPos += NSHeight([textLabel frame]); It's probably shorter to skip everything until the return, and replace with: [view setFrameSize:NSMakeSize(contentWidth, NSMaxY([textLabel frame]); https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:303: yPos += NSHeight([textLabel frame]); it seems most of your controls are only vertically stacked, with fixed spacing - if you want to, maybe it might be worth the effort to change l10n_util::VerticallyReflowGroup to take a spacing parameter. (No need to, though. And even if you want to do it, I'd suggest a follow up CL) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:305: contentFrame = [view frame]; See comment about setFrameSize: above. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:313: CGFloat contentWidth = kWindowWidth - 2 * kFramePadding; As above, maybe have a file-wide kContentWidth. In general, many of the suggestions from createViewBeforeTranslate can probably be applied to all the createViewXYZ functions. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:461: CGFloat diffY = [[sourcePopUpButton cell] As above, I'm not sure why you need the cell's titleRect to place items. Would you mind explaining the layout? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:468: frame.origin.x = textLabelWidth; Please use frame.origin = NSMakePoint(textLabelWidth, yPos) instead of individual member assignments. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:480: frame.origin.x = textLabelWidth; NSMakePoint, please. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:482: frame.size.width = (kWindowWidth - 2 * kFramePadding) - textLabelWidth; You can probably just use NSWidth([targetPopUpButton frame]) - you want them to be the same size, correct? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:497: if (alwaysTranslateCheckbox_) { No need to if-check - you can send messages to nil, that's perfectly OK. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:512: [advancedDoneButton_ setFrame:frame]; setFrameOrigin, please, since you don't change size. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:517: [advancedCancelButton_ setFrame:frame]; setFrameOrigin, please. And NSWidth(frame) instead of frame.size.width - if possible, do not refer to struct members of NSRect by name. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:523: withSize:(CGFloat)fontSize You seem to call this always with [NSFont smallSystemFontSize] - can you get rid of that parameter? Same thing for bold: - it seems to be always NO https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:529: [[NSTextField alloc] initWithFrame:frame]); Since you call -sizeToFit at the end anyways, you could just use -initWithFrame:NSZeroRect https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:542: textField]; Since you call sizeToFit later, do you need to do the frame size adjustment here? Also, if you do need this adjustment and remove the later sizeToFit, no need to do the -setFrame: call. -sizeToFitFixedWidthTextField: already adjusts the frame for |textField|. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:553: target:(id)target Since |target| is always self, can you remove that parameter? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:561: [button setCell:cell.get()]; No need to set the cell - you can just do [HyperLinkButtonCell buttonWithString:text] to get a button with the proper cell, and an already set title. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:566: if (target != nil && action != nil) { You can just set target and action - it's OK for them to be nil. (A nil target means it tries to find a responder along the first responder chain, a nil action just means nothing happens, independent of target) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:577: target:(id)target See above https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:587: if (target != nil && action != nil) { See above https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:621: if (target != nil && action != nil) { See above https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:677: switch ([button indexOfSelectedItem]) { You can directly set a target/action on the MenuItem, that would avoid the switch here. If you don't want to do that, I recommend not switching on the index, but instead storing kIndexXYZ as a tag on the item. (I.e [item setTag:kIndexXYZ] when you create, and switch ([[button selectedItem] tag]) here.
Thank you for the review! It's a pity that your travel was interfered. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/translate... File chrome/browser/translate/translate_manager.h (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/translate... chrome/browser/translate/translate_manager.h:58: // Sets the pair languages for translation to |source| and |target|. On 2014/03/21 01:21:42, groby wrote: > It doesn't really set the languages, or am I misunderstanding? Maybe "Gets > |source| and |target| language for translation" as a comment instead? Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:25: class TranslateDenialComboboxModel : public ui::ComboboxModel { On 2014/03/21 01:21:42, groby wrote: > This seems identical to the Views combobox model - can this be shared? They are a little different in that the first item is dummy on Mac. They are similar anyway, hmm, let me unify them later. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:77: const CGFloat kRelatedControlHorizontalSpacing = -2; On 2014/03/21 01:21:42, groby wrote: > Why is this negative? Intuitively, I would read that as "related controls > overlap horizontally" - which probably means I read this wrong :) This is actually negative. I put two buttons side by side on Xcode and adjust along with the automatic horizontal space constraint. I found that (the right button's X) - (the left button's X) - (the left button's width) was minus 2. Maybe buttons were overlapping. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:120: - (NSView*)currentView { On 2014/03/21 01:21:42, groby wrote: > I'm wondering if it would be nicer to simply have a ScopedPtrHashMap of > ViewState->NSView. It seems that ScopedPtrHashMap can't be used for NSView bacause NSView is not C++ class. I could use std::map, but the member variable *View_ would remain and the ownerships would remain on them. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:169: [[[self window] contentView] setFrame:contentViewFrame]; On 2014/03/21 01:21:42, groby wrote: > If you don't need to animate the window, you probably want [[self window] > setContentSize:contentViewFrame.size]; > > This will automatically adjust the window's size, so you only need to comput > windowFrame.origin, and call [[self window] setFrameOrigin:windowFrameOrigin]; > > If you DO want to animate, you want to compute the window's frame size via > -frameRectForContentRect: - this takes potential window decorations into > accoumt. Done (animation is needed, so I tried to use frameRectForContentRect.) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:177: animate:[[self window] isVisible]]; On 2014/03/21 01:21:42, groby wrote: > I assume you want the window to just pop up if it wasn't visible yet, no > animation? Right. It needs animation when changing the view. (ex: clicking the 'options' link to show the advanced view.) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:181: CGFloat contentWidth = kWindowWidth - 2 * kFramePadding; On 2014/03/21 01:21:42, groby wrote: > That seems to always be the contentWidth - maybe just add > CGFloat kContentWidth = kWindowWidth - 2 * kFramePadding; > > to the constants? Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:186: 1); On 2014/03/21 01:21:42, groby wrote: > It's OK to use 0 for the height - only NSWindow doesn't like zero width or > height. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: // Adjust width for the first item. On 2014/03/21 01:21:42, groby wrote: > A shorter version would be to just copy the popup, remove all but the first > item, sizeToFit, and copy over the size. > > I.e.: > scoped_nsobject <NSPopUpButton> helper([denyPopUpButton copy]); > [helper removeAllItems]; > [helper addItemWithTitle:[[denyPopUpButton itemAtIndex:0] title]]; > [helper sizeToFit]; > [denyPopUpButton setFrameSize:[helper frame].size]; > > (I can't believe we don't have a helper function that does that... Odd!) Oh, cool! However, it seems that NSButton is not copyable... https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:227: for (int i = 1; i < [denyPopUpButton numberOfItems]; i++) { On 2014/03/21 01:21:42, groby wrote: > If the above solution works, you don't need this, but as general info, it's > easier to iterate over Objective C arrays this way: > > for (NSMenuItem* item in [denyPopupButton itemArray]) { > ... Done leaving the above as it is. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:246: titleRectForBounds:[denyPopUpButton bounds]].origin.y; On 2014/03/21 01:21:42, groby wrote: > Usually, we don't base layout computations on cell size, since the size of > control decorations could conceivable change across SDKs. Can you change the > computation to base this on the button only? > > Or maybe I don't quite understand the computation - where is the button supposed > to be? It seems "almost at the bottom, with a small offset"? Since NSPopUpButton and NSButtond didn't align vertically, I needed to adjust Y of the NSPopUpButton. It looks like that NSPopUp is put a little below than normal NSButtons. I referred some similar cases: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:252: yPos += NSHeight([translateButton frame]); On 2014/03/21 01:21:42, groby wrote: > You can roll these two into one line :) Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( On 2014/03/21 01:21:42, groby wrote: > Are you sure you don't want any horizontal spacing between the label and the > button? Yes because the link seems like a text and I think it is natural to put texts side by side without space. > And tiny nit: In this case it's identical, but you probably want NSMaxX, not > NSWidth. It is a little bit cleaner in case the label is ever not at origin.x == > 0./ Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:259: yPos += NSHeight([textLabel frame]); On 2014/03/21 01:21:42, groby wrote: > It's probably shorter to skip everything until the return, and replace with: > > [view setFrameSize:NSMakeSize(contentWidth, NSMaxY([textLabel frame]); Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:303: yPos += NSHeight([textLabel frame]); On 2014/03/21 01:21:42, groby wrote: > it seems most of your controls are only vertically stacked, with fixed spacing - > if you want to, maybe it might be worth the effort to change > l10n_util::VerticallyReflowGroup to take a spacing parameter. > > (No need to, though. And even if you want to do it, I'd suggest a follow up CL) Thanks. I'll do this if I have time. I think other tasks like adding tests have higher priority. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:305: contentFrame = [view frame]; On 2014/03/21 01:21:42, groby wrote: > See comment about setFrameSize: above. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:313: CGFloat contentWidth = kWindowWidth - 2 * kFramePadding; On 2014/03/21 01:21:42, groby wrote: > As above, maybe have a file-wide kContentWidth. In general, many of the > suggestions from createViewBeforeTranslate can probably be applied to all the > createViewXYZ functions. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:461: CGFloat diffY = [[sourcePopUpButton cell] On 2014/03/21 01:21:42, groby wrote: > As above, I'm not sure why you need the cell's titleRect to place items. Would > you mind explaining the layout? Same reason as above. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:468: frame.origin.x = textLabelWidth; On 2014/03/21 01:21:42, groby wrote: > Please use frame.origin = NSMakePoint(textLabelWidth, yPos) instead of > individual member assignments. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:480: frame.origin.x = textLabelWidth; On 2014/03/21 01:21:42, groby wrote: > NSMakePoint, please. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:482: frame.size.width = (kWindowWidth - 2 * kFramePadding) - textLabelWidth; On 2014/03/21 01:21:42, groby wrote: > You can probably just use NSWidth([targetPopUpButton frame]) - you want them to > be the same size, correct? Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:497: if (alwaysTranslateCheckbox_) { On 2014/03/21 01:21:42, groby wrote: > No need to if-check - you can send messages to nil, that's perfectly OK. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:512: [advancedDoneButton_ setFrame:frame]; On 2014/03/21 01:21:42, groby wrote: > setFrameOrigin, please, since you don't change size. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:517: [advancedCancelButton_ setFrame:frame]; On 2014/03/21 01:21:42, groby wrote: > setFrameOrigin, please. And NSWidth(frame) instead of frame.size.width - if > possible, do not refer to struct members of NSRect by name. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:523: withSize:(CGFloat)fontSize On 2014/03/21 01:21:42, groby wrote: > You seem to call this always with [NSFont smallSystemFontSize] - can you get rid > of that parameter? > > Same thing for bold: - it seems to be always NO Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:529: [[NSTextField alloc] initWithFrame:frame]); On 2014/03/21 01:21:42, groby wrote: > Since you call -sizeToFit at the end anyways, you could just use > -initWithFrame:NSZeroRect Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:542: textField]; On 2014/03/21 01:21:42, groby wrote: > Since you call sizeToFit later, do you need to do the frame size adjustment > here? > > Also, if you do need this adjustment and remove the later sizeToFit, no need to > do the -setFrame: call. -sizeToFitFixedWidthTextField: already adjusts the frame > for |textField|. Oh, calling GTM.. wasn't needed... Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:553: target:(id)target On 2014/03/21 01:21:42, groby wrote: > Since |target| is always self, can you remove that parameter? Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:561: [button setCell:cell.get()]; On 2014/03/21 01:21:42, groby wrote: > No need to set the cell - you can just do [HyperLinkButtonCell > buttonWithString:text] to get a button with the proper cell, and an already set > title. Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:566: if (target != nil && action != nil) { On 2014/03/21 01:21:42, groby wrote: > You can just set target and action - it's OK for them to be nil. (A nil target > means it tries to find a responder along the first responder chain, a nil action > just means nothing happens, independent of target) Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:577: target:(id)target On 2014/03/21 01:21:42, groby wrote: > See above Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:587: if (target != nil && action != nil) { On 2014/03/21 01:21:42, groby wrote: > See above Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:621: if (target != nil && action != nil) { On 2014/03/21 01:21:42, groby wrote: > See above Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:677: switch ([button indexOfSelectedItem]) { On 2014/03/21 01:21:42, groby wrote: > You can directly set a target/action on the MenuItem, that would avoid the > switch here. > > If you don't want to do that, I recommend not switching on the index, but > instead storing kIndexXYZ as a tag on the item. (I.e [item setTag:kIndexXYZ] > when you create, and switch ([[button selectedItem] tag]) here. Done.
> It's a pity that your travel was interfered. Oh, I assumed that you were on vacation. Sorry if I misunderstood.
This is almost there - and sorry for splitting the comments across two patch sets. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:25: class TranslateDenialComboboxModel : public ui::ComboboxModel { On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > This seems identical to the Views combobox model - can this be shared? > > They are a little different in that the first item is dummy on Mac. They are > similar anyway, hmm, let me unify them later. True, OSX needs a dummy item - but that should be handled when converting from ComboboxModel to NSMenu. Shared models are always good. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:77: const CGFloat kRelatedControlHorizontalSpacing = -2; On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > Why is this negative? Intuitively, I would read that as "related controls > > overlap horizontally" - which probably means I read this wrong :) > > This is actually negative. I put two buttons side by side on Xcode and adjust > along with the automatic horizontal space constraint. I found that (the right > button's X) - (the left button's X) - (the left button's width) was minus 2. > Maybe buttons were overlapping. That's odd. Looking at the mocks, I see no two buttons that are directly side-by-side, though? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:120: - (NSView*)currentView { On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > I'm wondering if it would be nicer to simply have a ScopedPtrHashMap of > > ViewState->NSView. > > It seems that ScopedPtrHashMap can't be used for NSView bacause NSView is not > C++ class. I could use std::map, but the member variable *View_ would remain and > the ownerships would remain on them. You are of course right, sorry. I should have said NSDictionary - I have no idea what I was thinking about when I suggested otherwise. The dictionary then has ownership of all the views, and the switch becomes a simple valueForKey. (I'm not a fan of large switch statements) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: // Adjust width for the first item. On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > A shorter version would be to just copy the popup, remove all but the first > > item, sizeToFit, and copy over the size. > > > > I.e.: > > scoped_nsobject <NSPopUpButton> helper([denyPopUpButton copy]); > > [helper removeAllItems]; > > [helper addItemWithTitle:[[denyPopUpButton itemAtIndex:0] title]]; > > [helper sizeToFit]; > > [denyPopUpButton setFrameSize:[helper frame].size]; > > > > (I can't believe we don't have a helper function that does that... Odd!) > > Oh, cool! However, it seems that NSButton is not copyable... Sigh. I forgot, sorry. The cell is copyable - but that might not be enough since you change control attributes. Well, in this case, how about: scoped_nsobject<NSMenu> originalMenu([[denyPopUpButton menu] copy]); [denyPopUpButton removeAllItems]; [denyPopUpButton addItemWithTitle:[[originalMenu itemAtIndex:0] title]]; [denyPopUpButton sizeToFit]; [denyPopUpButton setMenu:menu]; NSMenu is copyable, so this should work. (I'd really like to avoid all the for loops, extracting the titles, etc.) https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:246: titleRectForBounds:[denyPopUpButton bounds]].origin.y; On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > Usually, we don't base layout computations on cell size, since the size of > > control decorations could conceivable change across SDKs. Can you change the > > computation to base this on the button only? > > > > Or maybe I don't quite understand the computation - where is the button > supposed > > to be? It seems "almost at the bottom, with a small offset"? > > Since NSPopUpButton and NSButtond didn't align vertically, I needed to adjust Y > of the NSPopUpButton. It looks like that NSPopUp is put a little below than > normal NSButtons. I referred some similar cases: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Ah, you're adjusting the baseline of the texts to match? Thank you for the explanation - that does make sense. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > Are you sure you don't want any horizontal spacing between the label and the > > button? > > Yes because the link seems like a text and I think it is natural to put texts > side by side without space. > > > And tiny nit: In this case it's identical, but you probably want NSMaxX, not > > NSWidth. It is a little bit cleaner in case the label is ever not at origin.x > == > > 0./ > > Done. I'm OK with this, but out of curiosity: From the mocks, it seems there's a space in between. But I assume they're screenshots you made using your code? https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:303: yPos += NSHeight([textLabel frame]); On 2014/03/24 07:16:30, hajimehoshi wrote: > On 2014/03/21 01:21:42, groby wrote: > > it seems most of your controls are only vertically stacked, with fixed spacing > - > > if you want to, maybe it might be worth the effort to change > > l10n_util::VerticallyReflowGroup to take a spacing parameter. > > > > (No need to, though. And even if you want to do it, I'd suggest a follow up > CL) > > Thanks. I'll do this if I have time. I think other tasks like adding tests have > higher priority. That is fine. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:38: NSButton* advancedDoneButton_; Please add comment who owns those buttons. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: #include "third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h" You don't need that any more. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:33: // NSMenuItem's setTitle. Having an entry twice in a popup menu is counter to the typical OSX user experience. Why is that necessary? https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:172: - (NSView*)createViewBeforeTranslate { Please declare private methods in a private interface, too. Also, this returns an NSView with a retainCount of 1 - methods that do that should always start with alloc/new/copy/mutableCopy. "newBeforeTranslateView" might be the best choice. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:200: new TranslateDenialComboboxModel(originalLanguageName)); Note for later CL's: When you factor out the TranslateDenialComboboxModel to share with Views, you might want to use a ui::MenuModel instead - this can handle all the callback mechanics, too. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: if (index != 0) { No need for this check (or for tracking index). It's less code to collect all titles and set them empty, then just [[button itemAtIndex:0] setTitle:titles.front()] [button sizeToFit] ... https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:263: - (NSView*)createViewTranslating { See above re: name https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:448: frame.size.width = NSWidth([targetPopUpButton frame]); Are you sure you don't want target & source width to be max(targetWidth, sourceWidth)? Theoretically, target's width could be too small to accomodate all entries in source? https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:561: for (int i = 0; i < model->GetItemCount(); ++i) This works, but doesn't account for separators. Are you sure none of the comboboxes will have separators? https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:572: - (void)handleTranslateButtonPressed { I'd think most of the handleXYZ callbacks could live on a cross-platform controller and be shared with Views. Is that planned?
Thank you! https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:25: class TranslateDenialComboboxModel : public ui::ComboboxModel { On 2014/03/25 18:49:04, groby wrote: > On 2014/03/24 07:16:30, hajimehoshi wrote: > > On 2014/03/21 01:21:42, groby wrote: > > > This seems identical to the Views combobox model - can this be shared? > > > > They are a little different in that the first item is dummy on Mac. They are > > similar anyway, hmm, let me unify them later. > > True, OSX needs a dummy item - but that should be handled when converting from > ComboboxModel to NSMenu. Shared models are always good. Sure, but I'll leave it until your question about the UX of the popup button is resolved. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:77: const CGFloat kRelatedControlHorizontalSpacing = -2; On 2014/03/25 18:49:04, groby wrote: > On 2014/03/24 07:16:30, hajimehoshi wrote: > > On 2014/03/21 01:21:42, groby wrote: > > > Why is this negative? Intuitively, I would read that as "related controls > > > overlap horizontally" - which probably means I read this wrong :) > > > > This is actually negative. I put two buttons side by side on Xcode and adjust > > along with the automatic horizontal space constraint. I found that (the right > > button's X) - (the left button's X) - (the left button's width) was minus 2. > > Maybe buttons were overlapping. > > That's odd. Looking at the mocks, I see no two buttons that are directly > side-by-side, though? > > Yes, there is space between two buttons, while the number of this constant is negative. I wonder if the logical width and the actual width differ... https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:120: - (NSView*)currentView { On 2014/03/25 18:49:04, groby wrote: > On 2014/03/24 07:16:30, hajimehoshi wrote: > > On 2014/03/21 01:21:42, groby wrote: > > > I'm wondering if it would be nicer to simply have a ScopedPtrHashMap of > > > ViewState->NSView. > > > > It seems that ScopedPtrHashMap can't be used for NSView bacause NSView is not > > C++ class. I could use std::map, but the member variable *View_ would remain > and > > the ownerships would remain on them. > > You are of course right, sorry. I should have said NSDictionary - I have no idea > what I was thinking about when I suggested otherwise. > > The dictionary then has ownership of all the views, and the switch becomes a > simple valueForKey. (I'm not a fan of large switch statements) Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: // Adjust width for the first item. On 2014/03/25 18:49:04, groby wrote: > On 2014/03/24 07:16:30, hajimehoshi wrote: > > On 2014/03/21 01:21:42, groby wrote: > > > A shorter version would be to just copy the popup, remove all but the first > > > item, sizeToFit, and copy over the size. > > > > > > I.e.: > > > scoped_nsobject <NSPopUpButton> helper([denyPopUpButton copy]); > > > [helper removeAllItems]; > > > [helper addItemWithTitle:[[denyPopUpButton itemAtIndex:0] title]]; > > > [helper sizeToFit]; > > > [denyPopUpButton setFrameSize:[helper frame].size]; > > > > > > (I can't believe we don't have a helper function that does that... Odd!) > > > > Oh, cool! However, it seems that NSButton is not copyable... > > > Sigh. I forgot, sorry. The cell is copyable - but that might not be enough since > you change control attributes. > > Well, in this case, how about: > > scoped_nsobject<NSMenu> originalMenu([[denyPopUpButton menu] copy]); > [denyPopUpButton removeAllItems]; > [denyPopUpButton addItemWithTitle:[[originalMenu itemAtIndex:0] title]]; > [denyPopUpButton sizeToFit]; > [denyPopUpButton setMenu:menu]; > > NSMenu is copyable, so this should work. (I'd really like to avoid all the for > loops, extracting the titles, etc.) Done. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( On 2014/03/25 18:49:04, groby wrote: > On 2014/03/24 07:16:30, hajimehoshi wrote: > > On 2014/03/21 01:21:42, groby wrote: > > > Are you sure you don't want any horizontal spacing between the label and the > > > button? > > > > Yes because the link seems like a text and I think it is natural to put texts > > side by side without space. > > > > > And tiny nit: In this case it's identical, but you probably want NSMaxX, not > > > NSWidth. It is a little bit cleaner in case the label is ever not at > origin.x > > == > > > 0./ > > > > Done. > > I'm OK with this, but out of curiosity: From the mocks, it seems there's a space > in between. But I assume they're screenshots you made using your code? Your assuming is right. I don't put any space, but there seems space between the texts. I'm not familiar with the coordinate system on Mac OS X, but maybe the same thing is happaning to the buttons we're discussing about above. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:38: NSButton* advancedDoneButton_; On 2014/03/25 18:49:05, groby wrote: > Please add comment who owns those buttons. Done. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: #include "third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h" On 2014/03/25 18:49:05, groby wrote: > You don't need that any more. Done. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:33: // NSMenuItem's setTitle. On 2014/03/25 18:49:05, groby wrote: > Having an entry twice in a popup menu is counter to the typical OSX user > experience. Why is that necessary? The context is that we wanted the 'fancy' button (crbug/270052) for users to select actions. In Aura, I implemented a new controller for this. On Mac OS X, it seemed impossible to create the same controller retaining the natural UX, so we decided to use popup button controller instead. The image is at crbug/270052#42. I didn't know that was counter to the common Mac UX. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:172: - (NSView*)createViewBeforeTranslate { On 2014/03/25 18:49:05, groby wrote: > Please declare private methods in a private interface, too. > > Also, this returns an NSView with a retainCount of 1 - methods that do that > should always start with alloc/new/copy/mutableCopy. "newBeforeTranslateView" > might be the best choice. Done. As you mentioned before, should I split this file into some classes? https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:200: new TranslateDenialComboboxModel(originalLanguageName)); On 2014/03/25 18:49:05, groby wrote: > Note for later CL's: When you factor out the TranslateDenialComboboxModel to > share with Views, you might want to use a ui::MenuModel instead - this can > handle all the callback mechanics, too. Thanks. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:225: if (index != 0) { On 2014/03/25 18:49:05, groby wrote: > No need for this check (or for tracking index). It's less code to collect all > titles and set them empty, then just > > [[button itemAtIndex:0] setTitle:titles.front()] > [button sizeToFit] > ... Thanks, but I used your suggestion of copying NSCell. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:263: - (NSView*)createViewTranslating { On 2014/03/25 18:49:05, groby wrote: > See above re: name Done. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:448: frame.size.width = NSWidth([targetPopUpButton frame]); On 2014/03/25 18:49:05, groby wrote: > Are you sure you don't want target & source width to be max(targetWidth, > sourceWidth)? Theoretically, target's width could be too small to accomodate all > entries in source? The sets of source and target languages are same, and the widths of the comboboxes are fixed along with the combobox. I think a combobox whose width is fixed is not a problem because such like combobox is already used at the bookmark bubble's folder selector. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:561: for (int i = 0; i < model->GetItemCount(); ++i) On 2014/03/25 18:49:05, groby wrote: > This works, but doesn't account for separators. Are you sure none of the > comboboxes will have separators? Correct. I'll fix this. Sorry but I have to leave now, so I'll do it in another patch. https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:572: - (void)handleTranslateButtonPressed { On 2014/03/25 18:49:05, groby wrote: > I'd think most of the handleXYZ callbacks could live on a cross-platform > controller and be shared with Views. Is that planned? Not planned, but it seems not easy. Let me think about this.
LGTM - sorry this review took so long! https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:77: const CGFloat kRelatedControlHorizontalSpacing = -2; On 2014/03/26 11:06:05, hajimehoshi wrote: > On 2014/03/25 18:49:04, groby wrote: > > On 2014/03/24 07:16:30, hajimehoshi wrote: > > > On 2014/03/21 01:21:42, groby wrote: > > > > Why is this negative? Intuitively, I would read that as "related controls > > > > overlap horizontally" - which probably means I read this wrong :) > > > > > > This is actually negative. I put two buttons side by side on Xcode and > adjust > > > along with the automatic horizontal space constraint. I found that (the > right > > > button's X) - (the left button's X) - (the left button's width) was minus 2. > > > Maybe buttons were overlapping. > > > > That's odd. Looking at the mocks, I see no two buttons that are directly > > side-by-side, though? > > > > > > Yes, there is space between two buttons, while the number of this constant is > negative. I wonder if the logical width and the actual width differ... They sort-of do. The frame indicates "most" of the area for the button. The focus ring is (IIRC) drawn outside the frame. But if you place them just using NSWidth, you shouldn't need a constant. This is weird, but if this fixes your layout, leave it in. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( On 2014/03/26 11:06:05, hajimehoshi wrote: > On 2014/03/25 18:49:04, groby wrote: > > On 2014/03/24 07:16:30, hajimehoshi wrote: > > > On 2014/03/21 01:21:42, groby wrote: > > > > Are you sure you don't want any horizontal spacing between the label and > the > > > > button? > > > > > > Yes because the link seems like a text and I think it is natural to put > texts > > > side by side without space. > > > > > > > And tiny nit: In this case it's identical, but you probably want NSMaxX, > not > > > > NSWidth. It is a little bit cleaner in case the label is ever not at > > origin.x > > > == > > > > 0./ > > > > > > Done. > > > > I'm OK with this, but out of curiosity: From the mocks, it seems there's a > space > > in between. But I assume they're screenshots you made using your code? > > Your assuming is right. I don't put any space, but there seems space between the > texts. I'm not familiar with the coordinate system on Mac OS X, but maybe the > same thing is happaning to the buttons we're discussing about above. This is strange, but that doesn't impact the CL :) As a side note: If you are ever curious about what frames are used and where they are - are you familiar with F-Script? It's an excellent tool for looking where the actual frames are. (http://www.chromium.org/developers/f-script-anywhere). https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:33: // NSMenuItem's setTitle. On 2014/03/26 11:06:05, hajimehoshi wrote: > On 2014/03/25 18:49:05, groby wrote: > > Having an entry twice in a popup menu is counter to the typical OSX user > > experience. Why is that necessary? > > The context is that we wanted the 'fancy' button (crbug/270052) for users to > select actions. In Aura, I implemented a new controller for this. On Mac OS X, > it seemed impossible to create the same controller retaining the natural UX, so > we decided to use popup button controller instead. The image is at > crbug/270052#42. I didn't know that was counter to the common Mac UX. Ah, I understand. Thank you for the explanation, and for pointing me towards the bug. Since the item doesn't show twice in the menu, I think this is OK. (In the long run, maybe we can generalize the SplitButton Susanna Leng is working on - it's used in the context of Permission bubbles, which is pretty close to the translate bubble. UX will probably want the same buttons. Not for this CL, though) https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:147: toNSNumber(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE): nit: To translate from enum to NSNumber, you can used boxed literals - simply wrap the enum in @() Example: @{ @(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE) : [self newBeforeTranslateView],... https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:168: NSView* view = [views_ objectForKey:key]; I think this can be boxed too - @( model_->GetViewState() )
Thank you! I've not added tests for this, so I'm hesitate to commit this. I'd like to add tests to this CL soon. https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:256: [advancedLinkButton setFrameOrigin:NSMakePoint( On 2014/03/27 00:39:39, groby wrote: > On 2014/03/26 11:06:05, hajimehoshi wrote: > > On 2014/03/25 18:49:04, groby wrote: > > > On 2014/03/24 07:16:30, hajimehoshi wrote: > > > > On 2014/03/21 01:21:42, groby wrote: > > > > > Are you sure you don't want any horizontal spacing between the label and > > the > > > > > button? > > > > > > > > Yes because the link seems like a text and I think it is natural to put > > texts > > > > side by side without space. > > > > > > > > > And tiny nit: In this case it's identical, but you probably want NSMaxX, > > not > > > > > NSWidth. It is a little bit cleaner in case the label is ever not at > > > origin.x > > > > == > > > > > 0./ > > > > > > > > Done. > > > > > > I'm OK with this, but out of curiosity: From the mocks, it seems there's a > > space > > > in between. But I assume they're screenshots you made using your code? > > > > Your assuming is right. I don't put any space, but there seems space between > the > > texts. I'm not familiar with the coordinate system on Mac OS X, but maybe the > > same thing is happaning to the buttons we're discussing about above. > > This is strange, but that doesn't impact the CL :) > > As a side note: If you are ever curious about what frames are used and where > they are - are you familiar with F-Script? It's an excellent tool for looking > where the actual frames are. > (http://www.chromium.org/developers/f-script-anywhere). I didn't know that. Thank you for the information! https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:561: for (int i = 0; i < model->GetItemCount(); ++i) On 2014/03/26 11:06:05, hajimehoshi wrote: > On 2014/03/25 18:49:05, groby wrote: > > This works, but doesn't account for separators. Are you sure none of the > > comboboxes will have separators? > > Correct. I'll fix this. Sorry but I have to leave now, so I'll do it in another > patch. Done. https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:147: toNSNumber(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE): On 2014/03/27 00:39:39, groby wrote: > nit: To translate from enum to NSNumber, you can used boxed literals - simply > wrap the enum in @() > > Example: > @{ @(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE) : [self > newBeforeTranslateView],... Done. https://codereview.chromium.org/203223002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:168: NSView* view = [views_ objectForKey:key]; On 2014/03/27 00:39:39, groby wrote: > I think this can be boxed too - @( model_->GetViewState() ) Done.
I'm sorry if you are waiting for my review. LGTM on translate/ once my one concern is resolved. https://codereview.chromium.org/203223002/diff/120001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/203223002/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:79: TranslateTabHelper* translate_tab_helper = FromWebContents(web_contents); Did you confirm that there is no race and translate_tab_helper never becomes NULL on all platforms?
Thanks! No worries, I wasn't waiting for you. It's just that I was busy with other things... https://codereview.chromium.org/203223002/diff/120001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/203223002/diff/120001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:79: TranslateTabHelper* translate_tab_helper = FromWebContents(web_contents); On 2014/04/02 03:57:30, Takashi Toyoshima (chromium) wrote: > Did you confirm that there is no race Yes as long as this is called on UI thread. > and translate_tab_helper never becomes > NULL on all platforms? It could be NULL when testing. Done.
On second thought, I'll commit this before it becomes hard to rebase this.
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/203223002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/203223002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/04/07 05:34:17, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Added sky@ as TBR because the change in chrome/browser/ui/views is trivial.
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/203223002/200001
Message was sent while issue was closed.
Change committed as 262097 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
