Closed
Bug 261134
Opened 21 years ago
Closed 21 years ago
Add an overflow menu when there are more open tabs than will fit on screen
Categories
(Camino Graveyard :: Tabbed Browsing, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino0.9
People
(Reporter: me, Assigned: mikepinkerton)
References
Details
Attachments
(2 files, 2 obsolete files)
16.07 KB,
image/tiff
|
Details | |
14.65 KB,
patch
|
moz
:
review+
jaas
:
superreview+
|
Details | Diff | Splinter Review |
With the new tabs, it is possible to open more tabs than can fit in a window, or
resize a window with many tabs such that some of them become inaccessible. A
menu (similar to the one on our toolbar) is needed to allow access to these tabs.
Reporter | ||
Comment 1•21 years ago
|
||
This patch introduces an overflow menu when there are more tabs than fit
onscreen. It also removes the arbitrary limit on the number of tabs permitted,
since >16 tabs no longer seems to degrade performance with the new tab widget
and they will be easy to access with this menu. Additionally, it increases the
minimum tab width drawn to 100px, which makes them much more usable and
addresses some perceived performance issues with the mouseover animation in the
tab bar when there are many tabs.
For this patch to work, it is also necessary to add tab_overflow.tif to
resources/images/application/chrome and add it to the XCode project.
Reporter | ||
Comment 2•21 years ago
|
||
This goes in resources/images/chrome and needs to be added to XCode.
Reporter | ||
Updated•21 years ago
|
Attachment #159822 -
Flags: review?(qa-mozilla)
Reporter | ||
Updated•21 years ago
|
Attachment #159822 -
Flags: review?(mozilla)
Reporter | ||
Comment 3•21 years ago
|
||
This version fixes a memory leak found in review by mozilla@derailer.org, a
semi-related memory leak in the new tab code found while fixing that, and a
problem where the tracking rects were not being updated on window resize until
a new tab was added or one was closed
Attachment #159822 -
Attachment is obsolete: true
Reporter | ||
Updated•21 years ago
|
Attachment #159822 -
Flags: review?(qa-mozilla)
Attachment #159822 -
Flags: review?(mozilla)
Reporter | ||
Updated•21 years ago
|
Attachment #159936 -
Flags: review?(mozilla)
Reporter | ||
Comment 4•21 years ago
|
||
Comment on attachment 159936 [details] [diff] [review]
version 2
moving review requests to the new version of the patch
Attachment #159936 -
Flags: review?(qa-mozilla)
Comment 5•21 years ago
|
||
Comment on attachment 159936 [details] [diff] [review]
version 2
>? .DS_Store
>Index: browser/BrowserTabBarView.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.h,v
>retrieving revision 1.1
>diff -u -4 -r1.1 BrowserTabBarView.h
>--- browser/BrowserTabBarView.h 2 Sep 2004 22:52:28 -0000 1.1
>+++ browser/BrowserTabBarView.h 24 Sep 2004 03:28:25 -0000
>@@ -46,22 +46,28 @@
> // this tab view should be tabless and borderless
> IBOutlet BrowserTabView *mTabView;
>
> TabButtonCell *mActiveTabButton; // active tab button, mainly useful for handling drags (STRONG)
>+ NSButton *mOverflowButton; // button for overflow menu if we've got more tabs than space (STRONG)
>+ NSMenu *mOverflowMenu; // menu for tab overflow (STRONG);
>
> // drag tracking
> NSPoint mLastClickPoint;
> BOOL mDragStarted;
> NSView *mDragDest;
> TabButtonCell *mDragDestButton;
>+
>+ BOOL mOverflowTabs;
>+ NSMutableArray *mTrackingCells; // cells which currently have tracking rects in this view
> }
>-
> // destroy the tab bar and recreate it from the tabview
> -(void)rebuildTabBar;
> // return the height the tab bar should be
> -(float)tabBarHeight;
> -(BrowserTabViewItem*)tabViewItemAtPoint:(NSPoint)location;
>
>+- (IBAction)overflowMenu:(id)sender;
>+
> @end
>
> @interface NSImage ( BrowserTabBarViewAdditions )
> - (void)paintTiledInRect:(NSRect)rect;
>Index: browser/BrowserTabBarView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.mm,v
>retrieving revision 1.1
>diff -u -4 -r1.1 BrowserTabBarView.mm
>--- browser/BrowserTabBarView.mm 2 Sep 2004 22:52:28 -0000 1.1
>+++ browser/BrowserTabBarView.mm 24 Sep 2004 03:28:26 -0000
>@@ -46,27 +46,36 @@
> -(void)drawTabBarBackgroundInRect:(NSRect)rect withActiveTabRect:(NSRect)tabRect;
> -(TabButtonCell*)buttonAtPoint:(NSPoint)clickPoint;
> -(void)registerTabButtonsForTracking;
> -(void)unregisterTabButtonsForTracking;
>+-(void)initOverflowMenu;
>+-(NSRect)tabsRect;
> @end
>
> static NSImage *gBackgroundImage = nil;
> static NSImage *gTabButtonDividerImage = nil;
>+static NSImage *gTabOverflowImage = nil;
> static const float kTabBarDefaultHeight = 22.0;
>
> @implementation BrowserTabBarView
>
> static const int kTabBarMargin = 5; // left/right margin for tab bar
>-static const float kMinTabWidth = 42.0; // tabs smaller than this are useless... tabs this small may be useless
>+static const float kMinTabWidth = 100.0; // the smallest tabs that will be drawn
> static const float kMaxTabWidth = 175.0; // the widest tabs that will be drawn
>
> static const int kTabDragThreshold = 3; // distance a drag must go before we start dnd
>
>+static const float kOverflowButtonWidth = 16;
>+static const float kOverflowButtonHeight = 16;
>+static const int kOverflowButtonMargin = 1;
>+
> -(id)initWithFrame:(NSRect)frame
> {
> self = [super initWithFrame:frame];
> if (self) {
> mActiveTabButton = nil;
>+ mOverflowButton = nil;
>+ mOverflowTabs = NO;
> // this will not likely have any result here
> [self rebuildTabBar];
> [self registerForDraggedTypes:[NSArray arrayWithObjects: @"MozURLType", @"MozBookmarkType", NSStringPboardType,
> NSFilenamesPboardType, NSURLPboardType, nil]];
>@@ -81,9 +90,12 @@
> }
>
> -(void)dealloc
> {
>+ [mTrackingCells release];
> [mActiveTabButton release];
>+ [mOverflowButton release];
>+ [mOverflowMenu release];
> [super dealloc];
> }
>
> -(void)drawRect:(NSRect)rect
>@@ -92,9 +104,12 @@
> if (!gBackgroundImage || !gTabButtonDividerImage)
> [self loadImages];
>
> // determine the frame of the active tab button and fill the rest of the bar in with the background
>- NSRect activeTabButtonFrame = [mActiveTabButton frame];
>+ NSRect activeTabButtonFrame = [mActiveTabButton frame];
>+ NSRect tabsRect = [self tabsRect];
>+ // if any of the active tab would be outside the drawable area, fill it in
>+ if (NSMaxX(activeTabButtonFrame) > NSMaxX(tabsRect)) activeTabButtonFrame.size.width = 0.0;
> [self drawTabBarBackgroundInRect:rect withActiveTabRect:activeTabButtonFrame];
> NSArray *tabItems = [mTabView tabViewItems];
> NSEnumerator *tabEnumerator = [tabItems objectEnumerator];
> BrowserTabViewItem *tab = [tabEnumerator nextObject];
>@@ -103,9 +118,9 @@
> TabButtonCell *tabButton = [tab tabButtonCell];
> BrowserTabViewItem *nextTab = [tabEnumerator nextObject];
>
> NSRect tabButtonFrame = [tabButton frame];
>- if (NSIntersectsRect(tabButtonFrame,rect))
>+ if (NSIntersectsRect(tabButtonFrame,rect) && NSMaxX(tabButtonFrame) <= NSMaxX(tabsRect))
> [tabButton drawWithFrame:tabButtonFrame inView:self];
> // draw the first divider.
> if ((prevButton == nil) && ([tab tabState] != NSSelectedTab))
> [gTabButtonDividerImage compositeToPoint:NSMakePoint(tabButtonFrame.origin.x - [gTabButtonDividerImage size].width, tabButtonFrame.origin.y)
>@@ -118,9 +133,11 @@
> -(void)setFrame:(NSRect)frameRect
> {
> [super setFrame:frameRect];
> // tab buttons probably need to be resized if the frame changes
>+ [self unregisterTabButtonsForTracking];
> [self layoutButtons];
>+ [self registerTabButtonsForTracking];
> }
>
> -(NSMenu*)menuForEvent:(NSEvent*)theEvent
> {
>@@ -133,9 +150,9 @@
> -(void)mouseDown:(NSEvent*)theEvent
> {
> NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
> TabButtonCell *clickedTabButton = [self buttonAtPoint:clickPoint];
>-
>+
> mLastClickPoint = clickPoint;
>
> if (clickedTabButton && ![clickedTabButton willTrackMouse:theEvent inRect:[clickedTabButton frame] ofView:self])
> [[[clickedTabButton tabViewItem] tabItemContentsView] mouseDown:theEvent];
>@@ -219,8 +236,10 @@
> if (!gBackgroundImage)
> gBackgroundImage = [[NSImage imageNamed:@"tab_bar_bg"] retain];
> if (!gTabButtonDividerImage)
> gTabButtonDividerImage = [[NSImage imageNamed:@"tab_button_divider"] retain];
>+ if (!gTabOverflowImage)
>+ gTabOverflowImage = [[NSImage imageNamed:@"tab_overflow"] retain];
> }
>
> // construct the tab bar based on the current state of mTabView;
> // should be called when tabs are first shown.
>@@ -237,30 +256,41 @@
> -(void)registerTabButtonsForTracking
> {
> if ([self window]) {
> NSArray * tabItems = [mTabView tabViewItems];
>+ if(mTrackingCells) [self unregisterTabButtonsForTracking];
>+ mTrackingCells = [NSMutableArray arrayWithCapacity:[tabItems count]];
>+ [mTrackingCells retain];
> NSEnumerator *tabEnumerator = [tabItems objectEnumerator];
>
> NSPoint local = [[self window] convertScreenToBase:[NSEvent mouseLocation]];
> local = [self convertPoint:local fromView:nil];
>
> BrowserTabViewItem *tab = nil;
> while (tab = [tabEnumerator nextObject]) {
> TabButtonCell * tabButton = [tab tabButtonCell];
>- NSRect trackingRect = [tabButton frame];
>- [tabButton addTrackingRectInView: self withFrame:trackingRect cursorLocation:local];
>+ if (tabButton) {
>+ [mTrackingCells addObject:tabButton];
>+ NSRect trackingRect = [tabButton frame];
>+ // only track tabs that are onscreen
>+ if (NSMaxX(trackingRect) <= NSMaxX([self tabsRect]))
>+ [tabButton addTrackingRectInView: self withFrame:trackingRect cursorLocation:local];
>+ }
> }
> }
> }
>
> // causes tab buttons to stop reacting to mouse events
> -(void)unregisterTabButtonsForTracking
> {
>- NSArray * tabItems = [mTabView tabViewItems];
>- NSEnumerator *tabEnumerator = [tabItems objectEnumerator];
>- BrowserTabViewItem *tab = nil;
>- while (tab = [tabEnumerator nextObject])
>- [[tab tabButtonCell] removeTrackingRectFromView: self];
>+ if (mTrackingCells) {
>+ NSEnumerator *tabEnumerator = [mTrackingCells objectEnumerator];
>+ TabButtonCell *tab = nil;
>+ while (tab = (TabButtonCell *)[tabEnumerator nextObject])
>+ [tab removeTrackingRectFromView: self];
>+ [mTrackingCells release];
>+ mTrackingCells = nil;
>+ }
> }
>
> // returns the height the tab bar should be if drawn
> -(float)tabBarHeight
>@@ -281,10 +311,24 @@
> {
> const int numTabs = [mTabView numberOfTabViewItems];
> float tabWidth = kMaxTabWidth;
>
>- // calculate the largest tabs that would fit
>+ // calculate the largest tabs that would fit... [self tabsRect] may not be correct until mOverflowTabs is set here.
> float maxWidth = floor((NSWidth([self bounds]) - (2*kTabBarMargin))/numTabs);
>+ // if tabs will overflow, leave space for the button
>+ if (maxWidth < kMinTabWidth) {
>+ mOverflowTabs = YES;
>+ int i = 1;
>+ NSRect tabsRect = [self tabsRect];
>+ while ((maxWidth < kMinTabWidth) && (i < numTabs)) {
>+ maxWidth = floor(NSWidth(tabsRect)/(numTabs - i));
>+ i++;
>+ }
>+ // because the specific tabs which overflow may change, empty the menu and rebuild it as tabs are laid out
>+ [self initOverflowMenu];
>+ } else {
>+ mOverflowTabs = NO;
>+ }
> // if our tabs are currently larger than that, shrink them to the larger of kMinTabWidth or maxWidth
> if (tabWidth > maxWidth)
> tabWidth = (maxWidth > kMinTabWidth ? maxWidth : kMinTabWidth);
> // resize and position the tab buttons
>@@ -298,27 +342,82 @@
> NSSize buttonSize = [tabButtonCell size];
> buttonSize.width = tabWidth;
> buttonSize.height = kTabBarDefaultHeight;
> NSPoint buttonOrigin = NSMakePoint(xCoord,0);
>- // TODO: take care of tabs that run off the end... right now we're the same as Firebird, except I think we
>- // hit our tab limit before we'll run off the end of the average user's windows given the current kMinTabWidth
> [tabButtonCell setFrame:NSMakeRect(buttonOrigin.x,buttonOrigin.y,buttonSize.width,buttonSize.height)];
>- // If the tab ran off the edge, suppress its close button
>- if (buttonOrigin.x + buttonSize.width > NSMaxX([self bounds]))
>- [tabButtonCell hideCloseButton];
> // tell the button whether it needs to draw the right side dividing line
>- if (NSSelectedTab == [tab tabState]) {
>+ if ([tab tabState] == NSSelectedTab) {
> [tabButtonCell setDrawDivider:NO];
> [prevTabButton setDrawDivider:NO];
> } else {
> [tabButtonCell setDrawDivider:YES];
> }
>+ // If the tab ran off the edge, suppress its close button, make sure the divider is drawn, and add it to the menu
>+ if (buttonOrigin.x + buttonSize.width > NSMaxX([self tabsRect])) {
>+ [tabButtonCell hideCloseButton];
>+ // push the tab off the edge of the view to keep it from grabbing clicks if there is an area
>+ // between the overflow menu and the last tab which is within tabsRect due to rounding
>+ [tabButtonCell setFrame:NSMakeRect(NSMaxX([self bounds]),buttonOrigin.y,buttonSize.width,buttonSize.height)];
>+ // if the tab prior to the overflow is not selected, it must draw a divider
>+ if([[prevTabButton tabViewItem] tabState] != NSSelectedTab) [prevTabButton setDrawDivider:YES];
>+ [mOverflowMenu addItem:[tab menuItem]];
>+ }
> prevTabButton = tabButtonCell;
> xCoord += (int)tabWidth;
> }
>+ // if tabs overflowed, position and display the overflow button
>+ if (mOverflowTabs) {
>+ [mOverflowButton setFrame:NSMakeRect(NSMaxX([self tabsRect]) + kOverflowButtonMargin,
>+ ([self tabBarHeight] - kOverflowButtonHeight)/2,kOverflowButtonWidth,kOverflowButtonHeight)];
>+ [self addSubview:mOverflowButton];
>+ } else {
>+ [mOverflowButton removeFromSuperview];
>+ }
> [self setNeedsDisplay:YES];
> }
>
>+-(void)initOverflowMenu
>+{
>+ if (!mOverflowButton) {
>+ // if it hasn't been created yet, create an NSPopUpButton and retain a strong reference
>+ mOverflowButton = [[NSButton alloc] initWithFrame:NSMakeRect(0,0,kOverflowButtonWidth,kOverflowButtonHeight)];
>+ if (!gTabOverflowImage) [self loadImages];
>+ [mOverflowButton setImage:gTabOverflowImage];
>+ [mOverflowButton setImagePosition:NSImageOnly];
>+ [mOverflowButton setBezelStyle:NSShadowlessSquareBezelStyle];
>+ [mOverflowButton setBordered:NO];
>+ [[mOverflowButton cell] setHighlightsBy:NSNoCellMask];
>+ [mOverflowButton setTarget:self];
>+ [mOverflowButton setAction:@selector(overflowMenu:)];
>+ [(NSButtonCell *)[mOverflowButton cell]sendActionOn:NSLeftMouseDownMask];
>+ }
>+ if (!mOverflowMenu) {
>+ // create an empty NSMenu for later use and retain a strong reference
>+ mOverflowMenu = [[NSMenu alloc] init];
>+ [mOverflowMenu addItemWithTitle:@"" action:NULL keyEquivalent:@""];
>+ }
>+ // remove any items on the menu other than the dummy item
>+ for (int i = [mOverflowMenu numberOfItems]; i > 1; i--)
>+ [mOverflowMenu removeItemAtIndex:i-1];
>+}
>+
>+- (IBAction)overflowMenu:(id)sender
>+{
>+ NSPopUpButtonCell *popupCell = [[[NSPopUpButtonCell alloc] initTextCell:@"" pullsDown:YES] autorelease];
>+ [popupCell setMenu:mOverflowMenu];
>+ [popupCell trackMouse:[NSApp currentEvent] inRect:[sender bounds] ofView:sender untilMouseUp:YES];
>+}
>+
>+// returns an NSRect of the area where tab widgets may be drawn
>+-(NSRect)tabsRect
>+{
>+ NSRect rect = [self bounds];
>+ rect.origin.x += kTabBarMargin;
>+ rect.size.width -= 2 * kTabBarMargin + (mOverflowTabs ? kOverflowButtonWidth : 0.0);
>+ return rect;
>+}
>+
>+
> #pragma mark -
>
> // NSDraggingDestination destination methods
> -(unsigned int)draggingEntered:(id <NSDraggingInfo>)sender
>Index: browser/BrowserTabViewItem.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabViewItem.h,v
>retrieving revision 1.7
>diff -u -4 -r1.7 BrowserTabViewItem.h
>--- browser/BrowserTabViewItem.h 2 Sep 2004 22:52:28 -0000 1.7
>+++ browser/BrowserTabViewItem.h 24 Sep 2004 03:28:26 -0000
>@@ -50,8 +50,9 @@
> NSRect mLastDrawRect; // cached draw rect, used for dragging location
> BrowserTabItemContainerView* mTabContentsView;
> NSProgressIndicator* mProgressWheel; // STRONG ref
> NSButton* mCloseButton; // STRING ref
>+ NSMenuItem* mMenuItem; // STRONG ref
> BOOL mDraggable;
> int mTag;
> }
>
>@@ -69,8 +70,12 @@
> // call before removing to clean up close button and progress spinner
> - (void)willBeRemoved:(BOOL)remove;
>
> - (NSButton *)closeButton;
>+- (NSMenuItem *)menuItem;
>+- (void) willDeselect;
>+- (void) willSelect;
>+- (void) selectTab:(id)sender;
>
> + (NSImage*)closeIcon;
> + (NSImage*)closeIconPressed;
>
>Index: browser/BrowserTabViewItem.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabViewItem.mm,v
>retrieving revision 1.12
>diff -u -4 -r1.12 BrowserTabViewItem.mm
>--- browser/BrowserTabViewItem.mm 17 Sep 2004 13:37:04 -0000 1.12
>+++ browser/BrowserTabViewItem.mm 24 Sep 2004 03:28:26 -0000
>@@ -51,8 +51,10 @@
>
> // we cannot use the spinner before 10.2, so don't allow it. This is the
> // version of appkit in 10.2 (taken from the 10.3 SDK headers which we cannot use).
> const double kJaguarAppKitVersion = 663;
>+// truncate menuitem title to the same width as the bookmarks menu
>+const int kMenuTruncationChars = 60;
>
> @interface BrowserTabViewItem(Private)
> - (void)setTag:(int)tag;
> - (void)relocateTabContents:(NSRect)inRect;
>@@ -106,8 +108,10 @@
> - (void)dealloc
> {
> [mLabelCell release];
> [mTabButtonCell release];
>+ // needs to be nil so that [super dealloc]'s call to setMenu doesn't cause us to call setMenu on an invalid object
>+ mTabButtonCell = nil;
> [super dealloc];
> }
>
> - (TruncatingTextAndImageCell*)labelCell
>@@ -339,9 +343,14 @@
> [mCloseButton setButtonType:NSMomentaryChangeButton];
> [mCloseButton setTarget:self];
> [mCloseButton setAction:@selector(closeTab)];
> [mCloseButton setAutoresizingMask:NSViewMinXMargin];
>- [mCloseButton retain];
>+
>+ // create a menu item, to be used when there are more tabs than screen real estate. keep a strong ref
>+ // since it will be added to and removed from the menu repeatedly
>+ mMenuItem = [[NSMenuItem alloc] initWithTitle:[self label] action:@selector(selectTab:) keyEquivalent:@""];
>+ [mMenuItem setTarget:self];
>+ [mMenuItem retain];
>
> [[self tabView] setAutoresizesSubviews:YES];
>
> mDraggable = NO;
>@@ -362,8 +371,9 @@
> [mTabContentsView removeFromSuperview]; // may be noop
> [mTabContentsView release]; // balance our init
> [mProgressWheel release];
> [mCloseButton release];
>+ [mMenuItem release];
> [super dealloc];
> }
>
> - (NSView*)tabItemContentsView
>@@ -422,8 +432,9 @@
> - (void)setLabel:(NSString *)label
> {
> NSAttributedString* labelString = [[[NSAttributedString alloc] initWithString:label attributes:mLabelAttributes] autorelease];
> [[mTabContentsView labelCell] setAttributedStringValue:labelString];
>+ [mMenuItem setTitle:[label stringByTruncatingTo:kMenuTruncationChars at:kTruncateAtMiddle]];
> [(BrowserTabView *)[self tabView] refreshTabBar:NO];
>
> [super setLabel:label];
> }
>@@ -446,8 +457,9 @@
> -(void)setTabIcon:(NSImage *)newIcon
> {
> [super setTabIcon:newIcon];
> [[mTabContentsView labelCell] setImage:mTabIcon];
>+ [mMenuItem setImage:mTabIcon];
> [(BrowserTabView *)[self tabView] refreshTabBar:NO];
> }
>
> - (void)setTabIcon:(NSImage *)newIcon isDraggable:(BOOL)draggable
>@@ -499,8 +511,29 @@
> {
> return mCloseButton;
> }
>
>+- (NSMenuItem *) menuItem
>+{
>+ return mMenuItem;
>+}
>+
>+- (void) selectTab:(id)sender
>+{
>+ [[self tabView] selectTabViewItem:self];
>+}
>+
>+// called by delegate when a tab will be deselected
>+- (void) willDeselect
>+{
>+ [mMenuItem setState:NSOffState];
>+}
>+// called by delegate when a tab will be selected
>+- (void) willSelect
>+{
>+ [mMenuItem setState:NSOnState];
>+}
>+
> #pragma mark -
>
> + (NSImage*)closeIcon
> {
>Index: browser/BrowserWindowController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserWindowController.mm,v
>retrieving revision 1.144
>diff -u -4 -r1.144 BrowserWindowController.mm
>--- browser/BrowserWindowController.mm 2 Sep 2004 22:52:28 -0000 1.144
>+++ browser/BrowserWindowController.mm 24 Sep 2004 03:28:30 -0000
>@@ -125,9 +125,9 @@
> // Cached toolbar defaults read in from a plist. If null, we'll use
> // hardcoded defaults.
> static NSArray* sToolbarDefaults = nil;
>
>-#define kMaxBrowserWindowTabs 16
>+#define kMaxBrowserWindowTabs 0
>
> //////////////////////////////////////
> @interface AutoCompleteTextFieldEditor : NSTextView
> {
>@@ -2364,8 +2364,20 @@
> // Make the new view the primary content area.
> [mBrowserView makePrimaryBrowserView: mURLBar status: mStatus windowController: self];
> }
>
>+- (void)tabView:(NSTabView *)aTabView willSelectTabViewItem:(NSTabViewItem *)aTabViewItem
>+{
>+ // we'll get called for the sidebar tabs as well. ignore any calls coming from
>+ // there, we're only interested in the browser tabs.
>+ if (aTabView != mTabBrowser)
>+ return;
>+ if ([aTabView isKindOfClass:[BrowserTabView class]] && [aTabViewItem isKindOfClass:[BrowserTabViewItem class]]) {
>+ [(BrowserTabViewItem *)[aTabView selectedTabViewItem] willDeselect];
>+ [(BrowserTabViewItem *)aTabViewItem willSelect];
>+ }
>+}
>+
> - (void)tabViewDidChangeNumberOfTabViewItems:(NSTabView *)aTabView
> {
> [[NSApp delegate] fixCloseMenuItemKeyEquivalents];
> [mTabBrowser refreshTabBar:YES];
>Index: extensions/RolloverTrackingCell.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/RolloverTrackingCell.mm,v
>retrieving revision 1.1
>diff -u -4 -r1.1 RolloverTrackingCell.mm
>--- extensions/RolloverTrackingCell.mm 2 Sep 2004 22:52:29 -0000 1.1
>+++ extensions/RolloverTrackingCell.mm 24 Sep 2004 03:28:30 -0000
>@@ -88,20 +88,26 @@
> NSDictionary *userData = (NSDictionary *)[theEvent userData];
> NSView *view = [userData objectForKey:@"view"];
> mMouseWithin = YES;
> // only act on the mouseEntered if the view is active or accepts the first mouse click
>- if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent])
>+ if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent]) {
> [view setNeedsDisplayInRect:[self frame]];
>+ // calling displayIfNeeded prevents the "lag" observed when displaying rollover events
>+ [view displayIfNeeded];
>+ }
> }
>
> - (void)mouseExited:(NSEvent*)theEvent
> {
> NSDictionary *userData = (NSDictionary*)[theEvent userData];
> NSView *view = [userData objectForKey:@"view"];
> mMouseWithin = NO;
> // only act on the mouseExited if the view is active or accepts the first mouse click
>- if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent])
>+ if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent]) {
> [view setNeedsDisplayInRect:[self frame]];
>+ // calling displayIfNeeded prevents the "lag" observed when displaying rollover events
>+ [view displayIfNeeded];
>+ }
> }
>
> - (void)setDragTarget:(BOOL)isDragTarget
> {
>Index: extensions/TabButtonCell.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/TabButtonCell.h,v
>retrieving revision 1.1
>diff -u -4 -r1.1 TabButtonCell.h
>--- extensions/TabButtonCell.h 2 Sep 2004 22:52:29 -0000 1.1
>+++ extensions/TabButtonCell.h 24 Sep 2004 03:28:30 -0000
>@@ -42,8 +42,9 @@
>
> @interface TabButtonCell : RolloverTrackingCell {
> BrowserTabViewItem *mTabViewItem;
> BOOL mNeedsDivider;
>+ NSButton * mCloseButton;
> }
>
> -(id)initFromTabViewItem:(BrowserTabViewItem*)tabViewItem;
> -(BOOL)isSelected;
>Index: extensions/TabButtonCell.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/TabButtonCell.mm,v
>retrieving revision 1.1
>diff -u -4 -r1.1 TabButtonCell.mm
>--- extensions/TabButtonCell.mm 2 Sep 2004 22:52:29 -0000 1.1
>+++ extensions/TabButtonCell.mm 24 Sep 2004 03:28:31 -0000
>@@ -63,18 +63,17 @@
> @implementation TabButtonCell
> -(id)initFromTabViewItem:(BrowserTabViewItem *)tabViewItem
> {
> [super init];
>- mTabViewItem = [tabViewItem retain];
>+ mTabViewItem = tabViewItem;
> mNeedsDivider = YES;
> return self;
> }
>
>-// XXX does the menu need released here??
> -(void)dealloc
> {
>- [[mTabViewItem closeButton] removeFromSuperview];
>- [mTabViewItem release];
>+ [mCloseButton removeFromSuperview];
>+ [mCloseButton release];
> [super dealloc];
> }
>
> -(BOOL)isSelected
>@@ -101,9 +100,13 @@
> // XXX is it worth it to see if the load failed? I don't think so, as if it did, the bundle is trashed and
> // we have bigger problems
>
> NSSize textSize = [mTabViewItem sizeOfLabel:NO];
>- NSSize buttonSize = [[mTabViewItem closeButton] frame].size;
>+
>+ // retain a reference to the close button; otherwise we can't be sure it's removed from the view hierarchy
>+ // during our d'tor, when mTabViewItem will be invalid.
>+ if (!mCloseButton) mCloseButton = [[mTabViewItem closeButton] retain];
>+ NSSize buttonSize = [mCloseButton frame].size;
>
> // center based on the larger of the two heights if there's a difference
> float maxHeight = textSize.height > buttonSize.height ? textSize.height : buttonSize.height;
> NSRect buttonRect = NSMakeRect(rect.origin.x + kTabLeftMargin,
>@@ -147,15 +150,12 @@
> [[NSColor colorWithCalibratedRed:0.7 green:0.8 blue:1.0 alpha:1.0] set];
> rect.origin.y += kTabBottomPad;
> NSRectFill(rect);
> }
>- NSButton *closeButton = [mTabViewItem closeButton];
>- if (controlView != [closeButton superview]) {
>- [controlView addSubview:closeButton];
>+ if (controlView != [mCloseButton superview]) {
>+ [controlView addSubview:mCloseButton];
> }
>- [closeButton setFrame:buttonRect];
>- // XXX is this necessary, or even good?
>- [closeButton setNeedsDisplay:YES];
>+ [mCloseButton setFrame:buttonRect];
> [labelCell drawInteriorWithFrame:labelRect inView:controlView];
> }
>
> -(void)addTrackingRectInView:(NSView *)aView withFrame:(NSRect)trackingRect cursorLocation:(NSPoint)currentLocation
>@@ -169,11 +169,10 @@
> }
>
> -(void)hideCloseButton
> {
>- NSButton * closeButton = [mTabViewItem closeButton];
>- if ([closeButton superview] != nil)
>- [closeButton removeFromSuperview];
>+ if ([mCloseButton superview] != nil)
>+ [mCloseButton removeFromSuperview];
> }
>
> -(void)setDrawDivider:(BOOL)willDraw
> {
Attachment #159936 -
Flags: review?(qa-mozilla) → review+
Comment 6•21 years ago
|
||
Reporter | ||
Comment 7•21 years ago
|
||
Comment on attachment 159936 [details] [diff] [review]
version 2
Can we get permission for mozilla@derailer.org to set r+ on bugs he's been
asked to review?
Attachment #159936 -
Flags: review?(mozilla) → superreview?(pinkerton)
*** Bug 181402 has been marked as a duplicate of this bug. ***
*** Bug 161520 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 159936 [details] [diff] [review]
version 2
sr-
This leaks the BrowserTabView because the overflow button sets the tab view as
its target, which retains. there's now a cycle in the ownership hierarchy so
neither ever gets released.
there are a few ways to fix this, i think the easiest is to not use the tab
view as the target, though then you have to be very careful. You need something
out of band to break the cycle.
Sorry :(
Attachment #159936 -
Flags: superreview?(pinkerton) → superreview-
Reporter | ||
Comment 11•21 years ago
|
||
This version of the patch:
1. Fixes the memory leak Mike found (good catch!)
2. Changes a while loop that really should have been a for loop (mentioned on
IRC) into a for loop :-)
The actual cause of the memory leak was that
addTrackingRect:owner:userData:assumeInside implicitly retains owner without
documenting that fact. The solution was to have the window delegate notify the
tab view that the window was closing.
Reporter | ||
Updated•21 years ago
|
Attachment #159936 -
Attachment is obsolete: true
Reporter | ||
Updated•21 years ago
|
Attachment #160712 -
Flags: superreview?(pinkerton)
Attachment #160712 -
Flags: review?(mozilla)
Comment 12•21 years ago
|
||
Comment on attachment 160712 [details] [diff] [review]
version 3
Looks good to me; I'm not seeing any glaring omissions here.
Attachment #160712 -
Flags: review?(mozilla) → review+
Assignee | ||
Comment 13•21 years ago
|
||
landed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 14•21 years ago
|
||
Comment on attachment 160712 [details] [diff] [review]
version 3
adding + flag for pink's sr
Attachment #160712 -
Flags: superreview?(pinkerton) → superreview+
Comment 15•21 years ago
|
||
It'd be really nice to see an option to change the number of tabs that exist
before going to the menu. I'd personally like to be able to have more, but it
may be some people with different size screens would like to have less tabs
before going to the menu.
You need to log in
before you can comment on or make changes to this bug.
Description
•