diff options
| -rw-r--r-- | chrome/browser/cocoa/bookmark_bar_controller.h | 7 | ||||
| -rw-r--r-- | chrome/browser/cocoa/bookmark_bar_controller.mm | 89 | ||||
| -rw-r--r-- | chrome/browser/cocoa/bookmark_bar_controller_unittest.mm | 158 |
3 files changed, 212 insertions, 42 deletions
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h index 1e98b93..662da61 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller.h +++ b/chrome/browser/cocoa/bookmark_bar_controller.h @@ -40,6 +40,9 @@ class PrefService; BOOL contentViewHasOffset_; BOOL barShouldBeShown_; + // Our bookmark buttons, ordered from L-->R. + scoped_nsobject<NSMutableArray> buttons_; + // If the bar is disabled, we hide it and ignore show/hide commands. // Set when using fullscreen mode. BOOL barIsEnabled_; @@ -115,6 +118,10 @@ class PrefService; @interface BookmarkBarController(TestingAPI) // Set the delegate for a unit test. - (void)setDelegate:(id<BookmarkURLOpener>)delegate; +- (void)clearBookmarkBar; +- (NSArray*)buttons; +- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell xOffset:(int*)xOffset; +- (void)checkForBookmarkButtonGrowth:(NSButton*)button; @end #endif // CHROME_BROWSER_COCOA_BOOKMARK_BAR_CONTROLLER_H_ diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm index 7d1ea55..d4af000 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller.mm +++ b/chrome/browser/cocoa/bookmark_bar_controller.mm @@ -36,7 +36,7 @@ const int kBookmarkBarWebframeHeightAdjustment = 25; // Magic numbers from Cole const CGFloat kDefaultBookmarkWidth = 150.0; const CGFloat kBookmarkVerticalPadding = 2.0; -const CGFloat kBookmarkHorizontalPadding = 8.0; +const CGFloat kBookmarkHorizontalPadding = 1.0; }; @implementation BookmarkBarController @@ -53,6 +53,7 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; parentView_ = parentView; webContentView_ = webContentView; infoBarsView_ = infoBarsView; + buttons_.reset([[NSMutableArray alloc] init]); delegate_ = delegate; } return self; @@ -237,16 +238,15 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; node->GetParent()->IndexOfChild(node)); } -// Delete all items from the bookmark bar. TODO(jrg): once the -// bookmark bar has other subviews (e.g. "off the side" button/menu, -// "Other Bookmarks"), etc, this routine will need revisiting. +// Delete all bookmarks from the bookmark bar. - (void)clearBookmarkBar { - [[self view] setSubviews:[NSArray array]]; + [buttons_ makeObjectsPerformSelector:@selector(removeFromSuperview)]; + [buttons_ removeAllObjects]; } // Return an autoreleased NSCell suitable for a bookmark button. // TODO(jrg): move much of the cell config into the BookmarkButtonCell class. -- (NSCell *)cellForBookmarkNode:(const BookmarkNode*)node frame:(NSRect)frame { +- (NSCell*)cellForBookmarkNode:(const BookmarkNode*)node { NSString* title = base::SysWideToNSString(node->GetTitle()); NSButtonCell *cell = [[[BookmarkButtonCell alloc] initTextCell:nil] autorelease]; @@ -268,22 +268,59 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; return cell; } -// TODO(jrg): accomodation for bookmarks less than minimum width in -// size (like Windows)? -- (NSRect)frameForBookmarkAtIndex:(int)index { +// Return an appropriate width for the given bookmark button cell. +// The "+1" is needed because, sometimes, Cocoa is off by one. +// Example: for a bookmark named "Moma" or "SFGate", it is one pixel +// too small. For a bookmark named "SFGateFooWoo", it is just fine. +- (CGFloat)widthForBookmarkButtonCell:(NSCell*)cell { + CGFloat desired = [cell cellSize].width + 1; + return std::min(desired, kDefaultBookmarkWidth); +} + +// Returns a frame appropriate for the given bookmark cell, suitable +// for creating an NSButton that will contain it. |xOffset| is the X +// offset for the frame; it is increased to be an appropriate X offset +// for the next button. +- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell + xOffset:(int*)xOffset { NSRect bounds = [[self view] bounds]; // TODO: be smarter about this; the animator delays the right height if (bounds.size.height == 0) bounds.size.height = kBookmarkBarHeight; - NSRect frame = NSInsetRect(bounds, kBookmarkHorizontalPadding, kBookmarkVerticalPadding); - frame.origin.x += (kDefaultBookmarkWidth * index); - frame.size.width = kDefaultBookmarkWidth; + frame.size.width = [self widthForBookmarkButtonCell:cell]; + + // Add an X offset based on what we've already done + frame.origin.x += *xOffset; + + // And up the X offset for next time. + *xOffset = NSMaxX(frame); + return frame; } +// A bookmark button's contents changed. Check for growth +// (e.g. increase the width up to the maximum). If we grew, move +// other bookmark buttons over. +- (void)checkForBookmarkButtonGrowth:(NSButton*)button { + NSRect frame = [button frame]; + CGFloat desiredSize = [self widthForBookmarkButtonCell:[button cell]]; + CGFloat delta = desiredSize - frame.size.width; + if (delta) { + frame.size.width = desiredSize; + [button setFrame:frame]; + for (NSButton* each_button in buttons_.get()) { + NSRect each_frame = [each_button frame]; + if (each_frame.origin.x > frame.origin.x) { + each_frame.origin.x += delta; + [each_button setFrame:each_frame]; + } + } + } +} + // Add all items from the given model to our bookmark bar. // TODO(jrg): lots of things! // - bookmark folders (e.g. menu from the button) @@ -291,26 +328,27 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; // screen // - ... // -// TODO(jrg): contextual menu (e.g. Open In New Tab) for each button -// in this function) -// // TODO(jrg): write a "build bar" so there is a nice spot for things // like the contextual menu which is invoked when not over a // bookmark. On Safari that menu has a "new folder" option. - (void)addNodesToBar:(const BookmarkNode*)node { + int x_offset = 0; for (int i = 0; i < node->GetChildCount(); i++) { const BookmarkNode* child = node->GetChild(i); - NSRect frame = [self frameForBookmarkAtIndex:i]; + NSCell* cell = [self cellForBookmarkNode:child]; + NSRect frame = [self frameForBookmarkButtonFromCell:cell xOffset:&x_offset]; NSButton* button = [[[NSButton alloc] initWithFrame:frame] autorelease]; DCHECK(button); + [buttons_ addObject:button]; + // [NSButton setCell:] warns to NOT use setCell: other than in the // initializer of a control. However, we are using a basic // NSButton whose initializer does not take an NSCell as an // object. To honor the assumed semantics, we do nothing with // NSButton between alloc/init and setCell:. - [button setCell:[self cellForBookmarkNode:child frame:frame]]; + [button setCell:cell]; if (child->is_folder()) { // For now just disable the button if it's a folder. @@ -375,10 +413,15 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; // TODO(jrg): linear searching is bad. // Need a BookmarkNode-->NSCell mapping. +// +// TODO(jrg): if the bookmark bar is open on launch, we see the +// buttons all placed, then "scooted over" as the favicons load. If +// this looks bad I may need to change widthForBookmarkButtonCell to +// add space for an image even if not there on the assumption that +// favicons will eventually load. - (void)nodeFavIconLoaded:(BookmarkModel*)model node:(const BookmarkNode*)node { - NSArray* views = [[self view] subviews]; - for (NSButton* button in views) { + for (NSButton* button in buttons_.get()) { NSButtonCell* cell = [button cell]; void* pointer = [[cell representedObject] pointerValue]; const BookmarkNode* cellnode = static_cast<const BookmarkNode*>(pointer); @@ -387,6 +430,10 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; if (image) { [cell setImage:image]; [cell setImagePosition:NSImageLeft]; + // Adding an image means we might need more room for the + // bookmark. Test for it by growing the button (if needed) + // and shifting everything else over. + [self checkForBookmarkButtonGrowth:button]; } return; } @@ -403,4 +450,8 @@ const CGFloat kBookmarkHorizontalPadding = 8.0; delegate_ = delegate; } +- (NSArray*)buttons { + return buttons_.get(); +} + @end diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm index f08978e..e1dbdbc 100644 --- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm +++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm @@ -27,6 +27,29 @@ @end +// NSCell that is pre-provided with a desired size that becomes the +// return value for -(NSSize)cellSize:. +@interface CellWithDesiredSize : NSCell { + @private + NSSize cellSize_; +} +@property(readonly) NSSize cellSize; +@end + +@implementation CellWithDesiredSize + +@synthesize cellSize = cellSize_; + +- (id)initTextCell:(NSString*)string desiredSize:(NSSize)size { + if ((self = [super initTextCell:string])) { + cellSize_ = size; + } + return self; +} + +@end + + namespace { static const int kContentAreaHeight = 500; @@ -51,6 +74,15 @@ class BookmarkBarControllerTest : public testing::Test { infoBarsView:infobar_view_.get() delegate:nil]); [bar_ view]; // force loading of the nib + + // Awkwardness to look like we've been installed. + [parent_view_ addSubview:[bar_ view]]; + NSRect frame = [[[bar_ view] superview] frame]; + frame.origin.y = 100; + [[[bar_ view] superview] setFrame:frame]; + + // make sure it's open so certain things aren't no-ops + [bar_ toggleBookmarkBar]; } CocoaTestHelper cocoa_helper_; // Inits Cocoa, creates window, etc... @@ -62,16 +94,15 @@ class BookmarkBarControllerTest : public testing::Test { }; TEST_F(BookmarkBarControllerTest, ShowHide) { - // Assume hidden by default in a new profile. + // The test class opens the bar by default since many actions are + // no-ops with it closed. Set back to closed as a baseline. + if ([bar_ isBookmarkBarVisible]) + [bar_ toggleBookmarkBar]; + + // Start hidden. EXPECT_FALSE([bar_ isBookmarkBarVisible]); EXPECT_TRUE([[bar_ view] isHidden]); - // Awkwardness to look like we've been installed. - [parent_view_ addSubview:[bar_ view]]; - NSRect frame = [[[bar_ view] superview] frame]; - frame.origin.y = 100; - [[[bar_ view] superview] setFrame:frame]; - // Show and hide it by toggling. [bar_ toggleBookmarkBar]; EXPECT_TRUE([bar_ isBookmarkBarVisible]); @@ -151,31 +182,112 @@ TEST_F(BookmarkBarControllerTest, OpenBookmarkFromMenus) { } } +TEST_F(BookmarkBarControllerTest, TestAddRemoveAndClear) { + BookmarkModel* model = helper_.profile()->GetBookmarkModel(); -// TODO(jrg): Make sure showing the bookmark bar calls loaded: (to -// process bookmarks) -TEST_F(BookmarkBarControllerTest, ShowAndLoad) { + EXPECT_EQ(0U, [[bar_ buttons] count]); + unsigned int initial_subview_count = [[[bar_ view] subviews] count]; + + // Make sure a redundant call doesn't choke + [bar_ clearBookmarkBar]; + EXPECT_EQ(0U, [[bar_ buttons] count]); + EXPECT_EQ(initial_subview_count, [[[bar_ view] subviews] count]); + + GURL gurl1("http://superfriends.hall-of-justice.edu"); + std::wstring title1(L"Protectors of the Universe"); + model->SetURLStarred(gurl1, title1, true); + EXPECT_EQ(1U, [[bar_ buttons] count]); + EXPECT_EQ(1+initial_subview_count, [[[bar_ view] subviews] count]); + + GURL gurl2("http://legion-of-doom.gov"); + std::wstring title2(L"Bad doodz"); + model->SetURLStarred(gurl2, title2, true); + EXPECT_EQ(2U, [[bar_ buttons] count]); + EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]); + + for (int i = 0; i < 3; i++) { + // is_starred=false --> remove the bookmark + model->SetURLStarred(gurl2, title2, false); + EXPECT_EQ(1U, [[bar_ buttons] count]); + EXPECT_EQ(1+initial_subview_count, [[[bar_ view] subviews] count]); + + // and bring it back + model->SetURLStarred(gurl2, title2, true); + EXPECT_EQ(2U, [[bar_ buttons] count]); + EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]); + } + + [bar_ clearBookmarkBar]; + EXPECT_EQ(0U, [[bar_ buttons] count]); + EXPECT_EQ(initial_subview_count, [[[bar_ view] subviews] count]); + + // Explicit test of loaded: since this is a convenient spot + [bar_ loaded:model]; + EXPECT_EQ(2U, [[bar_ buttons] count]); + EXPECT_EQ(2+initial_subview_count, [[[bar_ view] subviews] count]); } -// TODO(jrg): Make sure adding 1 bookmark adds 1 subview, and removing -// 1 removes 1 subview. (We can't test for a simple Clear since there -// will soon be views in here which aren't bookmarks.) -TEST_F(BookmarkBarControllerTest, ViewChanges) { +// Make sure that each button we add marches to the right and does not +// overlap with the previous one. +TEST_F(BookmarkBarControllerTest, TestButtonMarch) { + scoped_nsobject<NSMutableArray> cells([[NSMutableArray alloc] init]); + + CGFloat widths[] = { 10, 10, 100, 10, 500, 500, 80000, 60000, 1, 345 }; + for (unsigned int i = 0; i < arraysize(widths); i++) { + NSCell* cell = [[CellWithDesiredSize alloc] + initTextCell:@"foo" + desiredSize:NSMakeSize(widths[i], 30)]; + [cells addObject:cell]; + [cell release]; + } + + int x_offset = 0; + CGFloat x_end = x_offset; // end of the previous button + for (unsigned int i = 0; i < arraysize(widths); i++) { + NSRect r = [bar_ frameForBookmarkButtonFromCell:[cells objectAtIndex:i] + xOffset:&x_offset]; + EXPECT_GE(r.origin.x, x_end); + x_end = NSMaxX(r); + } } -// TODO(jrg): Make sure loaded: does something useful -TEST_F(BookmarkBarControllerTest, Loaded) { - // Clear; make sure no views - // Call loaded: - // Make sure subviews +TEST_F(BookmarkBarControllerTest, CheckForGrowth) { + BookmarkModel* model = helper_.profile()->GetBookmarkModel(); + GURL gurl1("http://www.google.com"); + std::wstring title1(L"x"); + model->SetURLStarred(gurl1, title1, true); + + GURL gurl2("http://www.google.com/blah"); + std::wstring title2(L"y"); + model->SetURLStarred(gurl2, title2, true); + + EXPECT_EQ(2U, [[bar_ buttons] count]); + CGFloat width_1 = [[[bar_ buttons] objectAtIndex:0] frame].size.width; + CGFloat x_2 = [[[bar_ buttons] objectAtIndex:1] frame].origin.x; + + NSButton* first = [[bar_ buttons] objectAtIndex:0]; + [[first cell] setTitle:@"This is a really big title; watch out mom!"]; + [bar_ checkForBookmarkButtonGrowth:first]; + + // Make sure the 1st button is now wider, the 2nd one is moved over, + // and they don't overlap. + NSRect frame_1 = [[[bar_ buttons] objectAtIndex:0] frame]; + NSRect frame_2 = [[[bar_ buttons] objectAtIndex:1] frame]; + EXPECT_GT(frame_1.size.width, width_1); + EXPECT_GT(frame_2.origin.x, x_2); + EXPECT_GE(frame_2.origin.x, frame_1.origin.x + frame_1.size.width); } -// TODO(jrg): Test cellForBookmarkNode: -TEST_F(BookmarkBarControllerTest, Cell) { +// TODO(jrg): write a test to confirm that nodeFavIconLoaded calls +// checkForBookmarkButtonGrowth:. + +// TODO(jrg): Make sure showing the bookmark bar calls loaded: (to +// process bookmarks) +TEST_F(BookmarkBarControllerTest, ShowAndLoad) { } -// TODO(jrg): Test frameForBookmarkAtIndex -TEST_F(BookmarkBarControllerTest, FrameAtIndex) { +// TODO(jrg): Test cellForBookmarkNode: +TEST_F(BookmarkBarControllerTest, Cell) { } TEST_F(BookmarkBarControllerTest, Contents) { |