319777 - When a tab in the overflow menu is active, highlight the overflow menu (or scroll the tab bar)

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[froodian (Ian Leue)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051208 Camino/1.0b1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051208 Camino/1.0b1+

When there are enough tabs to create an overflow menu, and one of the tabs in the overflow menu is in front, we should give the overflow menu the "active tab" look.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I like this idea, though I don't know if it's possible.  

(If so, maybe we could extend it so that acted as a proxy for the tab for the context menu--"move tab to new window" is something I most often want to do with the very tabs I cannot see the widget for and thus get a CM for.)

The drawback of this is that we start to mix metaphors a bit here: when you drag a url/bookmark/webloc/etc. to the chevron, it makes a new tab.  When you drag them to an active tab widget, it replaces the current tab.  Maybe that won't be a problem; dunno.

Comment 2

[Mike Pinkerton (not reading bugmail)]

what about "scrolling" so that it's in view and adding a chevron to both ends?

Comment 3

[Håkan Waara]

I'm definitely in favor of scrolling to the active tab and adding chevron to both ends, like comment 2 suggests.   Right now, not being able to see the current tab(s) when they overflow is just a pain, IMHO.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think we should look at this for 1.2 in conjunction with reorderable tabs--presumably the two will need to work together.

Comment 5

[Jasper]

Attachment: overflow on both ends

Back in the days when Geoff and I worked on the current tab implementation we talked about this. And we both thought that doing overflow menu's on both ends of the tab bar would be the best and easy way for people to use.

As seen in the screen shot we'd have a button on both sides. Clicking on them would show the overflow menu. Selecting an item would slide the tabs to the selected tab. We could also do click and holding on the button, but then it could happen that non of the tabs in view are selected, like now, which is rather odd.

I'd be really nice if we could make the slide effect be similar to the speed and slow down as all osx apps have in the toolbar. I'd say that tab reorder would work the same as the toolbar effect aswell.

Comment 6

[froodian (Ian Leue)]

(In reply to comment #5)
> Created an attachment (id=212388) [edit]
> overflow on both ends

With this implementation, we'd lose all empty tab bar space (for dragging favicons to, for instance).  Is this a problem?

Comment 7

[Håkan Waara]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=212388) [edit]
> > overflow on both ends
> 
> With this implementation, we'd lose all empty tab bar space (for dragging
> favicons to, for instance).  Is this a problem?

What empty tab bar space? In this case all space is occupied by tabs...

Comment 8

[Håkan Waara]

Jasper, how do I get to the tab I want to, if it's far out somewhere and not near a selected tab? ?   It seemed like you wanted a selected tab to always be visible, unless I'm just confused.

Comment 9

[Markus Amalthea Magnuson]

A really slick solution would be if you could scroll the tab bar horisontally by pointing at the arrow icon. An icon would only be visible at the far left/right end if any tabs were outside the visible space. In other words it would work exactly as a regular meny, but horizontally instead of vertically.

Comment 10

[Jasper]

Like I said, if you click on either of the overflow buttons  the menu would appear with all the tabs that are overflown.

I think it's a bad idea to have an autoscrolling tab bar when you move your mouse to either of the side, then the UI would be constantly changing. I have seen flash movies who do this, and although it looks slick, it's a ui nightmare.

Comment 11

[Markus Amalthea Magnuson]

Why isn't it a ui nightmare in regular menus?

Comment 12

[Stuart Morgan]

Regular menus aren't persistent; you don't have to avoid mousing over them in general use.  If mousing over a chevron scrolled, then every time you moved your cursor from the content area to bookmark bar, toolbar, menu, or a different window, you would have to be very careful about the path the mouse took in order to avoid unintended scrolling.

Comment 13

[Simon Fraser [no longer active]]

The problem with scrolling the tab bar is that you lose positional consistency with the tabs. Is the leftmost (visible) tab the first tab, or the Nth tabs? This is getting dangerously close to the Windows nightmare of stacked tabs for me.

Comment 14

[Stuart Morgan]

You'd always know if the leftmost was the first by whether or not there is a chevron, the same you currently know if the rightmost is the end. Plus, this would only apply in the case where there are more tabs than would fit, and there already isn't any positional information for the tabs beyond the first n.  If I'm in a spillover menu, where am I?  The end? Near the end?  Not even close?

It comes down to whether in the many-tabs case it's better to have some tabs be arbitrarily very accessible, and the rest highly inaccessible, or to distribute lesser pain evenly across all tabs.

Comment 15

[Håkan Waara]

I vote for chevrons (both sides if needed) and:

* Single click = scroll one tab
* Clicking-and-holding = scrolling

Comment 16

[Jasper]

So no more menu's, so no more easy way to quickly select a tab from it? That sounds like a bad idea. Perhaps command or shift click is one tab?

Comment 17

[Håkan Waara]

If we fix this together with bug 197720 (Show tabs in Window menu, indented) we get the best of both worlds.

Comment 18

[Simon Fraser [no longer active]]

I just think that scrolling tabs are bad for positional memory.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I don't exactly disagree with Simon, and I'm sure I've read some negative reviews of this in whichever browser has it--but working in disembodied tabs is confusing, too. :-(

If we do go with "scrolling tab bar", the overflow menus need to remain *menus*.  Making them buttons only improves the situation (access to hidden tabs) if the tab you want to get to is the next tab, and we already have keyboard shortcuts to do that.  (And don't get me started about click-and-hold and its evils.)  The overflow menus, however, immediately let me see and select the tab I need, whether it's the next tab over or 10 tabs over.

(Also, if we do go with scrolling tabs, the tabs should only move when a new one outside of view is selected--and then always move to a standard position within the view, which we'd need to figure out--clicking among tabs in view should never cause the bar to scroll.)

I almost think it might be better--at least in the short term--to do what froodian suggested in this bug originally, give the overflow widget the active look, and also do what timeless wanted in bug 318123.

Comment 20

[Desmond Elliott]

A sketch of my proposed implementation is available at http://wiki.caminobrowser.org/Image:Bug319777.jpg

Comment 21

[Desmond Elliott]

Attachment: This is an interim patch that has a few potential redraw problems that need to be investiaged.

This bug is not complete, there are a few bugs that need to be dealt with, specifically:

It seems like when you click to scroll the tab bar the tab to at the left-most or right-most (depending on which way you are scrolling) does not redraw correctly.

When a user clicks to scroll, then resizes the window, there appears to be some weird drawing problems.

This is my first patch submission so I probably do not adhere to Mozilla coding guidelines very strictly. Please strongly criticise my code.

Comment 22

[Desmond Elliott]

Attachment: Left scroll button

Comment 23

[Desmond Elliott]

Attachment: Right scroll button

Comment 24

[Desmond Elliott]

A visual representation of the work - ableit broken - for users who do not want to build.

http://www.delliott.eu/soc/video/tabScrollingFirstDemo.mov

Comment 25

[Mike Pinkerton (not reading bugmail)]

+int visibleTabs; // the # of tabs visible in the tab bar
+int currentStart = 0; // the index of the left-most visible tab

are these globals? this won't work at all if there's > 1 window. make them member variables on the tab bar.

Comment 26

[Nick Kreeger]

Comment on attachment 226812 [details] [diff] [review]
This is an interim patch that has a few potential redraw problems that need to be investiaged.

+int visibleTabs; // the # of tabs visible in the tab bar
+int currentStart = 0; // the index of the left-most visible tab
+

Are these supposed to be global members of class members?
Which ever they are supposed to be, please use the appropriate prefix:
Class member: mVisibleTabs
Global: kVisibleTabs

 -(void)dealloc
 {
   [mTrackingCells release];
   [mActiveTabButton release];
-  [mOverflowButton release];
-  [mOverflowMenu release];

If you commented out the other parts in code where these two variables where, why not comment them out here?

 -(void)mouseDown:(NSEvent*)theEvent
 {
+
   NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
   TabButtonCell *clickedTabButton = [self buttonAtPoint:clickPoint];
   
Any reason why there is new line here?


-// construct the tab bar based on the current state of mTabView;
-// should be called when tabs are first shown.
 -(void)rebuildTabBar
 {
-  [self loadImages];
 
+  // This method constructs the tab bar based on the current state of mTabView.
+  // It unregisters tab buttons, releases the active tab, resets the active tab
+  // lays out the tabs again, and re-registers the tabs.
+
+  [self loadImages];

Can you just put the new comments where the old comments where and remove the extra new line at the beggining of the method?

-// sets the tab buttons to the largest kMinTabWidth <= size <= kMaxTabWidth where they all fit
-// and calculates the frame for each.
 -(void)layoutButtons
 {
+
+  // This method sets the tab buttons to the largest kMinTabWidth 
+  // <= kMatTabWidth so that the visible tabs are best fit.
+     
   const int numTabs = [mTabView numberOfTabViewItems];
   float tabWidth = kMaxTabWidth;

Same story here.

+  float maxWidth = floor((NSWidth([self bounds]) - (2*kTabBarMarginL))/numTabs);
+  float width = NSWidth([self bounds]);
+

Rather than calculating the NSWidth of [self bounds] several times why not get your |width| first, then use it to get the max width.
Also, since you spaced out your minus operator, you should space out your multiply operator too.

+    int i = 0;
+	
+    while (maxWidth < kMinTabWidth) {
+	  visibleTabs = numTabs - i; // The # of tabs visible in the tab bar
+      maxWidth = floor((NSWidth([self bounds]) - (2*kTabBarMarginL))/(numTabs - i));
+      i++;
     }

This while loop has the potential to be dangerous. What happens when your i is greater than the numTabs? (does that happen?)
I would stick with the for loop that was there in the first place.
Also, rather than calculating visibleTabs every iteration, you should use set it once below the for loop with your |i| variable.
You should use your |width| variable here as well. (from above)
Also what are you using the visibleTabs variable for here? You do the same operation in maxWidth, maybe you should use it there.

Looks like you have tabs in here, also, i like two spaces after each character for a variable:
  numTabs - i;  // The #....
  
+
+	[self initOverflow]; //DE draw the buttons
+  }

+  else {
     mOverflowTabs = NO;
+	visibleTabs = numTabs;
   }

Looks like more tabs (or strange spacing?)

+  // If we are not overflowing then do not draw in the button margin
+  int xCoord;
+  if (!mOverflowTabs) 
+  {
+    xCoord = kTabBarMargin;
+  }
+  else
+  {
+    xCoord = kTabBarMarginL;
+  }

Maybe here you could use a condensded statement like:
int xCoord = (!mOverflowTabs ? kTabBarMargin : kTabBarMarginL);

   while ((tab = [tabEnumerator nextObject])) {
-    TabButtonCell *tabButtonCell = [tab tabButtonCell];
-    NSSize buttonSize = [tabButtonCell size];
-    buttonSize.width = tabWidth;
-    buttonSize.height = kTabBarDefaultHeight;
-    NSPoint buttonOrigin = NSMakePoint(xCoord,0);
-    [tabButtonCell setFrame:NSMakeRect(buttonOrigin.x,buttonOrigin.y,buttonSize.width,buttonSize.height)];
+    if (i < currentStart) {
+	  i++;
+	}

Please space between the commas on calls.
Rather than wrapping the next chunk of code in a else statement, how about a continue call if i < currentStart?

+	else {
+      TabButtonCell *tabButtonCell = [tab tabButtonCell];
+      NSSize buttonSize = [tabButtonCell size];
+      buttonSize.width = tabWidth;
+      buttonSize.height = kTabBarDefaultHeight;
+      NSPoint buttonOrigin = NSMakePoint(xCoord,0);
+      [tabButtonCell setFrame:NSMakeRect(buttonOrigin.x,buttonOrigin.y,buttonSize.width,buttonSize.height)];
 	  // tell the button whether it needs to draw the right side dividing line
 	  if ([tab tabState] == NSSelectedTab) {
 		  [tabButtonCell setDrawDivider:NO];
 		  [prevTabButton setDrawDivider:NO];
-	  } else {
+	  }
+	  else {
 		  [tabButtonCell setDrawDivider:YES];
 	  }

Looks like some more strange tabs/spaces combination here. With a text editor like BBEdit (or TextWrangler the free version) you can detab a section of text.
Xcode will sometimes forget its policy on tabs regardless of prefs if tabs and spaces are in a section you are editing.

 	  if (buttonOrigin.x + buttonSize.width > NSMaxX([self tabsRect])) {
-		  [tabButtonCell hideCloseButton];
+	    [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)];
+        [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]];
-    }

Rather than making a hundered calls to NSMaxX, it would be cleaner if you had some variables in the method that looked like:
  boundsMaxX = NSMaxX([self bounds]);
  
+    [mOverflowButtonRight setFrame:NSMakeRect(NSWidth([self tabsRect]) + kTabBarMarginL,
+                                         ([self tabBarHeight] - kOverflowButtonHeight)/2, kOverflowButtonWidth, kOverflowButtonHeight)];
+    [self addSubview:mOverflowButtonRight];
+	[mOverflowButtonLeft setFrame:NSMakeRect(0,
+                                         ([self tabBarHeight] - kOverflowButtonHeight)/2, kOverflowButtonWidth, kOverflowButtonHeight)];//DE
+    [self addSubview:mOverflowButtonLeft];//DE
+  } 
+  else {
+    [mOverflowButtonRight removeFromSuperview];
+	[mOverflowButtonLeft removeFromSuperview];//DE
   }

Match your spacing here, if there is a space between a subtract operator, please put a space around your divide operator.
Also may not need to be a new space after NSMakeRect(0, 
And make sure you put two spaces between your code and the // comment

-- (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];
+  //[self rebuildTabBar];
 }
 
You removed this entire method, but left the comment??

+-(void)initOverflow
+{
+  if (!mOverflowButtonRight) {
+    mOverflowButtonRight = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, kOverflowButtonWidth, kOverflowButtonHeight)];
+    [mOverflowButtonRight setImage:[NSImage imageNamed:@"r"]];

+	mOverflowButtonLeft = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, kOverflowButtonWidth, kOverflowButtonHeight)];
+    [mOverflowButtonLeft setImage:[NSImage imageNamed:@"l"]];

How many times is |initOverflow| getting called, if it is more than once, your leaking on the NSButton alloc calls.
Also is the image really called "r" and "l"?\\

+  NSArray *tabItems = [mTabView tabViewItems];
+  int numTabs = [tabItems count];

You can condense this down to one line:
  int numTabs = [[mTabView tabViewItems] count];

NITS:
-----
+  NSButton*         mOverflowButtonRight; //DE allows a user to scroll to the right
+  NSButton*         mOverflowButtonLeft; //DE allows a user to scroll to the left

Perhaps name the buttons: mOverflowRightButton and mOverflowLeftButton.

+-(IBAction)overflowControlLeft:(id)aSender; //DE called when a user has more tabs than can be displayed without overflow
+-(IBAction)overflowControlRight:(id)aSender; //DE called when a user has more tabs than can be displayed without overflow

+static const int kTabBarMarginL = 18; // left margin for tab bar when overflowing
+static const int kTabBarMarginR = 0; // right margin for tab bar
+static const int kTabBarMargin = 5; // left margin for tab bar when not overflowing

Please align your comments here.

Also I would actually write out Left and Right on your static int's (kTabBarMarginLeft...)

//DEmOverflowButton = nil;

On our next go around of the patch could you maybe put your comment like this (rather than using initials)
//mOverflowButton = nil;  Removing for now DE

Or something, the 'DEm' runs together


Good job so far, keep it coming!

Comment 27

[Chris Lawson (gone)]

(In reply to comment #25)
> +int visibleTabs; // the # of tabs visible in the tab bar
> +int currentStart = 0; // the index of the left-most visible tab
> 
> are these globals? this won't work at all if there's > 1 window. make them
> member variables on the tab bar.

Also, shouldn't those be unsigned? Is it possible for either of those *ever* to be negative?

cl

Comment 28

[Simon Fraser [no longer active]]

If indeed we do want to move the tabs around like this (which I don't agree with), then we'll absolutely have to animate the scrolling, otherwise you'll loose all sense of which tab is which (imagine 10 tabs all with the same favicon and similar/identical titles).

Comment 29

[Bruce Davidson]

Comment on attachment 226812 [details] [diff] [review]
This is an interim patch that has a few potential redraw problems that need to be investiaged.

Delliott, this is a good first attempt at a patch. The comments below are not to criticise but to (hopefully) help you write better patches in future. If you have any questions then please get in touch on IRC or by e-mail.

#include "Nick.Kreegers.Comments" :-)

+static const int kTabBarMarginR = 0; // right margin for tab bar

You don't seem to use this right margin anywhere in your patch (see also
later comments).

+static const int kTabBarMargin = 5; // left margin for tab bar when not overflowing

The names kTabBarMargin and kTabBarMarginL don't to me indicate
anything about the overflow state. Possibly rename these vars.

   // 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);
+  float maxWidth = floor((NSWidth([self bounds]) - (2*kTabBarMarginL))/numTabs);
+  float width = NSWidth([self bounds]);

Your margin constants imply assymetric left and right margins, but your
calculation here implies a symmetric margin based on the left margin?

-    NSRect tabsRect = [self tabsRect];
-    for (int i = 1; i < numTabs; i++) {
-      maxWidth = floor(NSWidth(tabsRect)/(numTabs - i));
-      if (maxWidth >= kMinTabWidth) break;
+    int i = 0;
+	
+    while (maxWidth < kMinTabWidth) {
+	  visibleTabs = numTabs - i; // The # of tabs visible in the tab bar
+      maxWidth = floor((NSWidth([self bounds]) - (2*kTabBarMarginL))/(numTabs - i));
+      i++;
     }

The earlier code not withstanding wouldn't it be quicker and more obvious
to replace the loop with a simple calculation of how many minimum-width tabs
we can fit in the available space, and then work out how large they can be to
just soak up the space? Also don't we need to use a different margin
here that allows sufficient space for the overflow buttons?
e.g.:

  // set usefulWidth to width excluding margins once
  visibleTabs = floor(usefulWidth/kMinTabWidth);
  maxWidth    = floor(usefulWidth/visibleTabs);

-  } else {
+	
+	[self initOverflow]; //DE draw the buttons
+  } 
+  else {
	
Nit: either keep "} else {" all on one line or spread it over three lines.
(The official Mozilla style guide prefers the first, but Camino seems to
be moving towards the (IMHO more readable) latter.)

+    if (i < currentStart) {
+	  i++;
+	}
+	else {
	
Nits: tabs and two line else! Also pre-incrementing is better practice (modern
compilers should optimise either form identically for ints, but useful to be
in the habit so that when you write this for a C++ iterator you don't
get bitten by the performance issues of post-incrementing).

+-(void)initOverflow
+{
+  if (!mOverflowButtonRight) {
+    mOverflowButtonRight = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, kOverflowButtonWidth, kOverflowButtonHeight)];
+    [mOverflowButtonRight setImage:[NSImage imageNamed:@"r"]];
...
+    [mOverflowButtonRight setAction:@selector(overflowControlRight:)];
+  }
+  if (!mOverflowButtonLeft) {
+	mOverflowButtonLeft = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, kOverflowButtonWidth, kOverflowButtonHeight)];
+    [mOverflowButtonLeft setImage:[NSImage imageNamed:@"l"]];
...
+    [mOverflowButtonLeft setAction:@selector(overflowControlLeft:)];
+  }
+}

Apart from the image and the action everything else about the overflow
buttons is identical. Can you not factor out the common bits into a small
subroutine that takes the NSButton to setup as an argument?

+-(IBAction)overflowControlLeft:(id)aSender
+{
...
+  if (currentStart > 0) {
+    currentStart -= 1;

Simple decrement (--currentStart;) would be clearer. (Also ++currentStart in
the action for the right hand button.)

Comment 30

[Desmond Elliott]

Attachment: WIP attempt 2

WIP patch 2. Only a few problems now - I think most of them are related to resizing the window and the left most tab not being set correctly. Fixed nearly all of the suggestions made by Kreeger.

Comment 31

[Desmond Elliott]

Attachment: Left hand scroll tab button

New, correctly named icon

Comment 32

[Desmond Elliott]

Attachment: Right hand scroll tab button

New, correctly named button.

Comment 33

[Bruce Davidson]

Comment on attachment 227087 [details] [diff] [review]
WIP attempt 2

--(void)initOverflowMenu;
+-(void)initOverflow;
+-(void)layoutButtons:startIndex:(int)start;

I couldn't find layoutButtons:startIndex in your patch. Is this an
obsolete holdover from your first patch?

+static const int kTabBarMarginWhenOverflowing = 18;   // left margin for tab bar when overflowing
+static const int kTabBarMargin = 5;                       // left margin for tab bar when not overflowing
+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

Nit: Comment alignment. The constant names are much better now, thanks.

+  float width = NSWidth([self bounds]);

In general avoid naming a variable with just a type of quantity (especially
if you have several such quantities in play). Calling it "availableWidth" might
avoid any "width of what?" type queries when people read the code.

   // calculate the largest tabs that would fit... [self tabsRect] may not be correct until mOverflowTabs is set here.

As per IRC conversation please let me know outcome of attempt to use non-loop
solution from previous review. It would definitely be neater if it works.

-    // because the specific tabs which overflow may change, empty the menu and rebuild it as tabs are laid out
-    [self initOverflowMenu];
+    int prev = mNumberOfVisibleTabs;
+    [self initOverflow]; // draw the buttons
   } else {

The int |prev| will go out of scope at the closing brace before the |else|,
by which point you haven't used it. Is this old code left in by mistake?

+  // If the width of the currently visible tabs is less than the maximum width
+  // and if the width of the currently visible tabs is greater than the minimum
+  // tab width then the width of the currently visible tabs = maxWidth as
+  // calculated above. 

That comments the exact algorithm you use, but it would be better to describe
the intent of the code, e.g. "Clamp the tab width to be between kMinTabWidth
and kMaxTabWidth".

+  // If we are not overflowing then do not draw in the button margin
+  int xCoord = (!mOverflowTabs ? kTabBarMargin : kTabBarMarginWhenOverflowing);

Please reverse your second and third arguments to the conditional operator
rather than negating the condition expression.

+  // Below is the code responsible for drawing the tabs into the tab bar.

Optional nit: Comments are good, concise ones are even better,
e.g. "The code below draws the..."

+  signed int currentBTVIIndex = 0; // Used to make sure the Enumerator starts at the right place.
+    

What is BTVI? Again if negative values of the index have a special meaning
document it here, or consider using an unsigned int.

+    // If the Enumerator is at a point that is less than the left-most index

Again, its better to write comment in terms of the intent of the code rather than
the code itself, so "If this tab is to the left of the first visible tab"
would help the user work out what the code is doing.

+    // then draw a tab that was no width at position kTabBarMarginWhenOverflowing.
+    // Also, hide this tab.
+    if (currentBTVIIndex < mLeftMostVisibleTabIndex) {
+        [tabButtonCell hideCloseButton];
+        [tabButtonCell setFrame:NSMakeRect(kTabBarMarginWhenOverflowing, buttonOrigin.y,
+                                         buttonSize.width - buttonSize.width, buttonSize.height)];

Isn't |buttonSize.width - buttonSize.width| just 0! :-)

Also all the block (apart from the last line) appear to be indented 4 rather
than the Camino-standard 2 spaces relative to the |if|.

+        [tabButtonCell setDrawDivider:YES];
+      ++currentBTVIIndex;
+    } else {
+      [tabButtonCell setFrame:NSMakeRect(buttonOrigin.x, buttonOrigin.y, 
+                                         buttonSize.width, buttonSize.height)];
+    // tell the button whether it needs to draw the right side dividing line
+    if ([tab tabState] == NSSelectedTab) {
+      [tabButtonCell setDrawDivider:NO];

You appear to have outdented here without actually finishing the child-block
of the |else|. Check that alignment and code logic are in, umm... "aligned"!

+    int boundsMaxX = NSMaxX([self bounds]);

boundsMaxX is a loop invariant. Good practice to calculate it before the loop,
although modern compilers can sometimes do this optimisation for you.
Also a few more blank lines between blocks of code in this part of the code
would help.

+        if([[prevTabButton tabViewItem] tabState] != NSSelectedTab) {
+        [prevTabButton setDrawDivider:YES];
+      }
+    }

Alignment has gone very screwy here. The child statement isn't indented relative
to the if, and goodness knows what the closing brace is lining up with!

Nit: Always put a space between a keyword and a following parenthesis.

+    prevTabButton = tabButtonCell;
+    xCoord += (int)tabWidth;
+    ++currentBTVIIndex;

Its a bit hard to tell with the aligment as it is, but it looks like you
increment currentBTVIIndex in both the true and false branches of the original
"off the left-hand edge?" if statement. If so then consider taking it outside
the if, e.g. right at the end of the loop body.

 // 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);
+  rect.origin.x += kTabBarMarginWhenOverflowing;
+  rect.size.width -= kTabBarMarginWhenOverflowing + (mOverflowTabs ? kOverflowButtonWidth : 0.0);

The use of |kTabBarMarginWhenOverflowing| appears to imply you always assume
overflow, but the conditional operator on |mOverflowTabs| implies the opposite.
Check whether your calculations are what you intend for both cases.

Comment 34

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Sorry for bringing my brain in late in the ballgame, and for this discordant note, but froodian and I talked about this bug a bit on irc after I built with the patch (and after I had already left some other, more positive, proto-comments on delliott's blog). :-(

I think we need to go back and refocus on the problem and reframe our solution.  Namely, the issue/problem to solve is to ensure that the active tab's tab widget is also visible on the tab bar, and that is the whole point of scrolling.

The current patch doesn't really accomplish that goal; the active tab's widget can still be scrolled off-screen (it's a bit disconcerting, actually, to see the tab bar move and the active tab/content remain the same, esp. in comparison to the current tab-switching behaviors with the shortcuts or overflow), nor does changing tabs with the keyboard shortcut ensure that the active tab always has a visible widget--the shortcuts don't cause scrolling at all atm.

I think the only way to accomplish the goal of keeping the active tab's widget in view on the tab bar is to use (dual) overflow menus; when a tab is selected from one, scroll the tab bar so that the selected tab's widget is in a fixed place (leftmost on the bar?) and make the existing keyboard shortcuts scroll, too, if you want to go through and activate every tab 1-by-1.

In the current impl, you have two ways of moving through tabs; neither one of them keeps the current tab's widget in view.  What we should have is one way that keeps the widget in view and that lets you "move" tab-by-tab (the kybd shortcuts)--which change focus as you move through each new tab--and one way that lets you jump directly to a tab, with only one focus change.

(We also have to think about FKA and tabs, for the bug filed on that; that should probably work like the existing keyboard shortcut, essentially the same behavior with a different keystroke (just the arrow) in FKA.  Moving activates the next tab, just like in the OS tabs.)

Comment 35

[Simon Fraser [no longer active]]

Don't ignore my comment #28 above, either.

Comment 36

[Desmond Elliott]

(In reply to comment #35)
> Don't ignore my comment #28 above, either.

Would you be able to expand on why you are against this, and could you expand on what you mean when you say animate?

Comment 37

[Desmond Elliott]

(In reply to comment #34)

> I think we need to go back and refocus on the problem and reframe our solution.
>  Namely, the issue/problem to solve is to ensure that the active tab's tab
> widget is also visible on the tab bar, and that is the whole point of
> scrolling.

I'm sorry to say that I disagree. If you have 20 tabs open and you are currently viewing tab 1, it is reasonable enough to assume that you might want to scroll to look and see which tabs you have open without actually changing the active tab.

> The current patch doesn't really accomplish that goal; the active tab's widget
> can still be scrolled off-screen (it's a bit disconcerting, actually, to see
> the tab bar move and the active tab/content remain the same, esp. in comparison
> to the current tab-switching behaviors with the shortcuts or overflow), nor
> does changing tabs with the keyboard shortcut ensure that the active tab always
> has a visible widget--the shortcuts don't cause scrolling at all atm.

I agree that I have not thought about the implications of using the keyboard shortcuts yet and this is something that I will need to address. Thanks for bringing this to my attention.

> I think the only way to accomplish the goal of keeping the active tab's widget
> in view on the tab bar is to use (dual) overflow menus; when a tab is selected
> from one, scroll the tab bar so that the selected tab's widget is in a fixed
> place (leftmost on the bar?) and make the existing keyboard shortcuts scroll,
> too, if you want to go through and activate every tab 1-by-1.

I thought we were trying to move away from clunky menus and into something more natural for the user?

> In the current impl, you have two ways of moving through tabs; neither one of
> them keeps the current tab's widget in view.  What we should have is one way
> that keeps the widget in view and that lets you "move" tab-by-tab (the kybd
> shortcuts)--which change focus as you move through each new tab--and one way
> that lets you jump directly to a tab, with only one focus change.
> 
> (We also have to think about FKA and tabs, for the bug filed on that; that
> should probably work like the existing keyboard shortcut, essentially the same
> behavior with a different keystroke (just the arrow) in FKA.  Moving activates
> the next tab, just like in the OS tabs.)
> 

I have no idea what an FKA is, could you explain this to me please?

It seems like the closer I get to achieving my goal the further back I am pushed. I do appreciate the time people take to offer me feedback and I do take all of it on-board, but it seems like people didn't take the time to really read through the implementation sketches I provided [http://summerofcamino.blogspot.com/2006/06/implementation-sketches-complete.html] and quiz me about things then.

Considering all the discussion that is being raised about how this should be implemented, I might need to put this aside for a couple of weeks in order to make sure that what I am coding is actually what people have in mind. I have some reasonably strict deadlines to meet and I'm still learning Cocoa and Objective-C as I go (as I'm sure you can all tell)

Comment 38

[Stuart Morgan]

(In reply to comment #37)
> (In reply to comment #34)
> 
> > I think we need to go back and refocus on the problem and reframe our solution.
> >  Namely, the issue/problem to solve is to ensure that the active tab's tab
> > widget is also visible on the tab bar, and that is the whole point of
> > scrolling.
> 
> I'm sorry to say that I disagree. If you have 20 tabs open and you are
> currently viewing tab 1, it is reasonable enough to assume that you might want
> to scroll to look and see which tabs you have open without actually changing
> the active tab.

With overflow menus, you can see all the tabs you have open (in one direction) in a single click, rather than having to always be looking through a fixed-size window.  Dual-menus give users better ability to get a sense of what they have open.

> I thought we were trying to move away from clunky menus and into something more
> natural for the user?

I don't think it's that the menus aren't natural for the users, it's that things *stay* in the menu, which means any operations that you'd like to perform on the tab widget itself is impossible for the overflow.

Having been out of the country, I hadn't had a chance to see the final sketches, so I apologize for chiming in late. But it seems like the most important things are:
-the ability to access any tab quickly
-the ability to easily get an overview of everything that's open
-the ability to operate on any tab equally
-not confusing the user about where they are

A dual-overflow-menu system that always keeps the current in the visible window is the best system I've seen proposed to meet those goals.

Comment 39

[Mike Pinkerton (not reading bugmail)]

users are used to scrolling and their selection going away. it's how every document-based ui works. i don't see that as a valid argument.

why can't we combine the two approaches? clickhold shows a menu, click scrolls a screen at a time. done. I think scrolling makes a lot of sense.

Comment 40

[Mike Pinkerton (not reading bugmail)]

oh and one more thing...

the other thing to keep in mind here, that i think everyone is forgetting, is that this is a first step to tab reordering. Regardless of what we do for the normal case (which we can still argue about), we need this to scroll when you're drag-ordering tabs.

so let's not block this for poor desmond with too much swirl, he's just trying to get to the holy grail of tab reordering...

Comment 41

[Desmond Elliott]

Attachment: WIP Attempt 3, bug squash galore.

I have beaten this patch for a couple of days now and the re-drawing bugs appear to have vanished. The code also appears to be able to support multiple windows in different states.

The following bugs still exist, that I know of:

1. Newly created tabs that are selected do not cause the tab bar to scroll along to make it the right-most tab.

2. If a tab is currently selected and is to the immediate right/left of the tab bar then there is no divider.

3. The current implementation does not obey the Full Keyboard Access rules that are currently in place.

I have done my best to continue to tidy up my code as I have written it, but there are bound to be some logic errors, or things that can be more succinctly tidied up.

As always, I look forward to reading what Bruce and Nick have to say.

Comment 42

[Stuart Morgan]

Comment on attachment 227544 [details] [diff] [review]
WIP Attempt 3, bug squash galore.

>+  unsigned int mTemporaryNumberOfVisibleTabs;  // The # of visble tabs before now

Does this need to be a member variable? It looks like its use is all local to one method.

>+// This method constructs the tab bar based on the current state of mTabView.
>+// It unregisters tab buttons, releases the active tab, resets the active tab
>+// lays out the tabs again, and re-registers the tabs.
> -(void)rebuildTabBar

This comment reads almost exactly like the method, so it's not really adding clarity, and adds potential for it to get out of sync with the implementation. Method comments should document the purpose/use of a function, not implementation details.

> -(void)layoutButtons
> {
>+  int numTabs = [mTabView numberOfTabViewItems];
>+  float widthOfVisibleTabs;
>+  float widthOfTabBar = NSMaxX([self frame]);

NSWidth, not NSMaxX

>+  float maxWidth = floor((widthOfTabBar - (2 * kTabBarMargin)) / numTabs);
>+  mTemporaryNumberOfVisibleTabs = mNumberOfVisibleTabs;

This assignment doesn't end up doing anything. In fact, the variable can be eliminated entirely, and the setNumberOfVisibleTabs: done in the if/else clauses.

>   if (maxWidth < kMinTabWidth) {
>     mOverflowTabs = YES;
>+    int tabBarWidthLessOverflowMargins = widthOfTabBar - (2 * kTabBarMarginWhenOverflowing);

float, not int

>+    float maxVisibleTabs = floor(tabBarWidthLessOverflowMargins / kMinTabWidth);

int, not float

>+    maxWidth = floor(tabBarWidthLessOverflowMargins / maxVisibleTabs);
>+    mTemporaryNumberOfVisibleTabs = maxVisibleTabs;
>+    [self initOverflow];
>     }
>+  else
>+  {

else {

>     mOverflowTabs = NO;
>+    mTemporaryNumberOfVisibleTabs = numTabs;
>+  }
>+  
>+  [self setNumberOfVisibleTabs:mTemporaryNumberOfVisibleTabs];

As mentioned above, just fold this into the clauses above

>+  // This section of code sets the width of tabs as appropriate
>+  if (kMaxTabWidth > maxWidth) {
>+    widthOfVisibleTabs = (maxWidth > kMinTabWidth ? maxWidth : kMinTabWidth);
>+  } else {
>+    widthOfVisibleTabs = kMaxTabWidth;
>   }

The combination of kMaxTabWidth and maxWidth makes it harder to tell what's going on here. Since it doesn't look like there's any need for both widthOfVisibleTabs and maxWidth, just use one |tabWidth| variable throughout (Visible is unnecessary, since an invisible tab has no width, and the plural makes it sound like it's talking about the sum of the widths of the tabs).

You might also try to rewrite the flow to not use both an if/else and a ternary operator at the same time for essentially the same purpose, since that also impedes legibility.


I'll try to take a closer look at the layout details later, but at a high level it seems like it would be clearer, instead of having one big loop that's checking each time if it's off to one side or another and doing totally different things depending on the answer, to structure it more like:

for tab index 0 to (first visible - 1)
  handle tabs that are offscreen to the left

for tab index (first visible) to (last visible)
  handle visible tabs

for tab index (last visible + 1) to last
  handle tabs that are offscreen to the right


>+  // the tabs are not overflowing then remove the buttons from the tab bar and
>+  // set their pointers to nil.
...
>+      [mOverflowRightButton removeFromSuperview];
>+      mOverflowRightButton = nil;
>+      [mOverflowLeftButton removeFromSuperview];
>+      mOverflowLeftButton = nil;

Why are you nil'ing these out?  Not only does that mean you have to spend time creating new ones the next time there's an overflow, but you are also leaking all of them.

>+-(IBAction)overflowControlLeft:(id)aSender

Action methods should describe what they do, not their source. This should be something like scrollLeft

>+    [self setLeftMostVisibleTabIndex:(mLeftMostVisibleTabIndex - 1)];
>+    [self rebuildTabBar];

If setting the visible tab window requires rebuilding the tab bar, then setLeftMostVisibleTabIndex: should call it as necessary.  Callers shouldn't need to worry about it.

>+-(IBAction)overflowControlRight:(id)aSender

Same here

>+// This is a helper method to try to make sure that the number of visible tabs
>+// and calls to the the left most visible tab helper method are being set
>+// correctly.
>+-(void)setNumberOfVisibleTabs:(int)number
...
>+// This helper method provides a place where the left-most visible tab index
>+// can be set without random ++ or -- tinkering.
>+-(void)setLeftMostVisibleTabIndex:(int)index

The combination of these two methods confuses me.  Both setting the start of the visible window and the size of the visible window require bounds checking to make sure you aren't ending up with something like 10 tabs with 5 visible and tab 10 as the leftmost--and yet all of the bounds checking logic is seemingly arbitrarily in one method, leaving potential to do the wrong thing if you call the other.  I think there needs to be one common function that both call which validates the requested combination of {total,visible,start}, and either denies or fixes up invalid sets. (And presumably changing the number of total tabs needs to call it or something like it as well).


As a general note, there are a bunch of weird tab and spacing issues in both the old and new code; please try to get all the indentation consistent and using spaces.  Also watch for putting if/else brackets on the right line (the same one).

Comment 43

[Desmond Elliott]

Attachment: New left arrow button

I, uhm, ripped these left and right arrows from XCode and manipulated them using The GIMP. We really need proper arrows before this gets to SR.

Comment 44

[Desmond Elliott]

Attachment: New right arrow button

Comment 45

[Desmond Elliott]

Attachment: WIP4

Current Bugs (that I have noticed):

* Double clicking on a disabled scroll button creates a new tab.
* There are some hilarious drawing issues, from time to time, that don't make sense.
* There is probably a lovely memory leak because after I've been playing for a while it takes up to 10 seconds to close Camino.
* Full keyboard access current allows a user to go from tab 0 to tab n -1 back to tab 0 if they continue in that direction, I need to fix this, it is insane.

Features that need to be added:

* Users should be able to double-click to jump a tab-bar width number of tabs in either direction.
* Animated sliding!

Little video of this baby in action http://www.tardis.ed.ac.uk/~delliott/tabScrollingThirdDemo.mov

I hope I have all of my tab spacing problems sorted now.

Also, this is a bit of a re-write from WIP3 so I wouldn't recommend using WIP3 as a reference point.

Comment 46

[Stuart Morgan]

Can you post a new version of this patch with the indentation fixed?  The indentation here is all over the place, and I'm finding it almost impossible to follow any of the logic as a result.

Comment 47

[Desmond Elliott]

Attachment: WIP4a

As requested by smorgan, hopefully fixes the tabs problems. If it does not then please identify the offending lines and I will take a closer look.

Comment 48

[Desmond Elliott]

It did not solve the problem, I tried to manually fix all of the indentation and then the combination of XCode / TextWrangler chewed it up. I will sumbit WIP4b ASAP.

Comment 49

[Desmond Elliott]

Attachment: WIP4b

Please please please work this time. I decided to actually put somebody in the requestee box since I should have been doing that anyway.

Comment 50

[Stuart Morgan]

>+  unsigned int      mIntermediatePreviousNumberOfTabs; // # of tabs prior to now
>+  unsigned int      mPreviousNumberOfTabs;             // Fucking stupid hack

Could you explain why there are two different previous values, and what they actually mean?

>+  unsigned int      mLeftMostVisibleTab;               // # of the left-most visible tab in the tab bar

This is really mLeftMostVisibleTabIndex

>+  mPreviousNumberOfVisibleTabs = floor(mPreviousWidthOfTabBar / mPreviousWidthOfATab);

Since you only use the previous widths to find the previous number of visible tabs, why not just store the previous number of visible tabs directly instead of storing the widths?

>+    NSEnumerator* tabEnumerator = [[mTabView tabViewItems] objectEnumerator];
>+    BrowserTabViewItem* tab = nil;
>+    unsigned int currentActiveTab = 0;
>+    while ((tab = [tabEnumerator nextObject])) {
>+      if ([tab tabState] == NSSelectedTab) {
>+        break;
>+      }
>+      else {
>+        currentActiveTab++;
>+      }
>+    }

Isn't this just giving you [mTabView indexOfTabViewItem:[mTabView selectedTabViewItem]]? (And again, the variable needs to be named ...TabIndex, not ...Tab)

>+    // This section of code attempts to determine whether creating new tabs -
>+    // when the tick box in the tabs prefpane for new tabs and windows to load
>+    // in the background is not ticked - should change the tab bar.

Refer to prefs/behaviors, not specific UI, especially when it's UI in a completely different part of the browser.  I.e., "when creating new foreground tabs", not "...when the tick box in the tab prefpane..."

>+    if ((totalNumberOfTabs > mPreviousNumberOfTabs) && (currentActiveTab != mPreviousActiveTab)) {
>+      mLeftMostVisibleTab = totalNumberOfTabs - mNumberOfVisibleTabs;
>+    }

This should be doing something based on the actual location of the active tab, rather than making assumptions about where new tabs are.

>+    // This section of code attempts to determine whether closing tabs that are
>+    // currenly in the tab bar view warrant any change of state in the tab bar.
>+    // If the user is overflowing and closing a tab would cause the number of
>+    // visible tabs to decrease, then move the left most visible tab 
>+    // appropriately; if closing at tab would not cause the number of visible
>+    // tabs to decrease then move the left most visible tab to keep the active
>+    // tab in view; if the user is not overflowing the tabs then the left most
>+    // visible tab should always be 0.
>+    else if (totalNumberOfTabs < mPreviousNumberOfTabs) {
>+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs > totalNumberOfTabs) {
>+        mLeftMostVisibleTab -= (mPreviousNumberOfTabs - totalNumberOfTabs);
>+      }
>+    }

This code doesn't match the comment; the comment describes two possibilities that should change the tab window, but only one exists in code. I'm also not clear on the intent--how could closing a tab when there's overflow cause fewer tabs to be visible?

>+    // This section of code handles users using FKA when they are scrolled away
>+    // from the currently active tab. If a user has scrolled away from the
>+    // currently active tab and they then use cmd+opt+right then the tab bar
>+    // slides back so that this tab is the right-most visible tab; if they have
>+    // scrolled away to the left then cmd+opt+left slides so that the tab is
>+    // left-most visible.

Again, don't refer to UI that's outside your scope. Your comments shouldn't become incorrect if keyboard shortcuts you don't control change (also, I'm not seeing where FKA comes in here)

>+    else if (currentActiveTab != mPreviousActiveTab) {
>+      // The user has pressed Command-Option-Right
>+      if (currentActiveTab == (mPreviousActiveTab + 1)) {

Wouldn't this also be true if the user clicks on the tab that's one tab to the right?  And why would moving one tab over cause potential scrolling, but moving more than one not?

Unless I'm misunderstanding the intent here, I think there should be a "scrollToTab:" method that is a no-op if the given tab is in the current window, makes the tab the leftmost (when possible) if its off to the left, and makes it rightmost (when possible) if it's off to the right; then all this entire case needs to do is call that method with the currently active tab. (Does it ever actually matter what the previously active tab was?)

>+          ...
>+          else if (currentActiveTab + mNumberOfVisibleTabs <= totalNumberOfTabs) {
>+            mLeftMostVisibleTab = currentActiveTab;
>+          }
>+          else {
>+            mLeftMostVisibleTab = totalNumberOfTabs - mNumberOfVisibleTabs;
>+          }

Code blocks like this come up over and over.  This is why I commented before about the need for a method that takes a desired tab window and does the right thing--there should be setters for the left-most tab, the number of visible tabs, and the total number of tabs, and all should make sure that fixup is done to the left-most tab as necessary; then it doesn't have to be checked at each place you want to change one of those values.

>+      ...
>+    else if (mNumberOfVisibleTabs > mPreviousNumberOfVisibleTabs) {
>+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs > totalNumberOfTabs) {
>+        mLeftMostVisibleTab -= (mNumberOfVisibleTabs - mPreviousNumberOfVisibleTabs);
>+      }
>+    }

This would be taken care of for free by having a setter for mLeftMostVisibleTab

>+    else if (mNumberOfVisibleTabs < mPreviousNumberOfVisibleTabs) {
>+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs < currentActiveTab) {
>+        mLeftMostVisibleTab = currentActiveTab - mNumberOfVisibleTabs;
>+      }
>+    }

What's the intended behavior here?  It looks like making a window smaller will always cause the active tab to scroll into view, which seems unexpected if it wasn't in view before the resize.

>+  unsigned int currentBrowserTabViewItem = 0; // Used to make sure the Enumerator starts at the right place.

...TabIndex, not ...TabViewItem.


>   while ((tab = [tabEnumerator nextObject])) {

Since the indexes are fundamentally important, there's no reason to use an iterator here, and it leads to lots of index testing against values that are known in advance.  Again, make this three for-loops (one for the left overflow, one for the middle, and one for the right overflow), and use tabViewItemAtIndex: to get the tabs. That will also eliminate things like tracking the previous tab button, since the edge cases for divider drawing are also at known values.

>     NSSize buttonSize = [tabButtonCell size];
>+    buttonSize.width = widthOfATab;
>     buttonSize.height = kTabBarDefaultHeight;

This is just NSSize buttonSize = NSMakeSize(widthOfATab, kTabBarDefaultHeight), and can be done outside the loop since the values are fixed.

>+  // If the tabs are overflowing then add these 2 buttons to the tab bar. If
>+  // the tabs are not overflowing then remove the buttons from the tab bar.
>   if (mOverflowTabs) {

The indentation inside most of this clause is off

>+  if (mLeftMostVisibleTab == 0) {
>+    [mOverflowLeftButton setEnabled:NO];
>+  }
>+  else {
>+    [mOverflowLeftButton setEnabled:YES];
>+  }

This is another thing that should be taken care of automatically anytime a setter for tab count, visible tab count, or left-most visible tab is called.


> -(NSRect)tabsRect
> {
>+  NSRect rect = [self frame];
>+  if (mOverflowTabs) {
>+    rect.origin.x += kTabBarMarginWhenMOverflowTabs;
>+    rect.size.width -= 2 * kTabBarMarginWhenMOverflowTabs;
>+    return rect;
>+  }
>+  else {
>+    rect.origin.x += kTabBarMargin;
>+    rect.size.width -= 2 * kTabBarMargin;
>+    return rect;
>+  }
> }

Move the return outside

>+// If the left or right overflow buttons do not exist, create them.
>+// The right button is disabled if the right-most tab is equal to the total #
>+// of tabs; the left button is disabled if the left-most tab is equal to 0.

Those last two lines aren't part of this method, so don't belong is the comment for it.

>+-(void)initOverflow
>+{
>+  if (!mOverflowRightButton) {
>+    ...
>+    mOverflowLeftButton = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, kOverflowButtonWidth, kOverflowButtonHeight)];

mOverflowLeftButton creation should be conditionalized on !mOverflowLeftButton, not !mOverflowRightButton

Comment 51

[Desmond Elliott]

(In reply to comment #50)

If I have not commented on something then it is because I agree with you.

> >+    if ((totalNumberOfTabs > mPreviousNumberOfTabs) && (currentActiveTab != mPreviousActiveTab)) {
> >+      mLeftMostVisibleTab = totalNumberOfTabs - mNumberOfVisibleTabs;
> >+    }
> 
> This should be doing something based on the actual location of the active tab,
> rather than making assumptions about where new tabs are.

If we have more tabs now than we did before and our currently active tab has changed then we have created a new tab and so to make this new tab right-most visible the above assignment makes sense.

> 
> >+    // This section of code attempts to determine whether closing tabs that are
> >+    // currenly in the tab bar view warrant any change of state in the tab bar.
> >+    // If the user is overflowing and closing a tab would cause the number of
> >+    // visible tabs to decrease, then move the left most visible tab 
> >+    // appropriately; if closing at tab would not cause the number of visible
> >+    // tabs to decrease then move the left most visible tab to keep the active
> >+    // tab in view; if the user is not overflowing the tabs then the left most
> >+    // visible tab should always be 0.
> >+    else if (totalNumberOfTabs < mPreviousNumberOfTabs) {
> >+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs > totalNumberOfTabs) {
> >+        mLeftMostVisibleTab -= (mPreviousNumberOfTabs - totalNumberOfTabs);
> >+      }
> >+    }
> 
> This code doesn't match the comment; the comment describes two possibilities
> that should change the tab window, but only one exists in code. I'm also not
> clear on the intent--how could closing a tab when there's overflow cause fewer
> tabs to be visible?

If a user has scrolled to the right most tab then they are viewing tabs n-1 to (n-1) - #visibleTabs. If they close any on of these tabs then they code needs to shift the left most visible tab to the left so that the correct number of visible tabs are present again.

> >+    // This section of code handles users using FKA when they are scrolled away
> >+    // from the currently active tab. If a user has scrolled away from the
> >+    // currently active tab and they then use cmd+opt+right then the tab bar
> >+    // slides back so that this tab is the right-most visible tab; if they have
> >+    // scrolled away to the left then cmd+opt+left slides so that the tab is
> >+    // left-most visible.
> 
> Again, don't refer to UI that's outside your scope. Your comments shouldn't
> become incorrect if keyboard shortcuts you don't control change (also, I'm not
> seeing where FKA comes in here)

What do you mean by you don't see where FKA comes in here?

> 
> >+    else if (currentActiveTab != mPreviousActiveTab) {
> >+      // The user has pressed Command-Option-Right
> >+      if (currentActiveTab == (mPreviousActiveTab + 1)) {
> 
> Wouldn't this also be true if the user clicks on the tab that's one tab to the
> right? 

Yes, but these if else clauses have been crafted to not do anything if a user changes the current tab index and it is in view in the tab bar.

 And why would moving one tab over cause potential scrolling, but moving
> more than one not?

Because the currently active tab might not be in view.

> Unless I'm misunderstanding the intent here, I think there should be a
> "scrollToTab:" method that is a no-op if the given tab is in the current
> window, makes the tab the leftmost (when possible) if its off to the left, and
> makes it rightmost (when possible) if it's off to the right; then all this
> entire case needs to do is call that method with the currently active tab.

I was more concerned with trying to get the logic down than making things nice and tidy.

> (Does it ever actually matter what the previously active tab was?)

Yes.

> >+          ...
> >+          else if (currentActiveTab + mNumberOfVisibleTabs <= totalNumberOfTabs) {
> >+            mLeftMostVisibleTab = currentActiveTab;
> >+          }
> >+          else {
> >+            mLeftMostVisibleTab = totalNumberOfTabs - mNumberOfVisibleTabs;
> >+          }
> 
> Code blocks like this come up over and over.  This is why I commented before
> about the need for a method that takes a desired tab window and does the right
> thing--there should be setters for the left-most tab, the number of visible
> tabs, and the total number of tabs, and all should make sure that fixup is done
> to the left-most tab as necessary; then it doesn't have to be checked at each
> place you want to change one of those values.

Setters can be introduced when people agree that logically the right things are happening.

> 
> >+      ...
> >+    else if (mNumberOfVisibleTabs > mPreviousNumberOfVisibleTabs) {
> >+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs > totalNumberOfTabs) {
> >+        mLeftMostVisibleTab -= (mNumberOfVisibleTabs - mPreviousNumberOfVisibleTabs);
> >+      }
> >+    }
> 
> This would be taken care of for free by having a setter for mLeftMostVisibleTab
> 
> >+    else if (mNumberOfVisibleTabs < mPreviousNumberOfVisibleTabs) {
> >+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs < currentActiveTab) {
> >+        mLeftMostVisibleTab = currentActiveTab - mNumberOfVisibleTabs;
> >+      }
> >+    }
> 
> What's the intended behavior here?  It looks like making a window smaller will
> always cause the active tab to scroll into view, which seems unexpected if it
> wasn't in view before the resize.

This is a good point, I shall take this code out to reflect your feedback.

> 
> >+  unsigned int currentBrowserTabViewItem = 0; // Used to make sure the Enumerator starts at the right place.
> 
> ...TabIndex, not ...TabViewItem.
> 
> 
> >   while ((tab = [tabEnumerator nextObject])) {
> 
> Since the indexes are fundamentally important, there's no reason to use an
> iterator here, and it leads to lots of index testing against values that are
> known in advance.  Again, make this three for-loops (one for the left overflow,
> one for the middle, and one for the right overflow), and use
> tabViewItemAtIndex: to get the tabs. That will also eliminate things like
> tracking the previous tab button, since the edge cases for divider drawing are
> also at known values.

This has been quite a chore to program, but I think I have transcribed it to three for loops although the code is pretty complex and might require closer inspection to make sure it is doing things sensibly.

> The indentation inside most of this clause is off

Should be fixed now.

Thanks for the review Simon, WIP4bi posted as a result.

Comment 52

[Desmond Elliott]

Attachment: WIP4bi

Iterated version of WIP4b as a result of smorgan's review. Sections marked *** blah *** are of particular importance.

Comment 53

[Stuart Morgan]

(In reply to comment #51)
> > >+    if ((totalNumberOfTabs > mPreviousNumberOfTabs) && (currentActiveTab != mPreviousActiveTab)) {
> > >+      mLeftMostVisibleTab = totalNumberOfTabs - mNumberOfVisibleTabs;
> > >+    }
> > 
> > This should be doing something based on the actual location of the active tab,
> > rather than making assumptions about where new tabs are.
> 
> If we have more tabs now than we did before and our currently active tab has
> changed then we have created a new tab and so to make this new tab right-most
> visible the above assignment makes sense.

I agree that making the newly active tab visible makes sense; the point is that that's not what the code does.  The code makes the absolute right-most tabs visible, and that's not the same thing (opening a tab group in new tabs, for example).

(The other issue is that every case where the active tab may not be visible anymore shouldn't be a special-case with its own clause; that's why I think there needs to be a scrollTo.)

> > 
> > >+    // This section of code attempts to determine whether closing tabs that are
> > >+    // currenly in the tab bar view warrant any change of state in the tab bar.
> > >+    // If the user is overflowing and closing a tab would cause the number of
> > >+    // visible tabs to decrease, then move the left most visible tab 
> > >+    // appropriately; if closing at tab would not cause the number of visible
> > >+    // tabs to decrease then move the left most visible tab to keep the active
> > >+    // tab in view; if the user is not overflowing the tabs then the left most
> > >+    // visible tab should always be 0.
> > >+    else if (totalNumberOfTabs < mPreviousNumberOfTabs) {
> > >+      if (mLeftMostVisibleTab + mNumberOfVisibleTabs > totalNumberOfTabs) {
> > >+        mLeftMostVisibleTab -= (mPreviousNumberOfTabs - totalNumberOfTabs);
> > >+      }
> > >+    }
> > 
> > This code doesn't match the comment; the comment describes two possibilities
> > that should change the tab window, but only one exists in code. I'm also not
> > clear on the intent--how could closing a tab when there's overflow cause fewer
> > tabs to be visible?
> 
> If a user has scrolled to the right most tab then they are viewing tabs n-1 to
> (n-1) - #visibleTabs. If they close any on of these tabs then they code needs
> to shift the left most visible tab to the left so that the correct number of
> visible tabs are present again.

Okay, perhaps this comment could be adjusted to not overload the term "visible tabs", as that's what confused me (although again I think this should be handled automatically in setters whenever the number of tabs is updated to prevent mistakes).

> > Again, don't refer to UI that's outside your scope. Your comments shouldn't
> > become incorrect if keyboard shortcuts you don't control change (also, I'm not
> > seeing where FKA comes in here)
> 
> What do you mean by you don't see where FKA comes in here?

There are shortcuts for changing tabs; FKA isn't necessary for scrolling away from a previous tabs with the keyboard.

>  And why would moving one tab over cause potential scrolling, but moving
> > more than one not?
> 
> Because the currently active tab might not be in view.

What if someone adds a keyboard shortcut later to scroll a full tab window left or right?  The way it's handled would be the same, so there's no need to introduce a magic number here (1) rather than leaving it generic.

> > Unless I'm misunderstanding the intent here, I think there should be a
> > "scrollToTab:" method that is a no-op if the given tab is in the current
> > window, makes the tab the leftmost (when possible) if its off to the left, and
> > makes it rightmost (when possible) if it's off to the right; then all this
> > entire case needs to do is call that method with the currently active tab.
> 
> I was more concerned with trying to get the logic down than making things nice
> and tidy.

I think having a generic function which can be relied on to do the right thing rather than having a special case for each thing as it comes up is a critical part of the logic, not just about tidiness.

> Setters can be introduced when people agree that logically the right things are
> happening.

Again, having code setters is a fundamental part of having the correct logic; handling it in code scattered around is error prone. Logically, the right thing here is to always enforce a combination of the left-tab/visible-tabs/total-tabs combination, and the only correct way to do that is to do it as part of changing any of them.

> > What's the intended behavior here?  It looks like making a window smaller will
> > always cause the active tab to scroll into view, which seems unexpected if it
> > wasn't in view before the resize.
> 
> This is a good point, I shall take this code out to reflect your feedback.

Just taking it out means that resizing the window with the active tab in view could make it fall out of view though, which also isn't desirable.  You may need to start tracking whether the active tab was visible before a re-layout, and use that to make decisions about whether to enforce its visibility.

Comment 54

[Stuart Morgan]

I see now why the current logic needs to track the previous tab, but I think there's a better way to get the right information.  Fundamentally what you need to know is
1) whether the active tab is visible (which you always know, trivially), and
2) whether the user would expect the active tab to be made visible as a result of their action
As I mentioned above, there are already cases like resizing where just knowing if the active tab changed isn't enough.  What about instead storing mActiveTabShouldBeVisible, which is set to false only if the user deliberately scrolls the active tab out of view, and reset to true any time the active tab changes or the active tab is scrolled back into view?

Comment 55

[Stuart Morgan]

Comment on attachment 229458 [details] [diff] [review]
WIP4bi

Looking at just the new loops:

>+  if (!mOverflowTabs) {
>     ...
>+  }
>+  else {

This whole if/else can go away.  In the case where there's no overflow, the 1st and 3rd loops just won't ever be entered due to the indexes, and the 2nd loop case is identical to this.

>+    for (i; i < mLeftMostVisibleTabIndex; i++) {
>+      tab = [mTabView tabViewItemAtIndex:i];
>+      tabTabButtonCell = [tab tabButtonCell];
>+      NSSize buttonSize = NSMakeSize(widthOfATab, kTabBarDefaultHeight);

buttonSize should be set once before of the loops

>+      NSPoint buttonOrigin = NSMakePoint(xCoord,0);
>+      [tabTabButtonCell hideCloseButton];
>+      [tabTabButtonCell setFrame:NSMakeRect(buttonOrigin.x, buttonOrigin.y, 0, buttonSize.height)];

Eliminate buttonOrigin entirely, since all you need is NSMakeRect(0, 0, 0, buttonSize.height) (although there's no real need for it to have height, either). Also, just make the rect once outside the loop.

>+      if ([tab tabState] == NSSelectedTab) {
>+        mPreviousActiveTabIndex = i;
>+      }

No need to do this check in all the loops--just query for the active tab index directly after all the loops.

>+    }
>     ...
+      NSPoint buttonOrigin = NSMakePoint(xCoord,0);
+      [tabTabButtonCell setFrame:NSMakeRect(buttonOrigin.x, buttonOrigin.y, buttonSize.width, buttonSize.height)];

You don't need button origin here either; just use xCoord and 0 directly in the NSMakeRect

>+      if ([tab tabState] == NSSelectedTab) {
>+        [tabTabButtonCell setDrawDivider:NO];
>+        [previousTabTabButtonCell setDrawDivider:NO];
>+        mPreviousActiveTabIndex = i;
>+      }
>+      else {
>+        [tabTabButtonCell setDrawDivider:YES];
>+      }

Again, just do this as a one-step fixup after the loop, rather than testing inside the loop (just assume YES for everything while in the loop).

>       ...
>+      NSPoint buttonOrigin = NSMakePoint(xCoord,0);
+      [tabTabButtonCell hideCloseButton];
>+      [tabTabButtonCell setFrame:NSMakeRect(NSMaxX([self frame]), buttonOrigin.y, 0, buttonSize.height)];

Eliminate buttonOrigin here too and use 0 directly in the setFrame:.  Again, make the rect once before this loop (so you don't have to keep calling [self frame])

Comment 56

[Håkan Waara]

Comment on attachment 229458 [details] [diff] [review]
WIP4bi

layoutTabs: is _huge_. Would it be possible to break it down, so some of the logic is factorized out in helper methods? It would make it easier to follow, I think.

By the way, instance vars never need to be initialized to nil/0, since they're guaranteed to be zeroed when the instance is created, afaik.

Comment 57

[Stuart Morgan]

(In reply to comment #56)
> (From update of attachment 229458 [details] [diff] [review] [edit])
> layoutTabs: is _huge_. Would it be possible to break it down, so some of the
> logic is factorized out in helper methods?

Once the things I've mentioned than need to be handled elsewhere (the setters, and scrollToTab:) are moved, it will be substantially shorter.

Comment 58

[Desmond Elliott]

(In reply to comment #57)
> Once the things I've mentioned than need to be handled elsewhere (the setters,
> and scrollToTab:) are moved, it will be substantially shorter.

Stuart,

Can you please expand a bit on this scrollToTab: method so I have a clearer picture of what you are imagining?

Comment 59

[Desmond Elliott]

Attachment: WIP4bii (no real changes, just a proposition to smorgan)

Stuart,

I have added in a method configureTabs:currentActiveTabIndex:totalNumberOfTabs which is called in |layoutTabs| that removes all the ugly attempts at logic into a helper method. Is this what you had in mind?

I don't want to re-design things without making sure that this is what you intended.

Comment 60

[Stuart Morgan]

No, this just moves all the special-case-based logic, rather than eliminating most of it.  The structural changes the last version needs are:

1) Setters (preventing the possibility of ever ending up with an invalid pair of visiblecount/leftmostindex for the actual number of tabs):
setLeftMostVisibleTabIndex:, which takes a desired left index, and sets the member variable to the closest thing that's actually possible.

and

setVisibleTabCount:, which sets the member variable, then adjusts the leftmost tab index if necessary.

(I've been looking more at the overall interaction of the rest of the browser with the tab bar, and a setter for the actual number of tabs turns out not to make sense)

2) scrollTabToVisible:, which figures out where the given tab is relative to the visible tabs, and adjusts what is visible accordingly.

3) A variable tracking whether or not the tab bar is in a state where the active tab should have its visibility enforced.  This would be set to YES any time the active tab changes, and NO any time the user explicitly moves the active tab out of view (using the left or right scroller arrows, debatably when the window shrinks, anywhere else it makes sense).


The combination of 2 and 3 should eliminate the majority of the logic in layoutTabs, and replace it with something like
if (mShouldKeepActiveTabVisible)
  [self scrollTabToVisible:[mTabView selectedTabViewItem]]

Comment 61

[Desmond Elliott]

(In reply to comment #60)
> No, this just moves all the special-case-based logic, rather than eliminating
> most of it.  The structural changes the last version needs are:
> 
> 1) Setters (preventing the possibility of ever ending up with an invalid pair
> of visiblecount/leftmostindex for the actual number of tabs):

Are you proposing that the setters check whether or not the pair of visiblecount/leftmostindex are valid?

> setLeftMostVisibleTabIndex:, which takes a desired left index, and sets the
> member variable to the closest thing that's actually possible.
> 
> and
> 
> setVisibleTabCount:, which sets the member variable, then adjusts the leftmost
> tab index if necessary.

It seems from these comments that you might be, but I wanted to check.

> (I've been looking more at the overall interaction of the rest of the browser
> with the tab bar, and a setter for the actual number of tabs turns out not to
> make sense)
> 
> 2) scrollTabToVisible:, which figures out where the given tab is relative to
> the visible tabs, and adjusts what is visible accordingly.

Ok, that should be alright.
 
> 3) A variable tracking whether or not the tab bar is in a state where the
> active tab should have its visibility enforced.  This would be set to YES any
> time the active tab changes, and NO any time the user explicitly moves the
> active tab out of view (using the left or right scroller arrows, debatably when
> the window shrinks, anywhere else it makes sense).

Ok, I can see why this would also be useful and shouldn't be too difficult to do.
 
> The combination of 2 and 3 should eliminate the majority of the logic in
> layoutTabs, and replace it with something like
> if (mShouldKeepActiveTabVisible)
>   [self scrollTabToVisible:[mTabView selectedTabViewItem]]

Alright, I will see what I can do.

Comment 62

[Stuart Morgan]

(In reply to comment #61)
> Are you proposing that the setters check whether or not the pair of
> visiblecount/leftmostindex are valid?

Check as well as enforce, by adjusting the left-most tab index whenever necessary.

Comment 63

[Desmond Elliott]

Things feel like they are starting to get closer to R+ in the near future. Are there any *serious* concerns about the direction this patch is going in?

Stepping back and looking through discussion prior to SoC it seems like there was no consensus as to how this should be approached, just lots of ideas. Let me recap how things are and how they might end up being:

Past: When a user opens more tabs than can fit in the tab bar and maintain the minimum width of a tab (100 pixels I believe) then a chevron appears at the far right of the tab bar and clicking on it results in a pop up menu that allows a user to choose tabs that have overflown.

Current: When a user opens more tabs than can fit in the tab bar and maintain the minimum width of a tab then 2 arrows appear at either side of the tab bar. Clicking on these arrows allows the user to change the tabs that are visible in the tab bar.

Comment 64

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Without (hopefully) causing acrimony, I'd like to be sure there's some (flex)ability in the framework for this to "scale" well with usage demands (which is something that Camino has always done well, IMO).

Click to scroll, click-and-hold to continually scroll, and double-click to scroll a window-full of tabs covers the basic cases well, but is there anything built in to the framework of these patches to ease the problems of heavy tab users?

While we can all laugh at Sam and timeless and their hundreds of tabs, they're real people who dogfood Camino as they earn their living on a daily basis.  Before I stepped back, I would regularly have 30-40 tabs open in a window which held 8 (and when I'm researching something, 20 or more is not unusual).  Living in disembodied tabs sucks, but the redeeming factor of overflow was that I could always end up exactly at the tab I wanted in 2 clicks.

(Maybe double-click to scroll windows-full will prove OK here; it's hard for me to tell without playing with things, and I haven't been able to get any of these WIP4-series patches to apply to a clean trunk :-( )

Comment 65

[Desmond Elliott]

(In reply to comment #64)
> Without (hopefully) causing acrimony, I'd like to be sure there's some
> (flex)ability in the framework for this to "scale" well with usage demands
> (which is something that Camino has always done well, IMO).

What would you consider an ability to scale?

> Click to scroll, click-and-hold to continually scroll, and double-click to
> scroll a window-full of tabs covers the basic cases well, but is there anything
> built in to the framework of these patches to ease the problems of heavy tab
> users?

At the moment there is nothing, I have considered writing in control-click NSMenus at either side so that for people who use an massive number of tabs they can get back their NSMenu access.
 
> While we can all laugh at Sam and timeless and their hundreds of tabs, they're
> real people who dogfood Camino as they earn their living on a daily basis. 
> Before I stepped back, I would regularly have 30-40 tabs open in a window which
> held 8 (and when I'm researching something, 20 or more is not unusual).  Living
> in disembodied tabs sucks, but the redeeming factor of overflow was that I
> could always end up exactly at the tab I wanted in 2 clicks.

Yes, Sam's use case has always concerned me. That is why programming back in NSMenu's that also change the visible tabs in the tab bar might be a nice addition.
 
> (Maybe double-click to scroll windows-full will prove OK here; it's hard for me
> to tell without playing with things, and I haven't been able to get any of
> these WIP4-series patches to apply to a clean trunk :-( )

I am flabbergasted as to why this is. I am pulling from trunk to see where the problem might lie.

Comment 66

[Mike Pinkerton (not reading bugmail)]

i think doing 2 things to address the "scalability" are in order, though they don't necessarily block this patch

1) scrolling by window width as the default. this may require animation to feel right, but i really think it's the right way to go.

2) provide context-click on left/right buttons to see the tabs in each direction, similar to what we have today. 

I don't think these two things impact usability at all for the normal case, and would help things a lot as they scale. Again, I think both of these should be addressed after the inital tab patch lands on the trunk and we can clean up from there in parallel. 

Comment 67

[Simon Fraser [no longer active]]

(In reply to comment #66)
> i think doing 2 things to address the "scalability" are in order, though they
> don't necessarily block this patch
> 
> 1) scrolling by window width as the default. this may require animation to feel
> right, but i really think it's the right way to go.

Are you suggesting that a single click on the arrow moves scrolls by N (or N-1) tabs, where N is the number of visible tabs? I this this would be very disorientating.

A problem with the proposed design is that once you start overflowing the visible tabs, you have no indication of where the visible tabs are in the set of all tabs. Context menus on the arrows (which I think would be essential, barring other design changes) give some context ("Oh, there are 3 tabs fallen off the left, and 5 off the right" -- that took 2 clicks to find out), but I think an always-visible affordance to show the user where they are in the list of tabs is needed.

Comment 68

[Desmond Elliott]

(In reply to comment #67)
> ... I think an always-visible affordance to show the user where they are in the list
> of tabs is needed.
> 

What would you propose a solution to this problem? A progress bar would be clunky, I agree that we need something but I am not sure what.

Comment 69

[Mike Pinkerton (not reading bugmail)]

dashboard solves this with a "page n of m" tooltip on the arrow.

why is scrolling by N(-1) any different than scrolling by 1, especially with animation to make it obvious? You still have no  idea "where you are" when you scroll by 1.

or is this simply an objection to the feature in general?

Comment 70

[Desmond Elliott]

(In reply to comment #69)
> dashboard solves this with a "page n of m" tooltip on the arrow.

Do you propose that I move the right click button to the left a little bit and then at the far right there is a text field showing "11 - 17 / 32" to indicate that that tabs 11 - 17 are in the tab bar out of 32 tabs in total? We could also use this to allow people to type in a number to let them jump to a certain tab... just throwing the idea out there.

Comment 71

[Simon Fraser [no longer active]]

(In reply to comment #69)
> dashboard solves this with a "page n of m" tooltip on the arrow.
> 
> why is scrolling by N(-1) any different than scrolling by 1, especially with
> animation to make it obvious? You still have no  idea "where you are" when you
> scroll by 1.

You certainly have a better idea. Rather than all but one of the visible tabs changing, only one does. The tabs move by {tabWidth} pixels. Don't forget that if you have a bunch of tabs open on the same site, it will be very hard to distinguish them. Move the tabs, and you've lost the user.

> or is this simply an objection to the feature in general?

I just think this is going to be hard to pull off well.

As for how to show context, I don't have any bright ideas. Maybe littls stacks of tabs on either side, with the correct number in each? Or render the tabs as if they are on the periphery of a giant wheel? Gotta think outside the box...

Comment 72

[Stuart Morgan]

(In reply to comment #67)
> I think an always-visible affordance to show the user where they are in 
> the list of tabs is needed.

I agree that would be helpful, but again that can be something that's explored/done in a follow-up to this patch (like the overflow menus, which are definitely going to be needed for this not to be a regression in the many-tab case).  The problem of having no idea where you are in the list of tabs is not new--right now you get 0 context for any but the left-most tabs. With scrolling tabs, you'll get immediate context where ever you are, and full context if you are at the left or right. That's a substantial improvement, even if it's not perfect yet.

One small thing I think might help users orient themselves is to keep the active tab at least one tab from the edge of the visible area whenever possible; that would make it very clear at a glance when you are at an end.

Comment 73

[Desmond Elliott]

(In reply to comment #72)
> One small thing I think might help users orient themselves is to keep the
> active tab at least one tab from the edge of the visible area whenever
> possible; that would make it very clear at a glance when you are at an end.

I might not be understand what you mean by this, so please let me know if I am. I would have thought that the arrows exhibiting a state would be sufficient for a user to see, at a glance, when they are at either end of their set of tabs.

A new patch will be coming by the end of the day.

Comment 74

[Stuart Morgan]

(In reply to comment #73)
> I would have thought that the arrows exhibiting a state would be sufficient for
> a user to see, at a glance, when they are at either end of their set of tabs.

It's much easier to see the difference between having an active tab at an edge vs. one from an edge than the difference between a small icon being grey vs. black.  Besides, having a one-tab buffer means that users can see at least one tab in either direction, which gives a little bit more context about what's around them when not at an edge.  (I would in fact almost argue for having the active tab stay more or less centered whenever possible, but I'd have to actually try that out to see if it was too confusing.)

It will be trivial to adjust that behavior later though, so you can leave out the buffer if you don't agree with it, and people can try out some other variations later and propose any follow-up changes in new bugs.

Comment 75

[Desmond Elliott]

Apple are yet again using the slide by full width feature here, rather interestingly they have two little buttons at the top that appear to denote which 'page' you are on. What are thoughts on this?

http://www.apple.com/getamac/ads/

Comment 76

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, I find that mighty confusing, and those dots do absolutely nothing to help....

Comment 77

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 346141 has been marked as a duplicate of this bug. ***

Comment 78

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FYI, and Ben didn't mention it when he filed his dupe, but the Fx bug where they're thrashing these issues out is bug 343251 (jump to bug 343251 comment 180 for the summary); see also http://wiki.mozilla.org/Tabbed_Browsing/User_Interface_Design/Tab_Overflow

No jumping all over poor Desmond!  (I think it sounds like we're mostly in the right place, actually, especially as a Mac app.)

Comment 79

[Desmond Elliott]

Attachment: WIP5

As a result of my stupidity last week, and some time away on vacation, I managed to look at this problem for a different angle and even though the code is probably still ugly, I think that the way that I am trying to do things now is a move in the right direction.

smorgan: I have taken onboard (almost?) everything that you recommended.

Comment 80

[Stuart Morgan]

Comment on attachment 232107 [details] [diff] [review]
WIP5

>+  BOOL              mShouldKeepActiveTabVisible; // Should the active tab stay visible when resizing?

This is isn't necessarily just about resizing; it's any time the tab bar is rebuilt.

######

>+-(void)scrollToTab:(int)tabIndex;
>+-(BOOL)overflowTabs;
>+-(int)numberOfVisibleTabs; 
>+-(int)leftMostVisibleTabIndex;

I don't think you'll end up needing any of these methods. Most or all of them are symptoms of having moved detailed logic up into classes that shouldn't need to know anything about tab bar implementation.

>+-(void)layoutButtons;

The tabs are just as much buttons as the scrollers, so call this layoutScrollButtons

>+// Calculates the width that tab widgets should be and lays them out in the tab bar
>+-(void)layoutTabs
> {
>+  unsigned int numberOfTabs = [mTabView numberOfTabViewItems];
>+  float widthOfTabBar = NSWidth([self tabsRect]);

You need to set mOverflowTabs to NO before calling this, because what you want to know is if you can fit the tabs in the overall width.

>+  float widthOfATab = floor(widthOfTabBar / numberOfTabs);
>+  unsigned int xCoord;
>+  
>+  if (widthOfATab < kMinTabWidth) {
>     mOverflowTabs = YES;

This requires updating widthOfTabBar, since the frame is now smaller

>+    mNumberOfVisibleTabs = floor(widthOfTabBar / kMinTabWidth);
>+    widthOfATab = floor(widthOfTabBar / mNumberOfVisibleTabs);
>+    xCoord = kTabBarMarginWhenOverflowTabs;
>+    [self initOverflow];

...

>+  BrowserTabViewItem* tab = nil;
>+  TabButtonCell* tabTabButtonCell = nil;

You don't need these outside of the scope of the loop, so just declare them as you need them (in fact, you won't need |tab| as an intermediate variable at all when you remove the selection checking in the loop).  Also, tabTabButtonCell -> tabCell.

>+  TabButtonCell* previousTabTabButtonCell = nil;

Eliminate this. As mentioned earlier, you can do what you need to with the selected cell and its neighbor outside of the loop.

>+  NSSize buttonSize = NSMakeSize(widthOfATab, kTabBarDefaultHeight);
>+  NSRect tabTabButtonCellRect = NSMakeRect(xCoord, 0, 0, 0);

Call this something like overflowTabRect, since that's what it is

...
>+  [self layoutButtons];
>   [self setNeedsDisplay:YES];
> }

What happened to enforcing mShouldKeepActiveTabVisible here?  You track the value, but never use it.


>+    [mOverflowLeftButton setFrame:NSMakeRect(0, ([self tabBarHeight] - kOverflowButtonHeight) / 2,
>+                                             kOverflowButtonWidth, kOverflowButtonHeight)];
...
>+    [mOverflowRightButton setFrame:NSMakeRect((NSWidth([self frame]) - kTabBarMarginWhenOverflowTabs), 
>+                                              ([self tabBarHeight] - kOverflowButtonHeight) / 2, 
>+                                              kOverflowButtonWidth, kOverflowButtonHeight)];


Since these are largely the same, just make the rect once and then update the origin.

>+    if (mLeftMostVisibleTabIndex == 0) {
>+      [mOverflowLeftButton setEnabled:NO];
>+    }
>+    else {
>+      [mOverflowLeftButton setEnabled:YES];
>+    }
...
>+    if (mLeftMostVisibleTabIndex + mNumberOfVisibleTabs == [mTabView numberOfTabViewItems]) {
>+      [mOverflowRightButton setEnabled:NO];
>+    }
>+    else {
>+      [mOverflowRightButton setEnabled:YES];
>+    }


No brackets on single-line if/else (or just use ternaries (?:)).

>+// The current images representing buttons are crude to say the least and need
>+// Jasper Hausen to produce some nicer buttons.

This isn't a code issue; don't put a comment about it.

>+// Programmatically designed.

What does this mean?

>+-(IBAction)scrollRight:(id)aSender
>+{
>+  if ((mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) < [mTabView numberOfTabViewItems]) {
>+    [self scrollToTab:(mLeftMostVisibleTabIndex + 1)];
>+  }
> }

Don't implement scrollRight/scrollLeft in terms of scrollToTab, since they have well-defined meanings in terms of changing the left-most tab index, where as scrollToTab is (should be) a looser concept.

>+// Called when a user creates a new tab, removes a tab, scrolls, or resizes.

Document the purpose; the documentation shouldn't be fragile to changing the details of when mShouldKeepActiveTabVisible is set.

>+-(void)scrollToTab:(int)tabIndex

scrollTabToVisible: (to make the functional parity with similar AppKit functions clear).  However, that's not what this function actually is; this is a setter for mLeftMostVisibleTabIndex.  You should have both (see previous comments for the behavior that scrollTabToVisible: should have).

>+  if (mLeftMostVisibleTabIndex < 0) {
>+    mLeftMostVisibleTabIndex = 0;
>+  }

Bounds-checking needs to happen at both the low and the high ends (and this should be checking the input value, not mLeftMostVisibleTabIndex).

>+  else if (tabIndex > mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) {
>+    [self rebuildTabBar];
>+    mLeftMostVisibleTabIndex = tabIndex - mNumberOfVisibleTabs;
>+  }

Don't rebuild the tab bar twice for one action.

>+  else if (mLeftMostVisibleTabIndex + mNumberOfVisibleTabs > tabIndex + mNumberOfVisibleTabs) {
>+    mLeftMostVisibleTabIndex = tabIndex;
>+  }

Remove mNumberOfVisibleTabs from both sides, and reverse the order.

>+  [self rebuildTabBar];

Don't rebuild the tab bar in every case, since not all calls to this method would actually change the leftmost tab.

>+// Returns the tab bar view to manipulate it
>+-(BrowserTabBarView*)browserTabBarView;

Nobody outside should have the ability to directly manipulate the browserTabBarView.

>- *   Matt Judy 	<[e-mail]> 	(Original Author)
>- *   David Haas 	<[e-mail]>
>+ *   Matt Judy  <[e-mail]>  (Original Author)
>+ *   David Haas   <[e-mail]>


When replacing tabs with spaces, keep aligned things aligned correctly; there are a few of instances of this in the patch.

>+  // Needed to check whether or not the user is loading new tabs in the background.
>+  BOOL loadTabsInBackgroundPref = [[PreferenceManager sharedInstance] 
>+                                   getBooleanPref:"browser.tabs.loadInBackground" 
>+                                   withSuccess:NULL];
>+  if (!loadTabsInBackgroundPref) {
>+    if ([mTabBar overflowTabs]) {
>+      [mTabBar setShouldKeepActiveTabVisible:YES];
>+      [mTabBar scrollToTab:([self numberOfTabViewItems])];
>+    }
>+  }

setShouldKeepActiveTabVisible:YES should be called whenever a new tab is selected, so this is way too high-level to be doing this.  tabWillChange: is probably where it should be.

> - (void)removeTabViewItem:(NSTabViewItem *)tabViewItem
...
>+  // If the index of the tab being removed is visible and there are more tabs to
>+  // the right then bring those into the tab bar first.


None of the code here belongs at this level. This should be free behavior anyway, since the left-most tab won't be changed.

>+// Handles the effect on the number of visible tabs in the tab bar due to resizing.
>+// The values hardcoded at 10, 32, and 100 represent kTabBarMargin, kTabBarMarginWhenOverflowTabs,
>+// and kMinTabWidth. I KNOW THIS IS BAD PRACTICE but I didn't know how to get
>+// at the variables.

Encapsulation is a feature, not a bug--if you can't think of a way to get values at some layer, it can be a hint that you shouldn't be trying.  You shouldn't need any of this code, since the tab bar rebuilds itself when its frame changes.

> - (IBAction)previousTab:(id)sender
> {
>+  BrowserTabBarView* tempBrowserTabBarView = [mTabBrowser browserTabBarView];
>+  int selectedTabViewItemIndex = [mTabBrowser indexOfTabViewItem:[mTabBrowser selectedTabViewItem]];
>+  int novt = [tempBrowserTabBarView numberOfVisibleTabs];
...

First, don't use abbreviations like |novt| and |lmvti|, as they kill readability.  Second, I'm again not clear on why all this code is here; when the active tab changes, the tab bar will rebuild, and the rebuild will do the correct thing.




Overall, the internal tab bar implementation is getting closer, but the tab bar implementation logic being scattered into higher-level classes is not the direction things should be going.

Comment 81

[Desmond Elliott]

Stuart,

I have taken onboard your comments but I am working in the tab dragging patch for the time being. Rest assured that any spare time I have, I am working on your proposal.

There are a couple of problems I have noticed

1) When you create a new tab, tabWillChange: is not called.
2) rebuildTabBar is called twice and the value of NSSelectedTab is not changed until the 2nd call which means that catching the active tab when there are more tabs being loaded from a bookmark folder than can fit in the tab bar is a problem.

Comment 82

[Stuart Morgan]

Would hooking up a delegate to receive tabView:willSelectTabViewItem: callbacks give you the information you need?

Comment 83

[Desmond Elliott]

Attachment: WIP7

WIP6 was a load of bollocks but it still existed.

Some new things here, I brought back the pop-up menu but that will be getting shifted into right-click or control-click on the scroll buttons. The pop-up menu uses dividers to show the relative context of the tabs in the tab bar with respect to all open tabs.

I hope that I have removed all of my crazy high-level stuff and brought it all back down to where it makes sense. I overrode a few of NSTabView's methods to achieve the effects you would expect from Command+option+direction and most things work now.

This patch should iterate a few more times and after I get right-click working, a few times more and then hopefully we will have something solid to work from.

Requested Nick Kreeger as a reviewer, but your thoughts are always welcome Stuart.

Comment 84

[froodian (Ian Leue)]

Comment on attachment 234428 [details] [diff] [review]
WIP7

Please finish the patch for the implementation you've been working on for the last three months, then add new functionality and UI in followup bugs.

Comment 85

[Desmond Elliott]

Attachment: WIP7a

There are still significant changes to the code that need to be reviewed. This new iteration includes click and hold scrolling of tabs and proper handling of opening new background tabs.

Nate has shared some of his own code with me to show how to implement right-click pop-up menus on buttons but I am still trying to figure out exactly how it works. It seems like Apple have never intended for people to do this.

Comment 86

[Desmond Elliott]

Attachment: WIP7b

No overflow menu button to avoid an "r- war" ;)

Comment 87

[Stuart Morgan]

Comment on attachment 234583 [details] [diff] [review]
WIP7b

It's getting much closer. Specific comments follow, but some general notes:
- There is weird drawing when the active tab is up against an active left-scroll button.  I would guess that this is caused by the tab-before-the-selected-tab being set to draw a divider; try making it only do that if it's visible.
- If I close background tabs that are left of the active tab, and there is left overflow, the tabs fill in from the left, leaving the active tab in the same place.  That seems unexpected.
- Double-clicking the scroll buttons makes a new tab; you need to change the |else if (!clickedTabButton && [theEvent clickCount] == 2)| in BrowserTabBarView's mouseDown: to also check for the scroll buttons.

>+  NSButton*         mOverflowMenuButton;  // button for overflow menu if we've got more tabs than space (STRONG)

Either get rid of all the menu stuff from this version of the patch, or leave it all in and enabled; don't just comment out pieces of it.

>-  
>+    

Don't add whitespace to blank lines

>+  float widthOfTabBar = NSWidth([self tabsRect]);


You still need to set mOverflowTabs to NO before doing this, as mentioned in comment 80.  Otherwise, the first rebuild after the bar goes from overflowing to not computes the widths incorrectly.

>+  TabButtonCell *tabButtonCell = nil;

Declare this inside the loop each time, since it doesn't need to persist outside of each loop's context

>+  TabButtonCell *previousTabButtonCell = nil;

This will be unnecessary after changes below

>+    [mOverflowMenu addItem:[[tabButtonCell tabViewItem] menuItem]];
>+  }
>+  if (mLeftMostVisibleTabIndex > 0) {
>+    [mOverflowMenu addItem:[NSMenuItem separatorItem]];
>+  }

Again, all the menu stuff needs to be removed for this patch if it's not going to have the menu for now; same in the subsequent loops

>+    // I still have GDB pop up and shout at me if I try and do this at the end
>+    // so I'm keeping it here just now.
>+    if ([[tabButtonCell tabViewItem] tabState] == NSSelectedTab) {
>+      [tabButtonCell setDrawDivider:NO];
>+      [previousTabButtonCell setDrawDivider:NO];
>     }
>+    previousTabButtonCell = tabButtonCell;

Fix per IRC discussion

>+    if (mLeftMostVisibleTabIndex == 0) {
>+      [mOverflowLeftButton setEnabled:NO];
>+    }
>+    else {
>+     [mOverflowLeftButton setEnabled:YES];
>+    }

[mOverflowLeftButton setEnabled:(mLeftMostVisibleTabIndex == 0)];
Similarly with the right button

>+-(void)initOverflow
>+{
>+  if (!mOverflowLeftButton) {
...
>+  }
>+  if (!mOverflowRightButton) {
...
>+  }

Make a setupScrollButton: helper method that will do all the common parts of this setup

>   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
>+  [mOverflowMenu removeItemsFromIndex: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];
> }

Remove for now

>+-(IBAction)scrollLeft:(id)aSender
>+{
>+  if (mLeftMostVisibleTabIndex > 0) {
>+    --mLeftMostVisibleTabIndex;
>+  }
>+  [self rebuildTabBar];

Put the rebuildTabBar inside the if; there's no reason to do all the work if no scrolling happens.  Same with scrollRight:

>+// Called when you select a tab to scroll to using the pop-up menu.

That's not the only use.  Document behavior, not triggering UI.

>+-(void)scrollToTab:(int)index

scrollToTabIndex:

>+  if (mLeftMostVisibleTabIndex < index && index < mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) {

Off-by-one error; should be mLeftMostVisibleTabIndex <= index

>+  else if (index + mNumberOfVisibleTabs > [mTabView numberOfTabViewItems]) {
>+    // Setting the left most visible tab index to |index| would cause lots of 
>+    // 'empty' tabs to be drawn so this is tinkered accordingly.
>+    mLeftMostVisibleTabIndex = [mTabView numberOfTabViewItems] - mNumberOfVisibleTabs;
>+  }
>+  else {
>+    // |index| lies in an area that is suitable for drawing the left most tab
>+    // index exactly where it is asked to.
>+    mLeftMostVisibleTabIndex = index;
>+  }

This logic is wrong.  Scrolling a tab to visible is not the same this as scrolling it to leftmost.  If I am looking at tabs 10-20 of 50, and I scrollToTabIndex:40, I would expect to be looking at tabs 30-40, not 40-50.

>+  [self rebuildTabBar];

Again, don't rebuildTabBar in the case where nothing happens.

>+    //rect.size.width -= 3 * kTabBarMarginWhenOverflowTabs;

Remove

>+   // A hack to force the tab bar to rebuild if mOverflowTabs and tabs load in the background
>+  [self refreshTabBar:YES];

Why you need this for addTabViewItem:, but not removeTabViewItem: or insertTabViewItem:atIndex:?  And why wasn't it ever needed before?

>+/* These methods have been over ridden because the tabs may need to slide 
>+   to visually reflect the actions performed by these methods. */   

You've only covered about half the selection methods.  They all need to be covered--or better yet, you should find what they are all implemented in terms of (assuming it's something public) and override that.

>+// Called when you select a tab from the pop-up menu. This calls for the tab bar
>+// to be updated to reflect this change of tab.
> - (void) selectTab:(id)sender

Is that really the only time this is called?  That seems unlikely.

Comment 88

[Stuart Morgan]

(In reply to comment #87)
> [mOverflowLeftButton setEnabled:(mLeftMostVisibleTabIndex == 0)];
> Similarly with the right button

Only of course you want
[mOverflowLeftButton setEnabled:(mLeftMostVisibleTabIndex != 0)];
So it's not completely backwards :P

Comment 89

[Desmond Elliott]

Attachment: WIP7c

(In reply to comment #87)
> It's getting much closer. Specific comments follow, but some general notes:
> - There is weird drawing when the active tab is up against an active
> left-scroll button.  I would guess that this is caused by the
> tab-before-the-selected-tab being set to draw a divider; try making it only do
> that if it's visible.

Working on this, there are also problems relating to the selected tab being to the right
or to the left of the currently visible tabs and there being no divider drawn.

> - If I close background tabs that are left of the active tab, and there is left
> overflow, the tabs fill in from the left, leaving the active tab in the same
> place.  That seems unexpected.

I cannot reproduce this behaviour, could you provide a STR so I can see it happening and fix it?

> - Double-clicking the scroll buttons makes a new tab; you need to change the
> |else if (!clickedTabButton && [theEvent clickCount] == 2)| in
> BrowserTabBarView's mouseDown: to also check for the scroll buttons.

Fixed this.

> >-  
> >+    
> 
> Don't add whitespace to blank lines

Comedy.

> >+  else if (index + mNumberOfVisibleTabs > [mTabView numberOfTabViewItems]) {
> >+    // Setting the left most visible tab index to |index| would cause lots of 
> >+    // 'empty' tabs to be drawn so this is tinkered accordingly.
> >+    mLeftMostVisibleTabIndex = [mTabView numberOfTabViewItems] - mNumberOfVisibleTabs;
> >+  }
> >+  else {
> >+    // |index| lies in an area that is suitable for drawing the left most tab
> >+    // index exactly where it is asked to.
> >+    mLeftMostVisibleTabIndex = index;
> >+  }
> 
> This logic is wrong.  Scrolling a tab to visible is not the same this as
> scrolling it to leftmost.  If I am looking at tabs 10-20 of 50, and I
> scrollToTabIndex:40, I would expect to be looking at tabs 30-40, not 40-50.

I am a little bit confused what you mean by this, as things stand there is no way to perform such a jump of position and what would you expect to look at if you were looking at tabs 30-40 and you scrollToTabIndex:10? If I am following you correctly then you'd expect to see 1 - 10?

> >+   // A hack to force the tab bar to rebuild if mOverflowTabs and tabs load in the background
> >+  [self refreshTabBar:YES];
> 
> Why you need this for addTabViewItem:, but not removeTabViewItem: or
> insertTabViewItem:atIndex:?  And why wasn't it ever needed before?

I don't anymore. There were problems with creating new background tabs but they seem to have been resolved.

> You've only covered about half the selection methods.  They all need to be
> covered--or better yet, you should find what they are all implemented in terms
> of (assuming it's something public) and override that.

They are all implemented in terms of - (void)selectTabViewItemAtIndex:(int)index so I am now just overriding that.

> >+// Called when you select a tab from the pop-up menu. This calls for the tab bar
> >+// to be updated to reflect this change of tab.
> > - (void) selectTab:(id)sender
> 
> Is that really the only time this is called?  That seems unlikely. 

The only other place it is called in BrowserTabView to select the jump back tab.

Thanks for your review, it does feel like the code is getting closer and I'm much happier with it at the moment.

Comment 90

[Stuart Morgan]

Comment on attachment 234670 [details] [diff] [review]
WIP7c

>+  else if (!clickedTabButton && [theEvent clickCount] == 2) {
>+    if (!mOverflowTabs) {
>+      [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
>                                                         object:mTabView];
>+    }
>+  }

I'd rather see this doing hit-testing on the overflow buttons (you'll need a function that does that when you get to tab dragging anyway) rather than assuming overflow means there's no blank area, but I suppose that can wait for another bug.

>+  for (i; i < mLeftMostVisibleTabIndex; i++) {
>+    TabButtonCell *tabButtonCell = [[mTabView tabViewItemAtIndex:i] tabButtonCell];
>+    [tabButtonCell setFrame:overflownTabRect];
>+    [tabButtonCell hideCloseButton];
>+  }

Don't the left and right overflow tabs generally need setDrawDivider:NO, to prevent some of the drawing issues?

>+  TabButtonCell *tabButtonCell = nil;
>+  if ((tabButtonCell = [[mTabView selectedTabViewItem] tabButtonCell]) != nil) {
>+    [tabButtonCell setDrawDivider:NO];
>+    int selectedTabIndex = [mTabView indexOfTabViewItem:[mTabView selectedTabViewItem]];
>+    if (selectedTabIndex > 0) {
>+      [[[mTabView tabViewItemAtIndex:(selectedTabIndex - 1)] tabButtonCell] setDrawDivider:NO];
>     }
>   }

Store the thing you need multiple times (selectedTabView), not the thing you need once (tabButtonCell). I.e.:

TabViewItem *selectedTab = [mTabView selectedTabViewItem]
if (selectedTab) {
  [[selectedTab tabButtonCell] setDrawDivider:NO];
  int selectedTabIndex = [mTabView indexOfTabViewItem:selectedTab]
  ...

But both of these are apparently going to need some extra logic based on whether they are within the visible tab window.

>+  if (position == @"Left") {
>+    [button setImage:[NSImage imageNamed:@"overflow_button_left"]];
>+    [button setAction:@selector(scrollLeft:)];
>+  }
>+  else {
>+    [button setImage:[NSImage imageNamed:@"overflow_button_right"]];
>+    [button setAction:@selector(scrollRight:)];
>+  }

Just do the common things in a setup method; do the image and action binding in initOverflow. (As a general note: never use a string as an enum; it's really inefficient.)  The helper method should also be declared in the private methods list.

>+    --mLeftMostVisibleTabIndex;
>+    [self rebuildTabBar];

Given the number of places you have to do a:
<update mLeftMostVisibleTabIndex>;
[self rebuildTabBar];
you should make a setter for mLeftMostVisibleTabIndex that does the right thing (sanity checking + calling rebuildTabBar if anything has changed).

>+-(void)scrollToTabIndex:(int)index

I forgot during the last review, but as mentioned before call this scrollTabIndexToVisible:

>+  if (mLeftMostVisibleTabIndex <= index && index < mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) {
>+    // Do nothing because index is already in the current tab bar.
>+  }
>+  else if (index + mNumberOfVisibleTabs > [mTabView numberOfTabViewItems]) {

Given the setter you need to make, this doesn't won't need to be a special case.

Once again, the logic for scrolling a tab to visible should be:
if (currently visible)
  do nothing (as you currently do for the first case)
else if (target tab is left of visible)
  make it leftmost
else
  make it rightmost

>-  NSRect rect = [self bounds];
>+  NSRect rect = [self frame];

Why are you making this change?  For internal drawing, the frame in the superview's coordinate system is irrelevant.

>+-(void)selectTabViewItemAtIndex:(id)aSender
>+{
>+  [[self tabBar] scrollToTabIndex:aSender];
>+  [super selectTabViewItemAtIndex:aSender];
>+}

This has the wrong prototype (and doesn't make any sense); selectTabViewItemAtIndex takes an integer index, not a sender object. This doesn't even compile on gcc4.

Comment 91

[Desmond Elliott]

Attachment: WIP7d

(In reply to comment #90)
> (From update of attachment 234670 [details] [diff] [review] [edit])
> >+  else if (!clickedTabButton && [theEvent clickCount] == 2) {
> >+    if (!mOverflowTabs) {
> >+      [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
> >                                                         object:mTabView];
> >+    }
> >+  }
> 
> I'd rather see this doing hit-testing on the overflow buttons (you'll need a
> function that does that when you get to tab dragging anyway) rather than
> assuming overflow means there's no blank area, but I suppose that can wait for
> another bug.

Done.

> But both of these are apparently going to need some extra logic based on
> whether they are within the visible tab window.

Working on trying to figure out exactly what needs to be done to fix this.
 
> >+  if (position == @"Left") {
> >+    [button setImage:[NSImage imageNamed:@"overflow_button_left"]];
> >+    [button setAction:@selector(scrollLeft:)];
> >+  }
> >+  else {
> >+    [button setImage:[NSImage imageNamed:@"overflow_button_right"]];
> >+    [button setAction:@selector(scrollRight:)];
> >+  }
> 

Done

> Just do the common things in a setup method; do the image and action binding in
> initOverflow. (As a general note: never use a string as an enum; it's really
> inefficient.)  The helper method should also be declared in the private methods
> list.

Done

> >+    --mLeftMostVisibleTabIndex;
> >+    [self rebuildTabBar];
> 
> Given the number of places you have to do a:
> <update mLeftMostVisibleTabIndex>;
> [self rebuildTabBar];
> you should make a setter for mLeftMostVisibleTabIndex that does the right thing
> (sanity checking + calling rebuildTabBar if anything has changed).
> 

Done

> >+-(void)scrollToTabIndex:(int)index
> 
> I forgot during the last review, but as mentioned before call this
> scrollTabIndexToVisible:
> 
> >+  if (mLeftMostVisibleTabIndex <= index && index < mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) {
> >+    // Do nothing because index is already in the current tab bar.
> >+  }
> >+  else if (index + mNumberOfVisibleTabs > [mTabView numberOfTabViewItems]) {
> 
> Given the setter you need to make, this doesn't won't need to be a special
> case.
> 
> Once again, the logic for scrolling a tab to visible should be:
> if (currently visible)
>   do nothing (as you currently do for the first case)
> else if (target tab is left of visible)
>   make it leftmost
> else
>   make it rightmost
>

Done
 
> >-  NSRect rect = [self bounds];
> >+  NSRect rect = [self frame];
> 
> Why are you making this change?  For internal drawing, the frame in the
> superview's coordinate system is irrelevant.

Mike suggested it way back at the beginning as he told me that [self bounds] was not what I wanted to use.
 
> >+-(void)selectTabViewItemAtIndex:(id)aSender
> >+{
> >+  [[self tabBar] scrollToTabIndex:aSender];
> >+  [super selectTabViewItemAtIndex:aSender];
> >+}
> 
> This has the wrong prototype (and doesn't make any sense);
> selectTabViewItemAtIndex takes an integer index, not a sender object. This
> doesn't even compile on gcc4.

Indeed, this was total nonsense that actually worked on gcc 3.3, I have changed it to reflect what it should be.

Current visible problems: divider drawing is sometimes incorrect in the following instances:

Unwanted tab divider:
1. Selected tab is the left most visible tab and there are tabs overflowing to the left

Wanted tab divider:
1. Selected tab index is 0 and this tab is overflowing.
2. Selected tab index is just to the right of the visible tabs.

I have tracked down these problems and believe they will be fixed in the next submission.

I hope that covers all of the concerns that you raised with my last patch.

Comment 92

[Stuart Morgan]

Comment on attachment 234774 [details] [diff] [review]
WIP7d

Looking good; just a couple small changes below.

One more odd behavior in addition to the divider drawing issues you already know about: the scroll buttons are considered valid drop targets for bookmarks, and when they are active as drop zones the entire tab bar highlights.  Just the button you are hovering over should highlight.

That raises the question of what should be done with drops on the left scroll button.  Having it make new tabs on the right doesn't really make sense, so it should probably either be disabled as a drop zone, or, for parity, should make a new tab at index 0.  That's a minor enough oddity that it could be tackled in a follow-up bug though.

(The behavior with tab closing I mentioned before was user error, by the way; I was at the right end of the tabs, so filling from the left was the only thing it could do.)


>+-(BOOL)tabIsVisibleInTabBar:(int)tabIndex

tabIndexIsVisible:--Things that take tab indexes should be named as such so people don't try to pass tab objects to them (and the InTabBar isn't adding anything, since that's the only place a tab can be visible)

>+  if (mLeftMostVisibleTabIndex <= tabIndex && tabIndex < mNumberOfVisibleTabs + mLeftMostVisibleTabIndex) {
>+    return YES;
>+  }
>+  return NO;

Just return the condition directly.

>+// A helper method that returns an NSButton for the tab bar based on which side
>+// of the tab bar the button is going to.

The "based on which side..." part is not true anymore

>+-(NSButton*)setupScrollButton:(NSButton*)button

No need for this to take a button since you assign based on the return value. Call it newScrollButton (to follow standard Cocoa naming for things that return retained objects), and just have it return a locally created button.

>+  mLeftMostVisibleTabIndex = index;
>+  [self rebuildTabBar];

Do a check that index != mLeftMostVisibleTabIndex, to skip the rebuild if nothing is actually changing.

Comment 93

[Desmond Elliott]

Attachment: WIP7e

(In reply to comment #92)

> One more odd behavior in addition to the divider drawing issues you already
> know about: the scroll buttons are considered valid drop targets for bookmarks,
> and when they are active as drop zones the entire tab bar highlights.  Just the
> button you are hovering over should highlight.
> 
> That raises the question of what should be done with drops on the left scroll
> button.  Having it make new tabs on the right doesn't really make sense, so it
> should probably either be disabled as a drop zone, or, for parity, should make
> a new tab at index 0.  That's a minor enough oddity that it could be tackled in
> a follow-up bug though.

I just noticed that too and it does look quite funny. I have no opinion on how it should be handled and am more than happy to leave it to a follow-up bug if you are.

Apart from that, I have implemented everything that you suggested in comment 92

Comment 94

[Stuart Morgan]

Comment on attachment 235946 [details] [diff] [review]
WIP7e

You still to fix the logic on when to draw dividers, so tabs still draw incorrectly when the selected tab is leftmost or one to the right of rightmost; that really should be done before this lands.

A few things to clean up while you are spinning a new patch (mostly style and warnings):

>+  else if (!clickedTabButton && [theEvent clickCount] == 2 && !clickedScrollButton) {
>+      [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
>                                                         object:mTabView];
>+  }

No {} for one-line conditionals.  Also, move !clickScrollButton to after !clickedTabButton, so it reads a bit smoother.

>+  if (NSPointInRect(clickPoint, [mOverflowLeftButton frame])) {
>+    return mOverflowLeftButton;
>+  }
>+  if (NSPointInRect(clickPoint, [mOverflowRightButton frame])) {
>+    return mOverflowRightButton;
>+  }
...
>+    if (mNumberOfVisibleTabs + mLeftMostVisibleTabIndex > numberOfTabs) {
>+      mLeftMostVisibleTabIndex = numberOfTabs - mNumberOfVisibleTabs;
>     }

No {}

>+  for (i; i < mLeftMostVisibleTabIndex; i++) {
>+    TabButtonCell *tabButtonCell = [[mTabView tabViewItemAtIndex:i] tabButtonCell];
...
>+    TabButtonCell *tabButtonCell = [[mTabView tabViewItemAtIndex:i] tabButtonCell];
...
>+    TabButtonCell *tabButtonCell = [[mTabView tabViewItemAtIndex:i] tabButtonCell];

These all need to be [(BrowserTabViewItem*)[mTabView tabViewItemAtIndex:i] tabButtonCell]

>+    [tabButtonCell setFrame:overflownTabRect];
>+    [tabButtonCell hideCloseButton];
>+    [tabButtonCell setDrawDivider:NO];
>+  }
>+  BrowserTabViewItem *selectedTab = [mTabView selectedTabViewItem];

This needs a cast to BrowserTabViewItem* as well.

>+  if (selectedTab) {
>+    [[selectedTab tabButtonCell] setDrawDivider:NO];
>+    int selectedTabIndex = [mTabView indexOfTabViewItem:selectedTab];
>+    if (selectedTabIndex > 0) {
>+      [[ tabButtonCell] setDrawDivider:NO];
>     }

[mTabView tabViewItemAtIndex:(selectedTabIndex - 1)] also needs an BrowserTabViewItem* cast.

>+  if (mLeftMostVisibleTabIndex > 0) {
>+    [self setLeftMostVisibleTabIndex:(mLeftMostVisibleTabIndex - 1)];
>+  }
...
>+  if ((mLeftMostVisibleTabIndex + mNumberOfVisibleTabs) < [mTabView numberOfTabViewItems]) {
>+    [self setLeftMostVisibleTabIndex:(mLeftMostVisibleTabIndex + 1)];
>+  }

No {}

>+  if ([self tabIndexIsVisible:index]) {
>+    return;
>+  }
>+  else if (index < mLeftMostVisibleTabIndex) {
>+    [self setLeftMostVisibleTabIndex:index];
>+  }
>+  else {
>+   [self setLeftMostVisibleTabIndex:((index + mNumberOfVisibleTabs) <= [mTabView numberOfTabViewItems] ? (index + mNumberOfVisibleTabs): [mTabView numberOfTabViewItems] - mNumberOfVisibleTabs)];
>+  }  

No {}, but also just do this as:

if (index < mLeftMostVisibleTabIndex)
...
else if (index >= mLeftMostVisibleTabIndex + mNumberOfVisibleTabs)
...

So you don't need the early return.

More importantly, the code for scrolling right still isn't doing the right thing; it's really helpful to walk code through examples as you write it when dealing with code like this.  If you have 20 tabs, 5 visible, and are currently at 0-4, scrolling to 15 should show 11-15, and scrolling to 16 should show 12-16.  This code would give 20-24 when scrolling to 15, none of which exist, and 16-20 for 16, which isn't the desired behavior.  All scrolling right needs to do is
    [self setLeftMostVisibleTabIndex:(index - mNumberOfVisibleTabs + 1)]
(which you know can't come out <0 because of the condition that gets you to this code)


>   if ([aTabView isKindOfClass:[BrowserTabView class]] && [aTabViewItem isKindOfClass:[BrowserTabViewItem class]]) {
>     [(BrowserTabViewItem *)[aTabView selectedTabViewItem] willDeselect];
>+    [[aTabView tabBar] scrollTabIndexToVisible:[aTabView indexOfTabViewItem:aTabViewItem]]; // Makes sure this selected tab is visible in the tab bar

Now that you are already overriding selectTabViewItemAtIndex: to do the scrolling, is this really necessary?  I'd really rather this (and consequently the tabBar method) not exist if possible. If it really has to stay, you need to cast aTabView and include BrowserTabView.h.

Comment 95

[Jon Hicks]

Attachment: Suggestions for tab overflow icon

I've put together a few ideas for the overflow icon, using 5 types of arrow, and 3 suggestions for backgrounds. My favourites are #6 and in particular #9. #6 uses the same size triangle as is used on system scrollbars, whereas #9 uses a single chevron from the current overflow icon, but slightly narrower. I think  a single chevron will relate it to the overflow, without causing confusion.

Comment 96

[Samuel Sidler (old account; do not CC)]

(In reply to comment #95)
> I've put together a few ideas for the overflow icon, using 5 types of arrow,
> and 3 suggestions for backgrounds. My favourites are #6 and in particular #9.

Thanks for the suggestions Jon. :)

I also like #9.

I'm unclear as to whether we need "disabled" versions of this icon as well. Desmond, do we only need two icons (left and right) or four icons (left/right disabled/active)?

Comment 97

[Desmond Elliott]

Attachment: Existing overflow implementations in OS X

(In reply to comment #95)
> Created an attachment (id=238812) [edit]
> Suggestions for tab overflow icon
> 
> I've put together a few ideas for the overflow icon, using 5 types of arrow,
> and 3 suggestions for backgrounds. My favourites are #6 and in particular #9.
> #6 uses the same size triangle as is used on system scrollbars, whereas #9 uses
> a single chevron from the current overflow icon, but slightly narrower. I think
>  a single chevron will relate it to the overflow, without causing confusion. 
> 

Thanks John, sorry for the late reply - they all look great.

Which set of buttons we use is a matter of design preference and that should be discussed.

If we are to go with the Dashboard / iTunes Store slide by a full width of visible items then it would make sense to go with #7 for consistency (even though Apple have shown no consistency in their design) 

If we go for a single slide per click then #6/#9 are both great options. I'd like to reserve something like #9 for the hopeful tab-list drop-down that I had been working on towards the end of the project.

To answer Smokey's question - it should not be necessary to have 4 different icons because setting a button to be disabled will grey out the image inside the button.

Comment 98

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Why are we now making the button look like an active tab?  I think that's going to be confusing.  I prefer 1/4 myself ;)

Comment 99

[Desmond Elliott]

(In reply to comment #98)
> Why are we now making the button look like an active tab?  I think that's going
> to be confusing.  I prefer 1/4 myself ;)
> 

I agree that the scroll buttons should probably look 'inactive' when they are not in use and would advocate one of the icons on the left.

Comment 100

[philippe (part-time)]

(In reply to comment #98)
> Why are we now making the button look like an active tab?  I think that's going
> to be confusing.  I prefer 1/4 myself ;)
> 

+ 1.
Having the background look like the active tab will be confusing. Adding a highlight effect when the user hovers over the arrow (or clicks) could be good feedback (I notice that the new iTunes does that for clicking on that cloverflow thing only). BonEcho brightens the background on hover/click.

Comment 101

[Jon Hicks]

(In reply to comment #98)
> Why are we now making the button look like an active tab?  I think that's going
> to be confusing.  I prefer 1/4 myself ;)

We're not doing anything yet ;)

I feel that the overflow icon should look different to the tab bar, but yes I agree, making it the same gradient as the active tab could be misleading. 

How about #14 though?

Comment 102

[Håkan Waara]

Personally, I prefer the suggestions in the middle (6-10). I agree with previous comments that the normal "unpushed" state should not use the same background as the active tab.

Whichever we go with, it will also need to look reasonable with the 10.3-Camino look too (striped toolbar).

Comment 103

[Desmond Elliott]

(In reply to comment #102)
> Personally, I prefer the suggestions in the middle (6-10). I agree with
> previous comments that the normal "unpushed" state should not use the same
> background as the active tab.
> 
> Whichever we go with, it will also need to look reasonable with the 10.3-Camino
> look too (striped toolbar).
> 

Are you - or is anybody else - able to provide a screenshot of of what Camino looks like in 10.3?

Comment 104

[Jon Hicks]

Attachment: Second iteration of icon ideas

Here is the option #1 from the last iteration made larger, and showing suggested pressed and disabled states.

I've also included the chevron style icon, as I think the triangle, while echoing the scrollbars, looks a little dull (and too much like a toggle seen in the bookmarks manager).

Comment 105

[Desmond Elliott]

(In reply to comment #104)
> Created an attachment (id=239236) [edit]
> Second iteration of icon ideas
> 
> Here is the option #1 from the last iteration made larger, and showing
> suggested pressed and disabled states.
> 
> I've also included the chevron style icon, as I think the triangle, while
> echoing the scrollbars, looks a little dull (and too much like a toggle seen in
> the bookmarks manager).

They look great. I am more than satisfied with them.

Are we OK to go with these icons as a first attempt?

Comment 106

[Desmond Elliott]

Attachment: WIP 7f

(In reply to comment #94)
> You still to fix the logic on when to draw dividers, so tabs still draw
> incorrectly when the selected tab is leftmost or one to the right of rightmost;
> that really should be done before this lands.

Done.

Also done everything else requested.

Comment 107

[Stuart Morgan]

Comment on attachment 240977 [details] [diff] [review]
WIP 7f

Nits:

> -(id)initWithFrame:(NSRect)frame 
> {
>   self = [super initWithFrame:frame];
>   if (self) {
>     mActiveTabButton = nil;
>-    mOverflowButton = nil;
>+    mOverflowRightButton = nil;
>+    mOverflowLeftButton = nil;
>     mOverflowTabs = NO;
>+    mNumberOfVisibleTabs = 0;
>+    mLeftMostVisibleTabIndex = 0;

This entire chunk of assignments can actually all be removed; member variables are automatically zero/nil-inited.

>+  // The button divider image should only be drawn in the left most visible tab is not selected.

Typo fix; s/in the left/if the left/

>+  if (mLeftMostVisibleTabIndex != [mTabView indexOfTabViewItem:[mTabView selectedTabViewItem]]) {

Swap the order of the inequality test so it reads the same as your comment.

>+    if (mOverflowTabs)
>+	  [mButtonDividerImage compositeToPoint:NSMakePoint(kTabBarMarginWhenOverflowTabs - [mButtonDividerImage size].width, 0) operation:NSCompositeSourceOver];
>+	else
>+      [mButtonDividerImage compositeToPoint:NSMakePoint(kTabBarMargin - [mButtonDividerImage size].width, 0) operation:NSCompositeSourceOver];

Fix up the indenting here (some tabs snuck in).

>+  else if (!clickedTabButton && !clickedScrollButton && [theEvent clickCount] == 2) {
>+      [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
>                                                         object:mTabView];
>+  }

No {}, and fix the indenting.

>+  // If there are tabs to the left of the currently active tab then draw them
>+  // with a width of 0 The tabs between the left-most visible tab index and the
>+  // right-most visible tab index should be drawn with the width calculate above.
>+  // The tabs to the right of the right-most visible tab index should be drawn
>+  // with a width of 0.

Tighten this comment up; it's a bit long. Something like:
  // Lay out the tabs, giving off-screen tabs a width of 0
would be plenty.

>+  NSRect overflownTabRect = NSMakeRect(xCoord, 0, 0, 0);

overflowTabRect (overflown is a form of 'to overfly')

>+    if (selectedTabIndex > 0 && [self tabIndexIsVisible:selectedTabIndex]) {
>+      [[(BrowserTabViewItem*)[mTabView tabViewItemAtIndex:(selectedTabIndex - 1)] tabButtonCell] setDrawDivider:NO];
>     }

No {}


>+#pragma mark -
...
>+#pragma mark -

I'm all for pragma marks for grouping code, but I don't see any logical group enclosed by these marks. Take these out unless you can find a few cohesive groupings of methods.

>+// Called when FKA is used for selecting tabs and will be used in the future to
>+// allow pop-up menu access and slide-by-width access.
>+-(void)scrollTabIndexToVisible:(int)index

Document behavior, not callers:
// Scrolls the tab bar to make the given tab visible if it isn't already

>+  else if (index >= mLeftMostVisibleTabIndex + mNumberOfVisibleTabs)
>+   [self setLeftMostVisibleTabIndex:(index - mNumberOfVisibleTabs + 1)];

Fix the indenting here.

>+/* This method has been over ridden because the tabs may need to slide 
>+   to visually reflect the actions performed by these methods. */   

Use //-style comments, and again, tighten it up a bit to something like:
// Tabs should be scrolled into view when selected

Lastly, you should add yourself to the contributors section of BrowserTabBarView.{h,mm}!


That's all just cleanup though, so the long-awaited r=me for trunk landing with those changes.  Thanks for your patience, and all the work!

After this lands on the trunk, we need a new bug covering the need for some kind of direct menu (menu bar, extra tab bar widget/widgets, or both) access to specific tabs. Once that's in, we can land both pieces on the branch. We'll also need a bug for the cosmetic issue that due to something about the way tabs are drawn, the right scroll button is about 8 pixels wider than the left scroll button.

Comment 108

[Desmond Elliott]

Attachment: WIP7fi

(In reply to comment #107)

Fixed a minor regression - opening tabs from a tab folder now works properly again. It turns out that everything is implemented in terms of |selectedTabViewItem|, not |selectTabViewItemAtIndex|.

> That's all just cleanup though, so the long-awaited r=me for trunk landing with
> those changes.  Thanks for your patience, and all the work!

Thanks for all of the patience from your side also, it is really appreciated.

> After this lands on the trunk, we need a new bug covering the need for some
> kind of direct menu (menu bar, extra tab bar widget/widgets, or both) access to
> specific tabs. Once that's in, we can land both pieces on the branch. We'll
> also need a bug for the cosmetic issue that due to something about the way tabs
> are drawn, the right scroll button is about 8 pixels wider than the left scroll
> button.

I attempted to address that back in WIP7 but was told to leave it for another day. What I worked on involved putting a third button on the bar and there seems to be some heavy opposition to that.

The cosmetic issue is caused as a result of drawing the right most visible tab divider immediately after the tab. The way the width of drawn tabs is calculated there is some left over space. How we approach this issue needs to be thought through.

Targetting at Stuart and Mike for R? and SR? respectively.

Comment 109

[Stuart Morgan]

Comment on attachment 241094 [details] [diff] [review]
WIP7fi

>     [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
>-                                                        object:mTabView];
>+                                          object:mTabView];

r=me, but get rid of this change, since the spacing was right before (you should wait for any comments from pink before respinning).

Comment 110

[Stuart Morgan]

(In reply to comment #108)
> I attempted to address that back in WIP7 but was told to leave it for another
> day.

That's why it needs a new bug ;)

Comment 111

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 241094 [details] [diff] [review]
WIP7fi

sr=pink

Comment 112

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The only project changes this will need are the new scroll button icons, right?  I most likely won't be able to spin project changes until Sun/Mon at the earliest....

Comment 113

[froodian (Ian Leue)]

Attachment: This is the bitrot police

This is just a re-spin of delliott's WIP7fi.  It makes a couple minor changes

- Unbitrots it
- Changes the icon names from overflow_button_foo to tab_scroll_button_foo
- Makes smorgan's nit
- Fixes a warning by casting a (floored, so definitely safe for int) value as an int

Comment 114

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: trunk project patch

This patch adds the new icons (renamed to sane names of tab_scroll_button_left.tif and tab_scroll_button_right.tif) and removes the now-unused overflow icon (it can be cvs removed later).

Comment 115

[froodian (Ian Leue)]

Checked in on trunk.  Congratulations Desmond!

Comment 116

[froodian (Ian Leue)]

Bug 355492 has been filed for the final versions of the icons.  Bug 355493 tracks the landing of the changes from this bug on 1.8branch.