406510 - Restructure tabs to be truly view-based

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Stuart Morgan]

Attachment: refactoring (trunk version)

While looking into bug 405672 (which this patch will fix), I started doing some cleanup of dead code from the old tab implementation that was confusing me, and then some refactoring, and many hours later the result was a restructuring of the tab implementation with all the tab functionality consolidated into a view. The current tab implementation tried to be cell-based, but a rather convoluted supporting scaffolding had emerged that re-invented most view behavior scattered through several class, so it wasn't any lighter-weight than just using a view directly (but was much more confusing). At some point, if it needs to walk like a duck and quack like a duck, a trained and well-painted cat just isn't the right solution.

Although the patch is large, there's almost no *new* code here; the code of the new TabButtonView is just collected from other classes in a series of refactorings, with a small bit of new glue to manage them as views. I deliberately left as much intact as possible in the code I brought in so that this patch would focus on consolidation rather than re-implementation, so there are some //XXX comments carried along and left as they are for future work. Hopefully this will give us a more manageable foundation for our upcoming tab work.

I was testing continuously through my refactoring, and I'm not aware of any regressions (but more people banging on it is always good). This is a trunk version; I'll make a branch version (which should be very similar) once this is ready to be checked in.


Here's a high-level overview of what the patch does:

BrowserTabItemContainerView--a strange, very low-functionality "glue view" that bridged between the cell drawing and the tab bar--has been renamed TabButtonView and pulled out into its own class. The new TabButtonView has pulled in the following tab button implementation pieces from where they were dispersed:
- Ownership of the subviews/cells (close button, label, spinner) from BrowserTabViewItem
- Drawing logic from TabButtonCell (and a tiny bit from IconTabViewItem)
- Mouse tracking from RolloverTrackingCell (the parent of TabButtonCell, which was kind of trying to be a view, and wasn't used anywhere else)
- Click and drag handling from BrowserTabBarView
- Tooltip support from BrowserTabBarView

Code for managing making the tab rects dirty has been simplified in a few places by the fact that the view can be told to redraw directly, rather than having to go through BrowserTabView.

The zero-sized-frame hack for spillover tabs has been replaced by simply not having them in the view hierarchy when they aren't visible.

The now-unused setImageVisible: method from TruncatingTextAndImageCell has been removed.

The following classes now contain nothing useful, and have been completely removed (the cvs remove isn't in the patch because it adds a lot of text to the patch without serving a useful purpose):
  TabButtonCell
  RolloverTrackingCell
  IconTabViewItem

Comment 1

[Stuart Morgan]

(And for those like me that enjoy these sort of statistics: the net change in lines of code, including the removed classes but not counting license blocks, is around -550)

Comment 2

[Stuart Morgan]

(In reply to comment #0)
> [...] and pulled out into its own class.

s/class/file/

Comment 3

[Jeff Dlouhy]

Comment on attachment 291160 [details] [diff] [review]
refactoring (trunk version)


>   // drag tracking
>-  NSPoint           mLastClickPoint;

A lot of the tab dragging calculations are made within BrowserTabBarView and this is one of the variables that is used in calculating the distance dragged. So please keep this variable around.

 
>--(void)mouseUp:(NSEvent*)theEvent
>-{
>-  NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>-  TabButtonCell * clickedTabButton = [self buttonAtPoint:clickPoint];
>-  if (clickedTabButton && ![clickedTabButton willTrackMouse:theEvent inRect:[clickedTabButton frame] ofView:self])
>-    [[[clickedTabButton tabViewItem] tabItemContentsView] mouseUp:theEvent];
>-}
>-
>--(void)mouseDragged:(NSEvent*)theEvent
>-{
>-  NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>-  TabButtonCell *clickedTabButton = [self buttonAtPoint:clickPoint];
>-  if (clickedTabButton && 
>-      ![clickedTabButton willTrackMouse:theEvent inRect:[clickedTabButton frame] ofView:self])
>-      [[[clickedTabButton tabViewItem] tabItemContentsView] mouseDragged:theEvent];
>-  /* else if (!mDragStarted) {
>-    // XXX TODO: Handle dnd of tabs here
>-    if ((abs((int)(mLastClickPoint.x - clickPoint.x)) >= kTabDragThreshold) ||
>-        (abs((int)(mLastClickPoint.y - clickPoint.y)) >= kTabDragThreshold)) {
>-          NSLog(@"Here's where we'd handle the drag among friends rather than the drag manager");
>-    }*/
>-}

I use mouseDragged heavily in tab dragging, none of the code inside of these functions are needed atm; however having the functions around is essential for tab DnD.



>+    // Don't do anything with offscreen tabs
>+    if (i < mLeftMostVisibleTabIndex || i >= mLeftMostVisibleTabIndex + mNumberOfVisibleTabs)
>+      continue

Missing a ; here.


>+- (void)mouseUp:(NSEvent *)theEvent
>+{
>+  if (mSelectTabOnMouseUp) {
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kTabWillChangeNotifcation object:mTabViewItem];
>+    [[mTabViewItem tabView] selectTabViewItem:mTabViewItem];
>+    mSelectTabOnMouseUp = NO;
>+  }
>+}
>+
>+- (void)mouseDragged:(NSEvent*)theEvent
>+{
>+  NSRect  iconRect   = [self convertRect:[mLabelCell imageFrame] fromView:nil];//NSMakeRect(0, 0, 16, 16);
>+  NSPoint localPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>+
>+  if (!NSPointInRect(localPoint, iconRect) || ![mTabViewItem draggable])
>+    return;
>+
>+  // only initiate the drag if the original mousedown was in the right place...
>+  // implied by mSelectTabOnMouseUp
>+  if (mSelectTabOnMouseUp) {
>+    mSelectTabOnMouseUp = NO;
>+
>+    BrowserWrapper* browserView = (BrowserWrapper*)[mTabViewItem view];
>+
>+    NSString     *url = [browserView currentURI];
>+    NSString     *title = [mLabelCell stringValue];
>+    NSString     *cleanedTitle = [title stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "];
>+
>+    NSPasteboard *pboard = [NSPasteboard pasteboardWithName:NSDragPboard];
>+    [pboard declareURLPasteboardWithAdditionalTypes:[NSArray array] owner:self];
>+    [pboard setDataForURL:url title:cleanedTitle];
>+
>+    NSPoint dragOrigin = [self frame].origin;
>+    dragOrigin.y += [self frame].size.height;
>+
>+    [self dragImage:[NSImage dragImageWithIcon:[mLabelCell image] title:title]
>+                 at:iconRect.origin
>+             offset:NSMakeSize(0.0, 0.0)
>+              event:theEvent
>+         pasteboard:pboard
>+             source:self
>+          slideBack:YES];
>+  }
>+}

BrowserTabBarView needs to be notified of the mouse events, so possibly add a [super mouseUp:theEvent] and [super mouseDrggged:theEvent] respectively.


Overall this is a much welcomed refactoring of the tab drawing code and will help us move forward with future tab development.

r=me with those changes.

Comment 4

[Stuart Morgan]

Comment on attachment 291160 [details] [diff] [review]
refactoring (trunk version)

I talked with Jeff, and I'm going to go ahead and remove all that stuff in this patch, and we'll look at adding specifically targeted information passing necessary for DnD in his patch, rather than forwarding all mouse events.

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 291160 [details] [diff] [review]
refactoring (trunk version)

rs=pink

Comment 6

[Stuart Morgan]

Landed on trunk, branch landing coming soon.

Comment 7

[Stuart Morgan]

Landed on MOZILLA_1_8_BRANCH as well.

Comment 8

[Stuart Morgan]

Follow-up crash fixer checked in on both as well (tracking rect removal when the tab views will be removed from the window).