160720 - Ability to rearrange/reorder tabs by drag and drop

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement, P1)

Description

[sairuh (rarely reading bugmail)]

if you had multiple tab browsers in a window, it'd be nifty to be able to
rearrange their order, eg, via drag'n'drop.

Comment 1

[Steve Smart]

How about we have command-clicking allow you to move tabs, just like it allows
you to move toolbar items and menu extras. Modifier key goodies like this are a
good way to incorporate features like this that would otherwise be too
fluff-like to get a command, and they're nice about being unobtrosive and fun to
discover. This will probably require a lot of work, however, so I'm assuming
this would be far future, probably even post 1.0.

Comment 2

[Greg K.]

I think I'd prefer to simply be able to click and drag them. They're not like
the toolbar, where you're likely to set it once and leave it for a while. You
might want to re-order your tabs much more often.

Comment 3

[Greg K.]

Does this bug have a Mozilla analog?

Comment 4

[Greg K.]

Ah, answered my own question: it's bug 105885. (No, this isn't a dup.)

Comment 5

[Simon Fraser [no longer active]]

Mine

Comment 6

[Kenny MacLeod]

I agree with this feature request.  I recently switched from Netcaptor, which is a wrapper around IE which adds (amongst other things) tabbed browsing, and I really miss the tab drag-and-drop.

Comment 7

[Stephane Moureau]

See also 159510

Comment 8

[Andrew Hagen]

This is a Chimera bug, not Mozilla, but it still looks like a duplicate of bug
105885.

Comment 9

[Stephane Moureau]

Should we keep this bug open if RFE bug 159510 comment 42 is implemented, see
bug 159510 comment 30 ?

Comment 10

[Andrew Hagen]

This bug is marked as a Chimera bug, but its implementation would not be
Chimera-specific. Dupe of bug 105885.

*** This bug has been marked as a duplicate of 105885 ***

Comment 11

[Greg K.]

I don't think this is a duplicate. Chimera's tabs implementation has nothing
whatsoever to do with Mozilla's.

Comment 12

[sairuh (rarely reading bugmail)]

actually, the implementation for chimera is specific to chimera. the code
differs from tabbed browsing in mozilla. reopening.

Comment 13

[Sam "SammyTheSnake" Penny]

This is clearly a dup of Bug105885, anyone with permissions care to mark it as such?

Comment 14

[Sam "SammyTheSnake" Penny]

This is clearly a dup of Bug105885, anyone with permissions care to mark it as such?

Comment 15

[Stuart Morgan]

> This is clearly a dup of Bug105885, anyone with permissions care to
> mark it as such?

No, see comments 11 and 12.

Comment 16

[Ludovic Hirlimann [:Usul]]

adding Geoff because he's the tab guy now.

Comment 17

[Geoff "temporarily off bugmail" Beier]

Taking.

Comment 18

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

I'd love to see it, but it's not a show-stopper for 1.0 and depending on how
hard it is may not get fixed for 1.1. Should be in 1.2, however.

Targeting for 1.2.

Comment 19

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

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

Comment 20

[Jaime Mitchell (use [e-mail] for email)]

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

Comment 21

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

Tweaking summary to see if we can catch more dupes...

Comment 22

[Chris Lawson (gone)]

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

Comment 23

[Jasper]

When Geoff and I worked on the current tab implementation we also discussed the drag around implementation and we both agreed that doing it exactly how the bookmarks bar behaves in Safari would be the best.

This would mean that the tab can only be draged horizontally. The other tabs would move behind it according to the forground tabs location.

The drag would be triggered by a) click and hold in the centre part of the tab, and by holding down the command key and dragging, just like the toolbar.

Comment 24

[Desmond Elliott]

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

Comment 25

[Desmond Elliott]

As I get closer to something that I can submit as a patch for this, how happy are people with tab dragging behaving in a similar way to bookmark dragging behaves in Safari?

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

For those of you who do not have access to Safari.

Comment 26

[Mike Pinkerton (not reading bugmail)]

i dig it.

Comment 27

[Jon Hicks]

This is exactly the action I was hoping for. The way that surrounding elements move to accommodate is so much better than the 'stiff' action of Firefox.

Comment 28

[Desmond Elliott]

Attachment: WIP1

Please don't kill me Stuart!

I know that this is going to be painful for you to read, but I wanted to get something in before I went away for the weekend. I felt it was crucial.

And may I be the first one to say "It's about bloody time, Desmond".

Comment 29

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

Comment on attachment 233239 [details] [diff] [review]
WIP1

Retargeting review request to the correct smorgan.

Comment 30

[Stuart Morgan]

Comment on attachment 233239 [details] [diff] [review]
WIP1

Some quick notes:

>+    int i = [[[sender draggingPasteboard] stringForType:@"CaminoTabViewItemIndex"] intValue];
>+    int j = [mTabView indexOfTabViewItem:[button tabViewItem]];

i and j are not acceptable names for anything other than simple loop variables; these need to be meaningfully named. Also, you need to use your pasteboard constant everywhere, not the string.

>+      NSArray *array = [NSArray arrayWithArray:[mTabView tabViewItems]];
>+      BrowserTabViewItem* temp = [array objectAtIndex:j]; //The item we are holding in the pasteboard
>+      BrowserTabViewItem* temp2 = [array objectAtIndex:i]; //The item we are replacing.

Any of these that remain in the next round (see below) need meaningful names as well. I'm not clear on what you mean by "replacing" in the comment; isn't this rearranging tabs, not replacing existing tabs?

>+      // Because the NSTabView's internal array of tab view items is not 
>+      // a mutable array, each item is removed and then re-added.

Given that you can remove arbitrary tabs and insert tabs at arbitrary indexes, I don't see why you would need to do this.

>+      if (j == i+ 1 || j == i -1) {
>+        // Simple drag from one tab to an adjacent tab.
...
>+      }
>+      else {
>+        if ( j > i ) {
>+          // Dragging from somewhere to the left to somewhere at the right
>+          // Still need to write this
>+        }
>+        else {
>+          // Dragging from somewhere at the right to somewhere at the left
>+          // Still need to write this
>+        }
>+      }
>+    }

Why are these all branches?  How is dragging left different from dragging right, and dragging one place over different than dragging two+?  It seems like all you should have to do is remove the tab that is being dragged and insert it into its new location.

>+    // Because I remove the tab view array and rebulid it, the value of
>+    // NSSelectedTab is fubared, this resets it.
>+    while ([mTabView indexOfTabViewItem:[mTabView selectedTabViewItem]] != j) {
>+      [mTabView selectNextTabViewItem:sender];
>+    }

What's wrong with selectTabViewItemAtIndex:?

>+      BrowserTabView *tmp = [mTabViewItem tabView];
>+      BrowserTabViewItem *tmp2 = [tmp selectedTabViewItem];
>+      int n = [tmp indexOfTabViewItem:tmp2];

Names again. Also, don't bother storing things you are only using as intermediaries (i.e., if you rename tmp tabView, then the last two lines are just |int n = [tabView indexOfTabViewItem:[tabView selectedTabViewItem];|)

>+      [pboard setString:[[NSNumber numberWithInt:n] stringValue] forType:@"CaminoTabViewItemIndex"];

Just use a string formatter with %d rather than round-tripping through NSNumber.


I'll take a closer look at this once it's a little more completely implemented.

Comment 31

[Desmond Elliott]

Attachment: WIP2

Revision 2 of this patch, much neater and much faster.

Built against trunk from last night. And it does compile and run!

I will spend this afternoon working on a custom class, similar to TruncatingTextAndImageCell, that changes the dragged image to look like the tab that they are dragging. When that stumps me I will work on dragging from one window into another - I've got a good idea of how to do this already.

Something worth noting: this does not work with tabs in the overflow menu. This is something that we will need to discuss and I think it could be the cause of yet another Camino war.

Comment 32

[Stuart Morgan]

Comment on attachment 233556 [details] [diff] [review]
WIP2

The code is much saner now, but there are a number of behavioral issues (some very serious; some less so):
- Dragging into the bookmark bar or the empty tab area raises an exception and destroys the tab (dragging to the empty area should move to the end, dragging to anywhere outside the tab bar should do nothing)
- The changes are all live, which means that if the user cancels the drag the changes happen anyway
- Bad Things (tm) happen when you drag to another window's tab bar; that should be expressly forbidden for now (since the inter-window dragging will presumably come after this patch)
- The tab bar doesn't act like it accepts drags, in that the snap-back animation always happens
- If the mouse dips into the content area while rearranging, tabs can wedge with the highlighted appearance
- Starting a drag selects the tab, which is disconcerting if you try to drag a background tab.
- There really needs to be some animation to make it more evident what's happening
- The drag image needs to look like a tab, as discussed above.


(In reply to comment #31)
> When that stumps me I will work on dragging from one
> window into another - I've got a good idea of how to do this already.

Given that you can have useful tab re-arranging without having inter-window dragging, but you can't have inter-window dragging without having this foundation work complete, I'd recommend not worrying about that at all for the moment.

> Something worth noting: this does not work with tabs in the overflow menu. This
> is something that we will need to discuss and I think it could be the cause of
> yet another Camino war.

When the scroll arrows for the tab bar are in, hovering on them should scroll the tab bar; I don't think there will need to be a war about that.

Comment 33

[Desmond Elliott]

(In reply to comment #32)
> - Dragging into the bookmark bar or the empty tab area raises an exception and
> destroys the tab (dragging to the empty area should move to the end, dragging
> to anywhere outside the tab bar should do nothing)

WIP3 attempts to resolve this problem by adding an additional clause to |shouldAcceptDrag|

> - The changes are all live, which means that if the user cancels the drag the
> changes happen anyway

Yes, that should return the tab to it's original position but it could be fun to try and make it do that.

> - Bad Things (tm) happen when you drag to another window's tab bar; that should
> be expressly forbidden for now (since the inter-window dragging will presumably
> come after this patch)

Don't you mean Really Bad Things (tm) ? 

> - The tab bar doesn't act like it accepts drags, in that the snap-back
> animation always happens

|shouldAcceptDrag| related.

> - If the mouse dips into the content area while rearranging, tabs can wedge
> with the highlighted appearance
> - Starting a drag selects the tab, which is disconcerting if you try to drag a
> background tab.

I need to think about how to allow a combination of mouseDown and mouseDragged on a background tab to not make that tab foreground.

> - There really needs to be some animation to make it more evident what's
> happening

Working on animatinig the movement of the TabButtonCells.

> - The drag image needs to look like a tab, as discussed above.

The method that creates the image that represents the tab button returns void... I'll probably just draw a blank rectangle in the time being.

> Given that you can have useful tab re-arranging without having inter-window
> dragging, but you can't have inter-window dragging without having this
> foundation work complete, I'd recommend not worrying about that at all for the
> moment.

Ok, taken on board.

> > Something worth noting: this does not work with tabs in the overflow menu. This
> > is something that we will need to discuss and I think it could be the cause of
> > yet another Camino war.
> 
> When the scroll arrows for the tab bar are in, hovering on them should scroll
> the tab bar; I don't think there will need to be a war about that.

Yup, but until then I thought I'd post this notice.

I'd expect to see WIP3 before the end of Thursday.

Comment 34

[Simon Fraser [no longer active]]

(In reply to comment #33)
> (In reply to comment #32)

> > - The changes are all live, which means that if the user cancels the drag the
> > changes happen anyway
> 
> Yes, that should return the tab to it's original position but it could be fun
> to try and make it do that.

The approach you need to take is to make all of the tab-moving-while-dragging "fake", and only commit the change when the user drops.

Comment 35

[Desmond Elliott]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> 
> > > - The changes are all live, which means that if the user cancels the drag the
> > > changes happen anyway
> > 
> > Yes, that should return the tab to it's original position but it could be fun
> > to try and make it do that.
> 
> The approach you need to take is to make all of the tab-moving-while-dragging
> "fake", and only commit the change when the user drops.

Thanks for the suggestion, I'm working on making sure that the tab that is being dragged is only 'fake' dragged while |draggingUpdated| and the move is permanent only when |performDragOperation| is called.

I should have WIP3 out by Sunday evening. I will not be focusing on any dragging between window issues.

Comment 38

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

Setting priority per 1.6 roadmap.

Comment 39

[Jeff Dlouhy]

Attachment: WIP 3

I decided to scrap the code in the previous WIP. Some of the code there might be needed for when we want to drag tabs across windows. I wrote this way of dragging tabs in the last day and a half, so I may have left some unused variables in.

[KNOWN ISSUES]
 * Dragging past furthest tab on the right moves it to index 0
 * Titles and close buttons draw over dragging tab
 * Cannot drag background tabs
 * Does not get me dates
 * Anything I forgot :P

[TODO]
 * Handle dragging with 'tab scrolling'
 * Animation

Comment 40

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

* Completely breaks dragging site icons :(

* When dragging, the tab to the left of the dragged-away tab becomes two tab-widths wide (i.e., the tab-to-left loses its right edge).  This is an artifact of the way we draw tabs, iirc, but odd nonetheless.

* We probably need to try to do something like Firefox does, with a drop indicator (thing moving a bookmark on the bookmark bar) so you can tell where the tab will end up.  I often end up moving a tab two tabs over because I can't tell where the threshold is for 1 vs 2 (and it seems to be inconsistent :( ).

* Can't drag out of the tab bar (even if you intend to land there again).  Odd, but maybe not a big deal; dunno.

All in all, I'm very impressed :D  It's quite fun to play with!  Sorry I can't get you dates, though; another one of our developers runs the matchmaking service :(

Comment 41

[froodian (Ian Leue)]

In case you haven't seen it, there's some insertion-point logic in BookmarkToolbar - perhaps some of that can be reused, or just plain lifted.

Comment 42

[Jeff Dlouhy]

(In reply to comment #40)
> * Completely breaks dragging site icons :(

Fixed for next WIP

> * We probably need to try to do something like Firefox does, with a drop
> indicator (thing moving a bookmark on the bookmark bar) so you can tell where
> the tab will end up.  I often end up moving a tab two tabs over because I can't
> tell where the threshold is for 1 vs 2 (and it seems to be inconsistent :( ).

Once we get animations the tabs will move out of the way of where the tab might be going. I think the tab shuffle will be enough to convey what is happening to the user. I will have a non animation version of this in the next WIP.

> * Can't drag out of the tab bar (even if you intend to land there again).  Odd,
> but maybe not a big deal; dunno.

My first attempt at this did just that; however, it was very kludgy. The code is set up so that we can have a clause for when the initial y click point is past the kTabDragThreshold we can initiate a drag between windows, where we would want the tab to leave the tabBarView. I think if we allow the tab to be dragged out of the bar without allowing dragging between windows users will complain that tab dragging is not working as they expect.

Comment 43

[Jeff Dlouhy]

Attachment: WIP 4

Another work in progress patch, this code still contains some stray code and should not be considered final. The main issue I am facing at the moment is how a tabs subviews will draw outside of a clipping region. A video of the current implementation can be found at http://drop.jeffd.org/dragr.mp4.

Instead of writing my own animation class I chose to go with GrowlAnimation, which is a NSAnimation substitute that works on 10.3.

[KNOWN ISSUES]
 * Close button and spinner subviews draw outside of clip area
 * Dragging offscreen and back again causes pointer to be off of dragging tab

[TODO]
 * Handle dragging to overflow tabs
 * Handle button borders better when animating and moving

Comment 44

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

Tossing over my comments from irc:

1) Missing a newline at the end of TabButtonView.h

2) The Growl files will all need proper license headers (and we'll have to do a license dance again when this lands)

3) I can't get tabs to actually move out of the way of a dragged tab, or a drag to "stick" and instead hit:
Dec 22 01:33:02 Qalaat-Samaan Camino[86534]: *** -[BrowserTabView swapTabViewItem:withItem:withOffset:]: unrecognized selector sent to instance 0x106001e0

Comment 45

[Jeff Dlouhy]

Attachment: WIP 4 Take 2

Forgot to diff BrowserTabView ... woops

Comment 46

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

Slick!

The only new thing I've noticed is that there's a small area between the site icon/globe and the close button that allows you to both drag the site icon and drag the tab, and tabs dragged this way will eventually turn into site icon drags and leave the tab dragged away a little bit (instead of snapping back into place).  It's a hard-to-trigger edge case.

Comment 47

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

The other, rather unfortunate problem with the current patch is that it won't build with gcc 3.3/10.3.9 SDK: http://pastebin.mozilla.org/277934

I expect Jeff will be on that Wednesday or so ;)

Comment 48

[philippe (part-time)]

Camino trunk, Xcode 2.5/GCC4.01/10.4.11ppc isn't happy either: http://pastebin.mozilla.org/278233

Comment 49

[Nick Kreeger]

(In reply to comment #47)
> The other, rather unfortunate problem with the current patch is that it won't
> build with gcc 3.3/10.3.9 SDK: http://pastebin.mozilla.org/277934
> 
> I expect Jeff will be on that Wednesday or so ;)
> 


Without compiling, just eyeing on my way out the door:

+- (void) drawFrame:(GrowlAnimationProgress)progress {
+  [self drawTransitionWithView:view progress:progress];
+}

Just rename the in-param to something like |inProgress|:


+- (void) drawFrame:(GrowlAnimationProgress)inProgress {

Comment 50

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

Attachment: WIP 4 Take 2 with 10.3/gcc 3.3 fixes

This is a patch with the fixes Jeff had me make to get PPC 10.3/gcc 3.3 building.

Comment 51

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

Comment on attachment 294328 [details] [diff] [review]
WIP 4 Take 2

Håkan, might you have some time to go through this?  It's a bit of a large patch, but we're trying to avoid piling any more big patches into smorgan's queue atm.

Comment 52

[Håkan Waara]

Comment on attachment 294618 [details] [diff] [review]
WIP 4 Take 2 with 10.3/gcc 3.3 fixes

First and foremost: we _have_ cleared with the Growl people that we can use (and re-license to tri-license?) their NSAnimation replacement stuff, right?

>Index: src/browser/TabButtonView.h
>===================================================================

>+@class GrowlSlidingViewTransition;
>+
>+typedef enum {
>+  TabButtonAnimationLeft,
>+  TabButtonAnimationRight,
>+  TabButtonAnimationStopped
>+} TabButtonAnimationDirection;

I was going to suggest a "k" prefix for these (for constant), but I notice that we use "e" as a prefix for most enums elsewhere, so please continue that.

>Index: src/browser/TabButtonView.mm
>===================================================================


>+- (NSRect)clippingRect
>+{
>+  if ([mTabViewItem tabState] != NSSelectedTab) {
>+    BrowserWindowController* bwc = (BrowserWindowController*)[[self window] windowController];
>+    BrowserTabViewItem* selectedTab = (BrowserTabViewItem*)[[bwc tabBrowser] selectedTabViewItem];
>+    NSRect selectedTabFrame = [[selectedTab buttonView] frame];
>+
>+    if (NSIntersectsRect([self frame], selectedTabFrame)) {
>+      NSRect slice;
>+      NSRect remainder;
>+      NSRect intersection = NSIntersectionRect([self frame], selectedTabFrame);
>+      
>+      NSRectEdge edge = NSMinX(intersection) > NSMinX([self frame]) ? NSMaxXEdge : NSMinXEdge;
>+      NSDivideRect([self bounds], &slice, &remainder, NSWidth(intersection), edge);
>+      
>+      NSRect buttonRect = [mCloseButton frame];
>+      NSRect thisFrame = [self frame];
>+
>+      return remainder;
>+    }    
>+  }
>+
>+  return [self bounds];
>+}

This method name is not all that clear. "clipping" relative to what? If I understand it correctly, it's the rect of a currently-dragged-over tab that is covered by the dragged tab. Please put a one-line comment there to quickly clarify. Also, a light comment or two in the logic would help make it possible to understand it quicker.

>+- (void)shiftTabTo:(NSRect)newRect
>+{
>+  NSRect curFrame = [self frame];
>+  
>+  if (!NSEqualRects(curFrame, newRect)) {
>+    if (!mShiftAnimation) {
>+      mShiftAnimation = [[GrowlSlidingViewTransition alloc] initWithView:self];
>+      [mShiftAnimation setDuration:0.75];
>+    }
>+    
>+    TabButtonAnimationDirection newDir = newRect.origin.x > curFrame.origin.x ? TabButtonAnimationRight : TabButtonAnimationLeft;
>+    
>+    // If the direction has changed, then we can move it to the new positon
>+    if (newDir != [self animationDirection]) {
>+      [mShiftAnimation slideFrom:curFrame.origin to:newRect.origin];
>+      mAnimationDirection = newDir;
>+    }
>+  }
>+}

Is "shift" more like "slide" or "animate"?

That mShiftAnimation seems to be leaking, which would also make the tab views leak (the transition object is holding a reference to them), which in turn would make lots of other bigger stuff leak.

>+
> #pragma mark -
> 
> - (void)viewWillMoveToWindow:(NSWindow*)window
> {
>   [self removeTrackingRect];
>   [super viewWillMoveToWindow:window];
> }
> 
>@@ -427,23 +492,26 @@
> 
> - (void)mouseUp:(NSEvent *)theEvent
> {
>   if (mSelectTabOnMouseUp) {
>     [[NSNotificationCenter defaultCenter] postNotificationName:kTabWillChangeNotifcation object:mTabViewItem];
>     [[mTabViewItem tabView] selectTabViewItem:mTabViewItem];
>     mSelectTabOnMouseUp = NO;
>   }
>+  [super mouseUp:theEvent];
> }
> 
> - (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];
> 
>+  [super mouseDragged:theEvent];
>+

Any reason why the event is passed here and not the very beginning method? Also, should the event be forwarded even when the if-statement below returns early?

>   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;
> 
>Index: src/browser/BrowserTabBarView.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.h,v
>retrieving revision 1.5.2.7
>diff -u -8 -r1.5.2.7 BrowserTabBarView.h
>--- src/browser/BrowserTabBarView.h	16 Dec 2007 01:17:30 -0000	1.5.2.7
>+++ src/browser/BrowserTabBarView.h	27 Dec 2007 01:05:10 -0000
>@@ -50,16 +50,21 @@
>   IBOutlet BrowserTabView*  mTabView;
>   
>   NSButton*         mOverflowRightButton; // button to slide tabs to the left
>   NSButton*         mOverflowLeftButton;  // button to slide tabs to the right
>   NSButton*         mOverflowMenuButton;  // button to popup the tab menu
>   
>   // drag tracking
>   BOOL              mDragOverBar;
>+  BOOL              mDragStarted;         // whether or not a user has started to drag a tab
>+  NSPoint           mLastClickPoint;
>+  NSPoint           mLastDragPoint;
>+  NSPoint           mInitialDragOrigin;
>+  TabButtonView*    mDraggingTab;

Is mDraggingTab the currently-dragged tab? "mDraggedTab" ? Otherwise it sounds almost like a boolean.

Can you clarify mLastDragPoint which is not self-explaining (in connection with mInitialDragOrigin)?

It was some time since I worked with cocoa drag and drop; but isn't the starting drag point available from some global place?

>Index: src/browser/BrowserTabBarView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.mm,v
>retrieving revision 1.11.2.20
>diff -u -8 -r1.11.2.20 BrowserTabBarView.mm
>--- src/browser/BrowserTabBarView.mm	16 Dec 2007 00:20:40 -0000	1.11.2.20
>+++ src/browser/BrowserTabBarView.mm	27 Dec 2007 01:05:13 -0000
>@@ -196,16 +196,75 @@
>   }
>   else if (elapsedTime < doubleClickTime) {
>     mLastClickTime = 0;
>     [[NSNotificationCenter defaultCenter] postNotificationName:kTabBarBackgroundDoubleClickedNotification
>                                                         object:mTabView];
>   }
> }
> 
>+-(void)mouseUp:(NSEvent*)theEvent
>+{
>+  mDragStarted = NO;
>+  mDraggingTab = nil;
>+  [self rebuildTabBar];
>+}
>+
>+-(void)mouseDragged:(NSEvent*)theEvent
>+{
>+  NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>+  TabButtonView* clickedTabButton = [self buttonAtPoint:clickPoint];
>+
>+  if (!mDragStarted && ((abs((int)(mLastClickPoint.x - clickPoint.x)) >= kTabDragThreshold) ||
>+                        (abs((int)(mLastClickPoint.y - clickPoint.y)) >= kTabDragThreshold))) {
>+    mDragStarted = YES;
>+    mDraggingTab = clickedTabButton;
>+    mLastDragPoint = mInitialDragOrigin = clickPoint;      
>+  }
>+

No carriage return here please.

>+  else if (mDragStarted && mDraggingTab) {

>Index: src/browser/BrowserTabView.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabView.h,v
>retrieving revision 1.10.2.12
>diff -u -8 -r1.10.2.12 BrowserTabView.h
>--- src/browser/BrowserTabView.h	14 Dec 2007 00:35:22 -0000	1.10.2.12
>+++ src/browser/BrowserTabView.h	27 Dec 2007 01:05:16 -0000
>@@ -81,9 +81,11 @@
> 
> // get and set the "jumpback tab", the tab that is jumped to when the currently
> // visible tab is closed. Reset automatically when switching tabs or after
> // jumping back. This isn't designed to be a tab history, it only lives for a very
> // well-defined period.
> - (void)setJumpbackTab:(BrowserTabViewItem*)inTab;
> - (BrowserTabViewItem*)jumpbackTab;
> 
>+- (void)swapTabViewItem:(NSTabViewItem*)tabViewItem1 withItem:(NSTabViewItem*)tabViewItem2 withOffset:(int)tabOffset;

"Swap" I understand, but what does the "offset" mean in this context? Maybe you can clarify the method name or add a one-line comment?

>+
> @end
>Index: src/browser/BrowserTabView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabView.mm,v
>retrieving revision 1.31.2.27
>diff -u -8 -r1.31.2.27 BrowserTabView.mm
>--- src/browser/BrowserTabView.mm	14 Dec 2007 00:35:22 -0000	1.31.2.27
>+++ src/browser/BrowserTabView.mm	27 Dec 2007 01:05:18 -0000
>@@ -171,16 +171,65 @@
> {
>   // inserting a new tab means clearing the jumpback tab.
>   [self setJumpbackTab:nil];
>   
>   [super insertTabViewItem:tabViewItem atIndex:index];
>   [self showOrHideTabsAsAppropriate];
> }
> 
>+- (void)swapTabViewItem:(NSTabViewItem*)tabViewItem1
>+               withItem:(NSTabViewItem*)tabViewItem2
>+             withOffset:(int)tabOffset
>+{
>+  NSLog(@"swap");

This log is left, and some other ones below.

>+  int animIndex = [self indexOfTabViewItem:tabViewItem1];
>+  int endIndex = [self indexOfTabViewItem:tabViewItem2];
>+  
>+  BOOL shiftTabsRight = ((animIndex - endIndex) > 0);
>+  
>+  // Make sure we skip ourselves
>+  if (shiftTabsRight)
>+    animIndex--;
>+  else
>+    animIndex++;
>+  
>+  while (shiftTabsRight && animIndex >= endIndex) {
>+    NSLog(@"Moving Right Swapping Item: %d", animIndex);
>+    BrowserTabViewItem* curItem = (BrowserTabViewItem*)[self tabViewItemAtIndex:animIndex];
>+    TabButtonView* curView = [curItem buttonView];
>+    
>+    NSRect newRightFrame = [curView frame];
>+    float leftMargin = [mTabBar leftMargin];
>+    newRightFrame.origin.x = ((animIndex - tabOffset) + 1) * newRightFrame.size.width + leftMargin;
>+    [curView setDrawsDivider:YES];
>+    [curView shiftTabTo:newRightFrame];
>+    animIndex--;
>+  }
>+  
>+  while (!shiftTabsRight && animIndex <= endIndex) {
>+    NSLog(@"Moving Left Swapping Item: %d", animIndex);
>+    BrowserTabViewItem* curItem = (BrowserTabViewItem*)[self tabViewItemAtIndex:animIndex];
>+    TabButtonView* curView = [curItem buttonView];
>+    
>+    NSRect newLeftFrame = [curView frame];
>+    float leftMargin = [mTabBar leftMargin];
>+    newLeftFrame.origin.x = ((animIndex - tabOffset) - 1) * newLeftFrame.size.width + leftMargin;
>+    [curView setDrawsDivider:YES];
>+    [curView shiftTabTo:newLeftFrame];
>+    animIndex++;
>+  }
>+  
>+  [super removeTabViewItem:[tabViewItem1 retain]];
>+  [super insertTabViewItem:tabViewItem1 atIndex:endIndex];
>+  [super selectTabViewItem:tabViewItem1];
>+  
>+  [tabViewItem1 release];
>+}

Comments guiding the flow in the above two loops, please!

Comment 53

[Håkan Waara]

Comment on attachment 294328 [details] [diff] [review]
WIP 4 Take 2

Cancelling review requests on old patch.

Comment 54

[Jeff Dlouhy]

Attachment: WIP 4 Take 3

A quick update to the WIP. Addresses Håkan's concerns in comment #52. I have added some comments as well as removed some code / variables that were lying around.

I will quickly follow up with a WIP 5 that will address some of the current drawing issues.

Comment 55

[Jeff Dlouhy]

Attachment: WIP 5

As per discussion on IRC. This patch will only cover dragging and animation without tabs scrolling when the user drags them to the far left or right. My fix/hack for the clipping of subviews issue (where the close button would bleed through) was to remove the close button when another tab was over it and draw just the image of a button.

[KNOWN ISSUES]
 * Mouse position is off original position when dragged out of window while dragging.

[TODO]
 * Handle dragging to overflow tabs (MOVE TO ANOTHER BUG)
 * Handle button borders better when animating and moving (PARTIALLY FIXED)

Comment 56

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

(In reply to comment #55)
>  * Handle dragging to overflow tabs (MOVE TO ANOTHER BUG)

Bug 412861.

Comment 57

[Jeff Dlouhy]

Attachment: TabDnD

Comment 58

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

Comment on attachment 298492 [details] [diff] [review]
TabDnD

WRT the licenses on the Growl files, Toolkit just landed the files as-they-were, in a /growl/ subfolder.

So we'll just need to patch about:license; I'll open a bug on that.

Comment 59

[Jeff Dlouhy]

Comment on attachment 298492 [details] [diff] [review]
TabDnD

Håkan Waara (away until beginning of February) ... Hopefully Murph is around till the beginning of February. ;)

Comment 60

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

(In reply to comment #58)
> (From update of attachment 298492 [details] [diff] [review])
> WRT the licenses on the Growl files, Toolkit just landed the files
> as-they-were, in a /growl/ subfolder.
> 
> So we'll just need to patch about:license; I'll open a bug on that.

Bug 413686.

Comment 61

[Sean Murphy]

(In reply to comment #59)
> (From update of attachment 298492 [details] [diff] [review])
> Håkan Waara (away until beginning of February) ... Hopefully Murph is around
> till the beginning of February. ;)

Sure thing Jeff, just wanted to confirm that I am around and actively looking over the patch right now :)

Comment 62

[Sean Murphy]

Comment on attachment 298492 [details] [diff] [review]
TabDnD

Really awesome work here Jeff.  This is the first I really looked at the dragging code in detail, and I think it came together quite well.  In general, I did not notice any major problems, rather just a handful of code tweaks and a few behavioral issues we need to take a look at.

Also, we will need a project patch for this bug to incorporate the Growl Animation source files.

Before jumping into the code itself, there are a few glitches in the dragging behavior which I ran up against:

1. When dragging a TabButton rapidly off either edge of the TabBar, the button often appears like it cannot keep up with the mouse and falls short of resting at the furthest possible position.  (A visual demonstration of this: <http://www.seanmurph.com/camino/TabDnD-over-edge.mov>)

2. We only begin a drag if the tab button is the active tab.  Dragging tabs does not feel as natural this way - if I decide to move a background one I have to first select it, release the mouse, then click again and start dragging.  I think a better approach would be to active the tab immediately and begin a drag all under the same mouseDown: event.

3. The divider images (either left or right) of animating non-selected tabs are not drawn correctly sometimes.  Notice how the left divider of the middle tab goes missing here: <http://www.seanmurph.com/camino/TabDnD-left-divider.mov>.  In the first movie you can see a missing right divider as well.

4.  I found dragging to be unreliable when the tabs are scrolling - the animating background tabs frequently lose their place.  See some example problems here: <http://www.seanmurph.com/camino/TabDnD-when-scrolling.mov>

--

>  +-(void)animateVisibleTabs:(ETabButtonAnimationDirection)direction;

This method is not implemented or used anywhere.

>  +  // Start dragging once the mouse has moved at least kTabDragThreshold pixels in the x or y direction
>  +  if (!mDragStarted && ((abs((int)(mLastClickPoint.x - clickPoint.x)) >= kTabDragThreshold) || (abs((int)(mLastClickPoint.y - clickPoint.y)) >= kTabDragThreshold))) {
  
Long if condition here, so maybe throw in a few line breaks.

> +  if (NSPointInRect(clickPoint, [mOverflowRightButton frame])) {
> +    [mOverflowRightButton performClick:self];
> +  }
> +  else if (mDragStarted && mCurrentlyDraggingTab) {
> +    ...the actual dragging code here

It seems that calling [mOverflowRightButton performClick:self] is causing some trouble.  While dragging a tab, if the mouse moves over the right scroll button it will proceed to start clicking this scroll button repeadly.  Aside from the visual clicking appearance of the button, the actual scroll actions are not performed while the drag is still in session.  Subsequently, after the drag concludes, the tabs are finally scrolled all at once and the newly dropped tab (among others) rapidly jumps into an entirely different position than where it was released at.

> +    // Don't allow the tab to be dragged over the edges
> +    if (NSMinX(draggingRect) >= [self leftMargin] && (NSMaxX(draggingRect) <= [self rightMargin])) {
> +      [mCurrentlyDraggingTab setFrame:draggingRect];
> +    } else {
> +      return;
> +    }

I'd break the |setFrame:| call out of any condition, since it is the routine scenario, and re-work the if logic to return if the tab is out of bounds:

if (dragRect is out of bounds)
  return;

[mCurrentlyDraggingTab setFrame:draggingRect];

>  +    TabButtonView* buttonAtPoint = [self buttonAtPoint:draggingEdge];

Without reading some context code, it wasn't clear what the 'point' actually is; Maybe rename to better indicate which button this actually is, something like |buttonUnderDrag| for instance.

> +    NSRect intersection = NSIntersectionRect([mCurrentlyDraggingTab frame], [buttonAtPoint frame]);
> +    NSRect buttonFrame = [buttonAtPoint frame];
> +    NSRect clippingRect = NSMakeRect(NSMinX(buttonFrame), NSMinY(buttonFrame), NSMinX(intersection), NSHeight(buttonFrame));

clippingRect and buttonFrame are unused; later there are calls to [buttonAtPoint frame] instead of using the buttonFrame variable.  (There's no build warning for the unused variables here, which I guess could be caused by NSMakeRect being an inline function?).

> +      if (intersection.origin.x > [buttonAtPoint frame].origin.x && 
> +          [buttonAtPoint animationDirection] != eTabButtonAnimationRight)
> +        [mTabView swapTabViewItem:[mCurrentlyDraggingTab tabViewItem]
> +                         withItem:[buttonAtPoint tabViewItem]
> +                       withOffset:mLeftMostVisibleTabIndex];
> +
> +      else if (intersection.origin.x == [buttonAtPoint frame].origin.x && [buttonAtPoint animationDirection] != eTabButtonAnimationLeft)
> +      {
> +        [mTabView swapTabViewItem:[mCurrentlyDraggingTab tabViewItem]
> +                         withItem:[buttonAtPoint tabViewItem]
> +                       withOffset:mLeftMostVisibleTabIndex];
> +      }
> +    }

About the condition style here: add a line break to the "else if" to match the "if" above, and use braces.

Also, maybe add a comment to explain that the two different calls to swapTabViewItem: are there to switch the animation direction even during an existing side.  I had to look at that a few times to figure out why we couldn't just combine them under one ">=" condition and check [button isAnimating].

> +
> +- (float)leftMargin
> +{
> +  if (mOverflowTabs)
> +    return NSWidth([mOverflowLeftButton frame]);
> +  
> +  return kTabBarMargin;
> +}
> +
> +- (float)rightMargin
> +{
> +  if (mOverflowTabs)
> +    return NSWidth([self frame]) - (NSWidth([mOverflowRightButton frame]) + NSWidth([mOverflowMenuButton frame]));
> +  
> +  return NSWidth([self frame]) - kTabBarMargin;
> +}
> +

Until I looked at the implementation of these methods, I got the impression that they simply returned the width of each left/right margin.  It's not a big deal, but since they actually return the x location of where the margin begins, maybe indicating this in the name would help (something like |rightMarginLocation|, or even |maximumTabLocation|).

>  +- (void)swapTabViewItem:(NSTabViewItem*)tabViewItem1 withItem:(NSTabViewItem*)tabViewItem2 withOffset:(int)tabOffset;

I'd recommended dropping the |withOffset:| in favor of just |offset:|.  As with Håkan, I hard a tough time at first understanding what the purpose of this argument was.  Maybe another variable name for the argument or else a re-worded comment might help explain its use.  Regarding the need for the argument, the method seems to always be called with the same offset of |mLeftMostVisibleTabIndex| - I guess perhaps this will change when dragging among scrolled tabs is supported?

> +  // If we drag tab #4 to position #0 make sure tabs #3-0 shift to the right
> +  while (shiftTabsRight && animIndex >= endIndex) {
> +    BrowserTabViewItem* curItem = (BrowserTabViewItem*)[self tabViewItemAtIndex:animIndex];
> +    TabButtonView* curView = [curItem buttonView];
> +
> +    // Calculate the rect the tab should move to
> +    NSRect newRightFrame = [curView frame];
> +    // Is the overflow button on the left?
> +    float leftMargin = [mTabBar leftMargin];
> +    newRightFrame.origin.x = ((animIndex - tabOffset) + 1) * newRightFrame.size.width + leftMargin;
> +    [curView setDrawsDivider:YES];
> +    [curView animateToRect:newRightFrame];
> +    animIndex--;
> +  }
> +
> +  // Do the same as above but to the left
> +  while (!shiftTabsRight && animIndex <= endIndex) {
> +    BrowserTabViewItem* curItem = (BrowserTabViewItem*)[self tabViewItemAtIndex:animIndex];
> +    TabButtonView* curView = [curItem buttonView];
> +    
> +    NSRect newLeftFrame = [curView frame];
> +    float leftMargin = [mTabBar leftMargin];
> +    newLeftFrame.origin.x = ((animIndex - tabOffset) - 1) * newLeftFrame.size.width + leftMargin;
> +    [curView setDrawsDivider:YES];
> +    [curView animateToRect:newLeftFrame];
> +    animIndex++;
> +  }

There is a large block of repeated code here, except for the math used when setting the new frame's x location.  See if it's possible not to have so much repetition in the behavior performed here.  The differences in the while's condition and the x coordinate calculation might make consolidation tricky, but doing so is worth considering at least.

>  +  [super removeTabViewItem:[tabViewItem1 retain]];
>  ...
>  +  [tabViewItem1 release];

Just autorelease the retained tabViewItem1, as that's the technique we seem to prefer.

>  +  GrowlSlidingViewTransition*   mShiftAnimation;

Indicate that this variable is a strong reference.

>  - (void)stopLoadAnimation
>  {
>    [mProgressWheel stopAnimation:self];
>    [mLabelCell removeProgressIndicator];
>    [self setNeedsDisplayInRect:mLabelRect];
>  }
>
> +- (ETabButtonAnimationDirection)animationDirection
> +{
> +  if (![mShiftAnimation isAnimating])
> +    mAnimationDirection = eTabButtonAnimationStopped;
> +
> +  return mAnimationDirection;
> +}

This method is listed after two others named |stopLoadAnimation| and |startLoadAnimation|, both of which are unrelated to the slide transition.  Again, just a very minor nit, but to make its useage clear it might help to rename it to |slideAnimationDirection|.

> +//
> +// Get the selected tab from the TabBarView
> +//
> +- (NSRect)selectedTabFrame

This method seems like it might be more cohesive as part of TabBarView itself, or some parent view.  I think ideally the TabViewButtons should not have to worry about dealing with or even know anything about their siblings.

> +//
> +// Calculate the rect that is not being covered by the currently dragging tab
> +//
> +- (NSRect)visibleRectUnder:(NSRect)frontmostView

There might be a better name for this method which would really indicate what it's returning (without having to rely on the comment).  Also, I think the argument should have a better label preceding it (like ...UnderRect:) and a name that doesn't make it seem like a NSView object.

> +//
> +// Draw a button on this view so it will be clipped when drawn
> +//
> +- (void)drawFakeSubviews

The method name is generic, so the comment should probably not be too specific either about what it is drawing.  It might also be helpful to use the comment to explain why this method exists - that subviews will not be overlapped by a dragging tab unless they are composited directly onto the tab view.

|drawFakeSubviews| is also not declared anywhere, so you'll need to throw it in the private category in TabButtonView.mm.  (There's no warning now just on the off chance that the implementation is listed earlier in the file than the code calling it).

> +  if (!NSEqualRects(NSZeroRect, selectedTabFrame) && NSIntersectsRect([self frame], selectedTabFrame)) {
> +    NSRectClip([self visibleRectUnder:selectedTabFrame]);
> +    [self drawFakeSubviews];
> +    [mCloseButton removeFromSuperview];
> +  } else if (![mCloseButton isDescendantOf:self]) {
> +    [self addSubview:mCloseButton];
> +  }
> +

Line break before "else if."

I think that, instead of removing mCloseButton from the view hierarchy and then inserting it back in, we should just control its visibility via |setHidden:| and then checking |isHidden|.  Removing views can often cause problems maintaining the key-view loop, and also require that you ensure the view is retained somewhere.

> +- (void)animateToRect:(NSRect)newRect
> +{
> +  NSRect curFrame = [self frame];
> +  
> +  if (!NSEqualRects(curFrame, newRect)) {
> +    if (!mShiftAnimation) {
> +      mShiftAnimation = [[GrowlSlidingViewTransition alloc] initWithView:self];
> +      [mShiftAnimation setDelegate:self];
> +      [mShiftAnimation setDuration:0.75];
> +    }
> +    
> +    ETabButtonAnimationDirection newDir = newRect.origin.x > curFrame.origin.x ? eTabButtonAnimationRight : eTabButtonAnimationLeft;
> +
> +    // If the direction has changed, then we can move it to the new positon
> +    if (newDir != [self animationDirection]) {
> +      [mShiftAnimation slideFrom:curFrame.origin to:newRect.origin];
> +      mAnimationDirection = newDir;
> +    }
> +  }
> +}

The entire behavior of this method is inside an if block, so maybe just check for your condition and return early if the actions are not needed:

if (NSEqualRects(curFrame, newRect))
  return;

...

Comment 63

[Jeff Dlouhy]

Attachment: Tab DnD v2

(In reply to comment #62)

> > +//
> > +// Get the selected tab from the TabBarView
> > +//
> > +- (NSRect)selectedTabFrame
> 
> This method seems like it might be more cohesive as part of TabBarView itself,
> or some parent view.  I think ideally the TabViewButtons should not have to
> worry about dealing with or even know anything about their siblings.
> 

I have not moved this to BrowserTabViewItem or the TabBarView. The whole purpose of this method is to access the BrowserTabView, which cannot be accessed without going through BWC. So if you think it should be moved to the BrowserTabViewItem, that is fine; however, it is only ever used in the TabBarView.

Other than that, I fixed most of the issues you brought up in the last review. I rewrote the mouse tracking code, as well as how frames are located for animation. The next frame is determined by storing the TabViewItem frame before the animation starts and getting the rect (offset by 1) when animating to it. Before I was determining the next frame based on the number of tabs and left margin width. This caused problems because the rightmost tabs have variable widths.

Let me know what you think.

Comment 64

[Sean Murphy]

Comment on attachment 301836 [details] [diff] [review]
Tab DnD v2

For the most part, everything is looking very good here.  I had to say r- for now, though basically because of the few remaining behavioral issues I ran into.  In general, the current code only has a few minor areas to tweak and the feature itself is real close.

Lingering areas of concern with overall behavior:

* The tab directly to the left of the currently dragging/selected one needs to draw its right divider, and vice versa.  See <http://seanmurph.com/camino/TabDrag-NoLeftDivider.jpg> and <http://seanmurph.com/camino/TabDrag-NoRightDivider.jpg>.

* Creating a new tab in the middle of an animation causes problems if the new tab will reduce the size of all tabs.

* Tabs can still become misplaced and overlap each other when rapidly dragging around a tab button.  You kinda have to be asking for this, however, and really moving things around recklessly.  See <http://seanmurph.com/camino/TabDnD-overlap.mov>

* The slide animations are a little bit choppy right now.  I'm not sure at the moment if running a non-optimized development build is the primary contributor.  If the smoothness is really an issue, we should look into addressing it at some point, whether that's right now or after we get the basic functionality in place.

> 2. We only begin a drag if the tab button is the active tab.  Dragging tabs
> does not feel as natural this way - if I decide to move a background one I have
> to first select it, release the mouse, then click again and start dragging.  I
> think a better approach would be to active the tab immediately and begin a drag
> all under the same mouseDown: event.

* Just to throw this in again from the previous review, I still do think it subtracts from the dragging usability.  It also reduces the obviousness that tabs are drag-able if only the selected one can be moved.

--

>+-(void)mouseDragged:(NSEvent*)theEvent

A style nit, throughout the patch check over the style of your conditional statements to make sure they match the project's: (http://wiki.caminobrowser.org/Development:Reviewing#Code_style>.  

This method has a few...

>+  if (!mDragStarted && ((abs((int)(mLastClickPoint.x - clickPoint.x)) >= kTabDragThreshold) ||
>+                        (abs((int)(mLastClickPoint.y - clickPoint.y)) >= kTabDragThreshold))) {
  
Not mentioned on the wiki, but multiline conditions have a different style and always throw the brace on a newline directly under the 'if':

if (foo &&
    bar)
{
  
}

Also, there are a few missing spaces before braces or parens.

>+  NSMutableArray* mSavedTabRects;
> ...
>+    mSavedTabRects = [[NSMutableArray alloc] init];
  
Mark as strong, and release in |dealloc|

>+- (void)swapTabViewItem:(NSTabViewItem*)tabViewItem1
>+               withItem:(NSTabViewItem*)tabViewItem2
>+                 offset:(int)tabOffset
                 
Just wanted to note that offset is unused as of yet.  I'm thinking that it will eventually come into play with dragging to scrolled tabs?

>+  while ((shiftTabsRight && animIndex >= endIndex) || (!shiftTabsRight && animIndex <= endIndex)) {
>+    BrowserTabViewItem* curItem = (BrowserTabViewItem*)[self tabViewItemAtIndex:animIndex];
>+    TabButtonView* curView = [curItem buttonView];
>+
>+    NSRect newFrame = [curView frame];
>+    // Is the overflow button on the left?
>+    //float leftMargin = [mTabBar leftMarginLocation];
>+    //float newWidth = NSWidth([curView frame]);
>+
>+    // Calculate the rect the tab should move to
>+    if (shiftTabsRight) {
>+      if ((unsigned)(animIndex + 1) <= [mSavedTabRects count])
>+        newFrame = [[mSavedTabRects objectAtIndex:(animIndex + 1)] rectValue];
>+
>+      animIndex--;
>+    } else {
>+      if ((animIndex - 1) >= 0)
>+        newFrame = [[mSavedTabRects objectAtIndex:(animIndex - 1)] rectValue];
>+
>+      animIndex++;
>+    }

To be extra safe, should we double check that mSavedTabRects does contain some frames, since this method assumes that |saveCurrentTabFrameLocations| was called before it?

>+      if ((animIndex - 1) >= 0)
>+        newFrame = [[mSavedTabRects objectAtIndex:(animIndex - 1)] rectValue];

We'll have to watch for exceptions here as well, making sure we always access a valid array index.  

>+    // Is the overflow button on the left?
>+    //float leftMargin = [mTabBar leftMarginLocation];
>+    //float newWidth = NSWidth([curView frame]);

Remove stray commented code.

>+    } else {

place a return before else {.

>+  [super removeTabViewItem:[tabViewItem1 retain]];
>+  [super insertTabViewItem:tabViewItem1 atIndex:endIndex];
>+  [super selectTabViewItem:tabViewItem1];
>+
>+  [tabViewItem1 autorelease];

Perform a [[tabViewItem1 retain] autorelease] instead, as that's what the rest of the project follows (which does prevent the autorelease from being unnoticed and unattached to any changes since it's down further).

>+// Save the frames of the visible tabs so we know
>+// where to move them to when animating
>+//
>+- (void)saveCurrentTabFrameLocations

While the comment indicates this stores only visible frames, this actually saves the frames of all tabs, visible or not.

>--(NSButton*)scrollButtonAtPoint:(NSPoint)clickPoint
>+-(NSButton*)scrollbuttonUnderDragPoint:(NSPoint)clickPoint

I would have probably left the name alone, as the method is used by other places than just dragging code.  In those other methods it might seem strange for them to be calling a method dealing with a drag point.  Plus, the dragging code calling scrollButtonAtPoint: seems coherent enough.

>+- (void)handleTabAnimationDidEnd
>+{
>+  [self rebuildTabBar];
>+}

This method is not used or declared anywhere.

Comment 65

[Jeff Dlouhy]

Attachment: Tab DnD v3

(In reply to comment #64)

> * The tab directly to the left of the currently dragging/selected one needs to
> draw its right divider, and vice versa.

This has been addressed. I was able to manage the left dividers without sending notifications and tried to find the best way to know when a tab has stopped animating and set the dividers accordingly.

There is now a method '- (void)setTabDividers' which is called every time a tab finishes animating and determine which dividers should be drawn. This might not be the best way to do this, but it was all I could think of.

> * Creating a new tab in the middle of an animation causes problems if the new
> tab will reduce the size of all tabs.

Moved to bug 418358.

> * Tabs can still become misplaced and overlap each other when rapidly dragging
> around a tab button.  You kinda have to be asking for this, however, and really
> moving things around recklessly.

My solution to this was to make sure the surrounding tabs are in the correct location when animating the other tabs. For example if we have tabs |1|2|3|4|5| and |3| swaps with |4| we go through tabs |2| and |1| and make sure they are in their last saved position, as well as tab |5|. This has seemed to stop *most* overlapping.

> > 2. We only begin a drag if the tab button is the active tab.  Dragging tabs
> > does not feel as natural this way - if I decide to move a background one I have
> > to first select it, release the mouse, then click again and start dragging.  I
> > think a better approach would be to active the tab immediately and begin a drag
> > all under the same mouseDown: event.
> 
> * Just to throw this in again from the previous review, I still do think it
> subtracts from the dragging usability.  It also reduces the obviousness that
> tabs are drag-able if only the selected one can be moved.

Moved to bug 418359.


In addition to the issues above, I fixed the other nits and concerns that were in the previous review.

Comment 66

[Sean Murphy]

Comment on attachment 304171 [details] [diff] [review]
Tab DnD v3

This is definitely an aspect of the patch I should have drawn attention to much sooner in the review process, but I am concerned about the way in which we actually move the currently dragged tab button around on screen.  Right now, we position the dragged tab merely with setFrame:, and it proceeds to overlap the other tab buttons.  Prior to 10.5, it is not considered safe to overlap sibling NSViews; instead you'd have to create an explicit view hierarchy.  All TabButtonViews are siblings under the parent BrowserTabBarView.  This overlapping danger is one reason why |drawCloseButtonImage| is needed...

"Note: For performance reasons, Cocoa does not enforce clipping among sibling views or guarantee correct invalidation and drawing behavior when sibling views overlap. If you want a view to be drawn in front of another view, you should make the front view a subview (or descendant) of the rear view."
From: <http://developer.apple.com/documentation/Cocoa/Conceptual/CocoaViewsGuide/WorkingWithAViewHierarchy/chapter_5_section_5.html>

It's my fault for just catching this now, and I apologize for failing to bring it up before you contributed so much work and time into the current approach.

If needed, an alternate plan of action might consist of the following:

1.  Allow the tab buttons to be dragged using the standard -[NSView dragImage:...] mechanism, called from inside BrowserTabBarView's mouseDragged:.

2. Control the bounds of the dragged image in BrowserTabBarView by responding to NSDraggingSource protocol methods (draggedImage:movedTo: or draggingUpdated:).

3. Perform setHidden:YES on the actual TabButtonView currently dragging, since the drag image is representing it now.

4. Introduce a method on BrowserTabBarView, named something like |updateTabDividers|, which will set tab dividers appropriately depending on whether there's a drag in session.  I think we should keep all tab divider logic in one class, instead of setting them in one class and reverting them in another as is done currently.  Having a separate method for dealing with dividers also keeps the dragging methods from growing too long and becoming hard to analyze.

5. In BrowserTabBarView, register for the custom 'tab button' drag type.  When NSDragDestination methods are received, instruct the necessary tabs to slide out of the way.

6. On concluding the drag operation, swap the actual BrowserTabViewItems into their proper place and perform setHidden:NO on the original tab button that was represented by the drag image.

7. Call |updateTabDividers| again and reset them to their normal condition.

I think most dragging behavior should exist in BrowserTabBarView, as the BrowserTabView doesn't really deal with TabButtonViews and shouldn't be responsible for dragging and sliding them into different positions.

--

Some other comments about code which will probably still exist unchanged:

We need to disable dragging if there is only one TabButtonView.  Right now, not only is it able to drag, but it throws some array out of bounds exceptions.

> +  // Draw the left divider when animating, but not when we end animating
> +  if ([mShiftAnimation isAnimating]) {
> +    [self setDrawsLeftDivider:NO];
> +    tabRect.size.width -= [sTabButtonDividerImage size].width;
> +    NSPoint dividerOrigin = NSMakePoint(NSMinX(tabRect), 0);
> +    [sTabButtonDividerImage compositeToPoint:dividerOrigin
> +                                   operation:NSCompositeSourceOver];
> +  }
> +
> +  if (mNeedsLeftDivider) {
> +    NSPoint dividerOrigin = NSMakePoint(NSMinX(tabRect), 0);
> +    [sTabButtonDividerImage compositeToPoint:dividerOrigin
> +                                   operation:NSCompositeSourceOver];
> +  }
> +
> +  if (mNeedsRightDivider) {
>      tabRect.size.width -= [sTabButtonDividerImage size].width;
>      NSPoint dividerOrigin = NSMakePoint(NSMaxX(tabRect), 0);
>      [sTabButtonDividerImage compositeToPoint:dividerOrigin
>                                     operation:NSCompositeSourceOver];
>    }
>
>    NSPoint patternOrigin = [self convertPoint:NSMakePoint(0, 0) toView:[[self window] contentView]];
>    if ([mTabViewItem tabState] == NSSelectedTab) {

In -[TabButtonView drawRect:], when determining whether to draw the dividers, we should probably just assume that when one is animating it should insert both (and cut down on some duplicate code):

if (mNeedsLeftDivider || [mShiftAnimation isAnimating]) {
  NSPoint dividerOrigin = NSMakePoint(NSMinX(tabRect), 0);
  [sTabButtonDividerImage compositeToPoint:dividerOrigin
                                 operation:NSCompositeSourceOver];
}

if (mNeedsRightDivider || [mShiftAnimation isAnimating]) {
  tabRect.size.width -= [sTabButtonDividerImage size].width;
  NSPoint dividerOrigin = NSMakePoint(NSMaxX(tabRect), 0);
  [sTabButtonDividerImage compositeToPoint:dividerOrigin
                                 operation:NSCompositeSourceOver];
}

> if ([mShiftAnimation isAnimating]) {

To encapsulate the logic about determining animation status, I think we should always call [self isSlideAnimating] to check this fact.  In case we change the animating classes around, we don't have to update all areas in which we directly access mShiftAnimation and depend upon its type.

> - (BOOL)isSildeAnimating;

Sorry I didn't notice this before, but slide is spelled wrong.

> - (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 ([self isSildeAnimating])
>+    return;
>+
>+  [super mouseDragged:theEvent];
>+

Dragging the proxy icon of the tabs is causing some trouble.  If you drag the icon of the selected tab, some dividers will appear incorrectly on its sides.  Dragging the proxy of a non selected tab exhibits the same divider problem, but goes a step further and becomes confused about what's dragging.  To reproduce: Open three tabs (to an actual page so the proxy is drag-able), select the third one, then begin to drag the proxy icon of the second tab and release it.  Now, open up a new tab and you'll see that it overlaps the third one.

This is caused because even when the proxy icon drag starts, TabButtonView still allows mouseDragged: to pass up to the BrowserTabBarView.  We do not want a tab drag to begin when the proxy icon is being clicked on, so in -[TabButtonView mouseDragged:] you should probably ensure mouseDragged: is not passed on if a proxy drag will ensue:

if (!NSPointInRect(localPoint, iconRect) || ![mTabViewItem draggable]) {
  [super mouseDragged:theEvent];
  return;
}

Also, you might want to move the first early return (isSlideAnimating) to the top of the method, grouping the two variables (iconRect, localPoint) closer to where they're actually being used.

Comment 68

[Sean Murphy]

Attachment: Patch v1 (New Approach)

Since I really believe this feature is important to us, and I did not want all of our other hard work to be overshadowed by the lack of such an ability (http://daringfireball.net/linked/2008/04/17/camino-16 :) ), I worked up an alternate approach building on some of the ideas I mentioned in the past reviews.

I avoided the overlapping views problem by using the standard -[NSView dragImage...] to drag the tab around on screen.  There is no way to constrain where that dragged image moves, however, so I actually drag an empty image and represent the dragged tab with a floating borderless window, which is repositioned based on the NSDraggingSource delegate methods.

Also included is my own implementation of an NSViewAnimation type class.  I found NSViewAnimation to be very slow, causing very jerky animations.  Furthermore, running the animation with a blocking mode of NSAnimationNonblockingThreaded caused the display to flash when updated, most likely meaning that it is not performing the UI updates on the main thread.  My CHViewAnimation class fixes both of those issues, so we have smoother animations that can work properly update the UI on a secondary thread if desired.

Comment 69

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

Comment on attachment 342016 [details] [diff] [review]
Patch v1 (New Approach)

Pretty slick :)

Alas, the tooltip thing we talked about is real, or something that looks the same is (at least in my debug build)--it occurs during dragging, not when dragging is done.

STR:

1) Start a drag
2) Drag one tab into the place that another tab occupies, but do not release the drag once the previous tab slides away.
3) Hover a bit
-->Observe the tooltip for the tab that slid away

There's another bug that's also a bit ugly but obscure:

STR: 

1) Start a drag but do not release
2) Switch apps (using Cmd-Tab) but do not terminate the drag.  Ideally, switch to an app that has a window covering the same space as the Camino tab bar
-->Observe dragged tab floating over top of the other app's window

What did we decide about dragging background tabs (Firefox 3 and Safari both can, with varying degrees of good timing required)? I'm fine with that being a follow-up; I just can't recall.

A few typos I spotted while browsing the patch:

>Index: src/browser/BrowserTabBarView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.mm,v

>+  // |mDraggedTab| will be released when it is removed from the superview,
>+  // but it is always retained by it's |BrowserTabViewItem|.

its

>+// Create a borderless floating window, with an image of the dragged tab as it's content view
>+// to move around on screen representing the dragged tab.

its

>+  // Animate the dragged tab to it's final destination.

its

>+  // If there is a tab to the right of the animated one, take away it'\s left divider.

its

>+  // Draw a left divider if this tab is to the right of the available tab spot.
>+  // (The drag may have ended before this tab's animation did, so also ensure there is a tab dragging)

Being really pedantic, that thought in parens is a full sentence, so it should end a period.

>+// Called when the dragging tab has been released and is finished animating into it's
>+// proper location.

its

>Index: src/extensions/CHSlidingViewAnimation.h
>===================================================================
>RCS file: src/extensions/CHSlidingViewAnimation.h

>+ * The Original Code is Camino code.\

I don't suppose we really need that "\" :-)

Comment 70

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

With just two tabs open, I'm finding that the dragged tab has a bit of a "ghost" tab when I drag it out towards the empty space on the bar (left tab) or when I drag it back to the spot where it originated (or to the left edge, for the right tab).  If you move "slowly" you also see a few other cases of incomplete tabs drawing.

I haven't been able to reproduce these with >2 tabs, just with 2.  It also looks like enabling "Tab bar: Always stays visible" prevents this wackiness.

Comment 71

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

(In reply to comment #69)
> What did we decide about dragging background tabs (Firefox 3 and Safari both
> can, with varying degrees of good timing required)? I'm fine with that being a
> follow-up; I just can't recall.

We in fact filed the follow-up; it's bug 418359.

Comment 72

[Stuart Morgan]

Comment on attachment 342016 [details] [diff] [review]
Patch v1 (New Approach)

Sorry I took so long :P

Looks and feels awesome in testing! A handful of things I noticed:
- If a tab closes during drag (whether it's the dragged tab or not), an array bounds exception is thrown. I think the expected behavior is that a background tab closing should Just Work seamlessly, and having the dragged tab close should gracefully cancel the drag (selecting either what would have been selected normally, or the tab after the current position; I'm not sure which makes more sense without trying). And I haven't tried yet, but I would guess that the saved frames would make results surprising if a new tab opened during drag...
- As with the switch-app case Smokey mentioned, you get bizarre results if a sheet comes down during drag, since it will likely be over the sheet location. It's going to interrupt the drag, which is okay, but the window level thing is odd. Can the tab window just be a child window of its associated window?
- Pressing esc to cancel the drag should return it to its original location, not drop it where it currently is (can be a follow-up if it's not easy, since that's going to be a rare case)
- The two-tabs behavior Smokey mentioned is pretty weird, so that should get fixed here as well.
- Dragging to the edge in tab-scroll setup needs to actually scroll. This should be a follow-up unless it's easy though.
- Dragging when there are scroll arrows causes the scroll arrows to lose their borders, which looks a bit odd at first glance; that too can be a follow-up if we decide it should behave the other way.



>+  BOOL              mTabDragIsInSession;

InSession is a bit odd; how about mCurrentlyDraggingTab?

>+  int               mAvailableTabIndex;   // The empty spot where the dragged tab will be inserted at.

"Available" isn't terribly clear. How about either mDropTargetIndex, or if you want to keep the idea of the empty space mEmptyPlaceholderIndex to be explicit about what this represents.

>+  TabButtonView*    mDraggedTab;
>+  NSWindow*         mDraggingTabWindow;

Why Dragged in one case, but Dragging in the other?

>+  float             mLocationOnTabButtonWhereDragBegan;

Location implies a point; mHorizontalGrabOffset?

>+-(int)lastVisibleTabIndex;

Call this rightMostVisibleTabIndex for parity with the left-tab setter (or even better, rightmostVisibleTabIndex and fix the caps in setLeftMost... that I missed at the time).

>+-(TabButtonView*)tabButtonAtIndex:(int)index;

Why add this method, but then not use it even in a lot of the new code?

>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(slidingTabAnimationEnded:) 
>+                                               name:@"TabButtonAnimationEnded"

This needs to be a constant in one place, not a string literal repeated in two places.

>+  [mSavedTabFrames release];

What about the other two object members you added? Even if you believe they are cleaned up elsewhere, release them here to make sure we don't accidentally start leaking windows or tabs at least beyond the window lifetime.

>+  if (mTabDragIsInSession) {
>+    [self drawTabBarBackgroundInRect:[self bounds] withActiveTabRect:NSZeroRect];
>+  }
>+  else {
>+    // determine the frame of the active tab button and fill the rest of the bar in with the background
>+    TabButtonView* activeTabButton = [(BrowserTabViewItem *)[mTabView selectedTabViewItem] buttonView];
>+    NSRect activeTabButtonFrame = [activeTabButton superview] ? [activeTabButton frame]
>+                                                              : NSMakeRect(0, 0, 0, 0);
>+    [self drawTabBarBackgroundInRect:[self bounds] withActiveTabRect:activeTabButtonFrame];
>+  }

Set up the rect in the if/else, and call drawTabBarBackgroundInRect: outside it rather than repeating the call.

>+  // Represent the dragged tab with a floating borderless window.

Put this comment several lines lower, with the code it refers to.

>+  // |mDraggedTab| will be released when it is removed from the superview,
>+  // but it is always retained by it's |BrowserTabViewItem|.

Why is this comment here? You retain it explicitly above, so it doesn't matter what BTVI does.

>+  // |screenPoint| seems to be an unreliable source

Unreliable how?

>+  NSPoint leftEdgeOfTabsRectInWindowCoords = [self convertPoint:[self tabsRect].origin toView:nil];
>+  NSPoint rightEdgeOfTabsRectInWindowCoords = [self convertPoint:NSMakePoint(NSMaxX([self tabsRect]), NSMaxY([self tabsRect])) toView:nil];

Call tabsRect once and save the answer. Name the variables to indicate that they are points rather than single-dimension values.

>+  // Since we're manipulating the dragged tab window's origin, retrieve the tab bar boundaries
>+  // in screen coordinates.
>+  NSPoint minAllowedTabLocation = [[self window] convertBaseToScreen:leftEdgeOfTabsRectInWindowCoords];
>+  NSPoint maxAllowedTabLocation = [[self window] convertBaseToScreen:rightEdgeOfTabsRectInWindowCoords];
>+  maxAllowedTabLocation.x -= [mDraggingTabWindow frame].size.width;

You need the intermediate window coords in points for conversions, but at the screen level you don't, so don't use NSPoints for these values. And make it clear that these refer to origin maxes, rather than edge boundaries, so the right-side constraint comparison below makes more sense when reading it.

>+  if (currentTabWindowLocation.x == minAllowedTabLocation.x &&
>+      mouseLocation.x < (minAllowedTabLocation.x + mLocationOnTabButtonWhereDragBegan))
>+  {
>+    return;
>+  }
>+  else if (currentTabWindowLocation.x == maxAllowedTabLocation.x &&
>+           mouseLocation.x > (maxAllowedTabLocation.x + mLocationOnTabButtonWhereDragBegan))
>+  {
>+    return;
>+  }

Collapse these two blocks:
if ((thing1 &&
     thing) ||
    (thing3 &&
     thinkg4))

>+  // The dragged tab was retained when the drag began.
>+  [mDraggedTab release];

Always nil a a member that you release anywhere outside dealloc.

>+- (void)slidingTabAnimationEnded:(NSNotification*)aNotification

If a tab closes while it's still animating, will this be called?

>+  [mDraggingTabWindow orderOut:self];
>+  [mDraggingTabWindow release];

Same note about nil'ing.

>+
>+- (void)saveTabButtonFramesForDragging
>+{
>+  // Normally, to figure out the position of sliding tabs, we could just multiply the width by
>+  // it's offset from the left, but our tabs are not consistently sized.

Since they are deliberately not all the same size, so you can never assume they are in our code, I'm not sure that qualifies as "normally" ;)

>+static const float kSlidingTabAnimationDuration = 0.40f;

Remove the trailing 0.

>+// For an NSView, coordinates should be expressed in the local coordinate system for
>+// that view.  NSWindow targets should supply screen coordinates.

Local to the view, or local to its container? I would expect the latter.

>+static const NSTimeInterval kDefaultAnimationDuration = 0.50;

Again, remove the trailing 0.

Comment 73

[Sean Murphy]

Attachment: Patch, v2

Patch with review comments addressed.

A few things worth mentioning:

- The tab view contains all logic about whether to hide or show the tab bar based on prefs, and also if the close button should be enabled on the first tab.  The tab bar, if a button closes during drag, has to suppress the |rebuildTabBar| method, and then later needs a way to perform this logic.  I moved the |showOrHideTabsAsAppropriate| method public to do this.  To me, it seemed reasonable over any other approach that came to mind.

- When dragging, the tab bar needed a way to know immediately when a tab view item is added or removed.  Since the BrowserTabBarView and BrowserTabView are tightly coupled already, I have the TabView directly inform when this happens, rather than use notifications.  Just calling having its rebuildTabBar method called did not serve as a clean enough approach for having the bar respond to such changes during a drag.

The following enhancements are filed as follow up bugs so we can get base functionality in place now:

> - Dragging to the edge in tab-scroll setup needs to actually scroll. This
> should be a follow-up unless it's easy though.

Bug 465793.

> - Dragging when there are scroll arrows causes the scroll arrows to lose their
> borders, which looks a bit odd at first glance; that too can be a follow-up if
> we decide it should behave the other way.

Bug 465796.

Everything else has been addressed.  Sorry the refresh took so long this time, if any other changes are needed, I'll be able to turn around much quicker.

Comment 74

[Stuart Morgan]

Comment on attachment 349043 [details] [diff] [review]
Patch, v2

The behavior looks very solid now; nicely done. I have one more round of comments, and then I imagine we'll be good to go!

>+  [mDraggingTab release];
>+  [mDraggingTabWindow release];
>+  [mSavedTabFrames release];
>+  [mDraggingTab release];
>+  [mDraggingTabWindow release];

Why are you double-releasing two objects?

>+  @synchronized (mCurrentlySlidingTabs) {
>+    [mCurrentlySlidingTabs release];
>+    mCurrentlySlidingTabs = nil;
>+  }

This isn't really doing what you want. You are niling out mCurrentlySlidingTabs so that it won't be used from a background thread, but for that case to happen your background callback method would have to be running in another thread--and in the process, calling methods on this object--while it is being torn down. So at the point where it would matter, you've already lost. The same applies to @synchronizing it. Instead you need to make sure that you can't get into that case (see later discussion).

>+  mDraggingTab = [[(BrowserTabViewItem *)[mTabView selectedTabViewItem] buttonView] retain];
[...]
>+  mDraggingTabWindow = [self newFloatingWindowForDraggedTab];

Just to be on the safe side, preface each of these with an [mDraggingFoo release] call.

>+  if ((currentTabWindowLocation.x == minAllowedTabXLocation &&
>+      screenPoint.x < (minAllowedTabXLocation + mHorizontalGrabOffset)) ||
>+      (currentTabWindowLocation.x == maxAllowedTabXLocation &&
>+      screenPoint.x > (maxAllowedTabXLocation + mHorizontalGrabOffset)))

Both |screenPoint.x ...| lines are missing a space of indentation. They should be visually inside their enclosing parens.

>+  // Animate the dragged tab into its proper position.
>+  CHSlidingViewAnimation *slidingWindowAnimation = [[CHSlidingViewAnimation alloc] initWithAnimationTarget:mDraggingTabWindow];
>+  [slidingWindowAnimation setDuration:0.2];
>+  [slidingWindowAnimation setFrameRate:0.0];
>+  [slidingWindowAnimation setAnimationBlockingMode:NSAnimationNonblocking];
>+  [slidingWindowAnimation setAnimationCurve:NSAnimationEaseInOut];
>+  [slidingWindowAnimation setDelegate:self];
>+  [slidingWindowAnimation setStartLocation:[mDraggingTabWindow frame].origin];
>+  [slidingWindowAnimation setEndLocation:draggedTabDestinationFrame.origin];
>+  [slidingWindowAnimation startAnimation];
>+  [slidingWindowAnimation release];

Who owns this animation now?

>+// Called when a background tab has finished sliding out of the way of the
>+// dragging tab.  This method is called from secondary threads.
>+- (void)slidingTabAnimationEnded:(NSNotification*)aNotification
>+{
>+  TabButtonView* animatedTab = (TabButtonView*)[aNotification object];
>+  int animatedTabIndex = [mTabView indexOfTabViewItem:[animatedTab tabViewItem]];
>+
>+  // These notifications can happen simultaneously, so we always have to
>+  // lock access modifications to this collection.
>+  @synchronized (mCurrentlySlidingTabs) {
>+    [mCurrentlySlidingTabs removeObjectIdenticalTo:animatedTab];
>+  }
>+
>+  // Update the tab divider images.
>+
>+  // If there is a tab to the right of the animated one and we finished animating (meaning we sit 
>+  // right next to it), take away its left divider.
>+  if (animatedTabIndex < [self rightmostVisibleTabIndex] &&
>+      [[[aNotification userInfo] valueForKey:kSlidingTabAnimationFinishedCompletelyKey] boolValue] == YES) {
>+    TabButtonView* nextTabButton = [self tabButtonAtIndex:(animatedTabIndex + 1)];
>+    [nextTabButton setDrawsLeftDivider:NO];
>+  }
>+
>+  // Draw a left divider if this tab is to the right of the available tab spot.
>+  // (The drag may have ended before this tab's animation did, so also ensure there
>+  // is a tab dragging.)
>+  if ([self tabIsCurrentlyDragging] && animatedTabIndex == (mEmptyTabPlaceholderIndex + 1))
>+    [animatedTab setDrawsLeftDivider:YES];
>+  else
>+    [animatedTab setDrawsLeftDivider:NO];
>+
>+  @synchronized (mCurrentlySlidingTabs) {
>+    if ([mCurrentlySlidingTabs count] == 0 && ![self tabIsCurrentlyDragging])
>+      [self performSelectorOnMainThread:@selector(rebuildTabBar) withObject:nil waitUntilDone:YES];
>+  }
>+}

If this is being called from a background thread, then you are implicitly assuming that every method you call from it is also threadsafe. That's certainly not true for several of the methods you call on |self|, since they involve member variable access, and you shouldn't be assuming it about other objects either (for example, setDrawsLeftDivider: changes a member variable and a display property).

Could you just have this notification posted from the primary thread instead? If not, you should probably have this method do nothing but performSelectorOnMainThread another method that does all this stuff.

>+- (void)animationDidStop:(NSAnimation *)animation
...
>+- (void)animationDidEnd:(NSAnimation *)animation
...
>+- (void)finishAnimation:(BOOL)animationFinishedCompletely;

Are these called on a background thread? If so, add comments to that effect.

>+  // NSAnimation does not seem to clean up threads appropriately. Creating a new animation
>+  // each time prevents our thread count from getting out of control.
>+  [mViewAnimation release];
>+  mViewAnimation = nil;
>+  [self setSlideAnimationDirection:eSlideAnimationDirectionNone];
>+
>+  NSDictionary *notificationUserInfo = 
>+    [NSDictionary dictionaryWithObject:[NSNumber numberWithBool:animationFinishedCompletely] 
>+                                forKey:kSlidingTabAnimationFinishedCompletelyKey];
>+  [[NSNotificationCenter defaultCenter] postNotificationName:kSlidingTabAnimationFinishedNotification
>+                                                      object:self
>+                                                    userInfo:notificationUserInfo];

Move the mViewAnimation release to after this. The animation owns the view, and the notification is synchronous, so the dealloc problem should be solved.

Comment 75

[Sean Murphy]

Attachment: Patch, v3

Thanks for the additional review, and the positive comments, Stuart.  In this patch, everything mentioned in the previous comment was addressed.

(In reply to comment #74)
> >+  // Animate the dragged tab into its proper position.
> >+  CHSlidingViewAnimation *slidingWindowAnimation = [[CHSlidingViewAnimation alloc] initWithAnimationTarget:mDraggingTabWindow];
> >+  [slidingWindowAnimation setDuration:0.2];
> >+  [slidingWindowAnimation setFrameRate:0.0];
> >+  [slidingWindowAnimation setAnimationBlockingMode:NSAnimationNonblocking];
> >+  [slidingWindowAnimation setAnimationCurve:NSAnimationEaseInOut];
> >+  [slidingWindowAnimation setDelegate:self];
> >+  [slidingWindowAnimation setStartLocation:[mDraggingTabWindow frame].origin];
> >+  [slidingWindowAnimation setEndLocation:draggedTabDestinationFrame.origin];
> >+  [slidingWindowAnimation startAnimation];
> >+  [slidingWindowAnimation release];
> 
> Who owns this animation now?

Similar to NSXMLParser, the animation retains itself in startAnimation and will release when the animation is finished.  I kept the behavior, and added a comment to indicate this fact before the release is performed, since it meant we can avoid adding another instance variable to release the animation later.

-

Otherwise, concerning the main issue from the last patch, the notifications are now processed on the main thread.

Comment 76

[Stuart Morgan]

Comment on attachment 349662 [details] [diff] [review]
Patch, v3

>+  // To prevent a rare crash when clicking on the dragged tab window right when it's being removed,
>+  // do not immediately release it.  (The crash is because of Gecko's NSWindow -sendEvent method swizzling
>+  // calling firstResponder on our deallocated floating tab window.)
>+  [mDraggingTabWindow performSelector:@selector(autorelease) withObject:nil afterDelay:2.0];

Reading again, I strongly suspect that this is unnecessary, and the crash was just a symptom of the fact that you were closing a window from a background thread (which is bad). Could you reproduce it reliably enough to get a definitive answer from testing?

r=smorgan, but please do see if we can remove the delayed autorelease.

Comment 77

[Sean Murphy]

(In reply to comment #76)
> (From update of attachment 349662 [details] [diff] [review])
> >+  // To prevent a rare crash when clicking on the dragged tab window right when it's being removed,
> >+  // do not immediately release it.  (The crash is because of Gecko's NSWindow -sendEvent method swizzling
> >+  // calling firstResponder on our deallocated floating tab window.)
> >+  [mDraggingTabWindow performSelector:@selector(autorelease) withObject:nil afterDelay:2.0];
> 
> Reading again, I strongly suspect that this is unnecessary, and the crash was
> just a symptom of the fact that you were closing a window from a background
> thread (which is bad). Could you reproduce it reliably enough to get a
> definitive answer from testing?
> 
> r=smorgan, but please do see if we can remove the delayed autorelease.

You know what, the crash is actually not caused by the background threading.  I have always had the dragged tab window animation (sliding the dragged tab to its final new position when dragging has completed) take place |NSAnimationNonblocking|.  I chose to perform this one in the main thread, because there is only of its kind.  The sliding background tabs are threaded since the amount of them would really jam up the main thread and slowed things down considerably.

So, the dragging tab window is closed from the main thread.  Thankfully, I have a reliable way to reproduce this problem.  Basically, to cause the bug, just drag the active tab a few pixels, then release it and keep clicking rapidly on the tab as it slides back into place and then keep clicking a few seconds after it's back in place.  The crash still occurs, as some of those events were probably not processed before the window was released, and because of the Gecko code that is running during |sendEvent|.

Comment 78

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

One odd thing I notice with this patch is that when I'm dragging tabs in the front window, I'm getting the tooltips for certain tabs in the background window (I'm also getting the tooltip for the close button, which I'm guessing is also from the background window).  Probably something for a follow-up?

Comment 79

[Mike Pinkerton (not reading bugmail)]

+  if (([currentEvent type] == NSKeyDown) && ([currentEvent keyCode] == 53)) {

extra set of parens not necessary. can you make 53 be a constant?

+  // Our tabs do not all have a consistent size, so we need to
+  // save the actual frames of the tabs to reference later.

be more specific here about the right/left arrows. This comment was a surprise to me.

+  BrowserTabViewItem* aTabViewItem;

nit. Gecko normally uses the |a| prefix to mean a parameter, which it isn't here (it's a local var). just make it |tabViewItem|.

+  for (int currentTabIndex = indexOfClosedItem; currentTabIndex <= [self rightmostVisibleTabIndex]; currentTabIndex++)
+  {

why is the brace on the next line? this method mixes styles.

 //     NEEDS IMPLEMENTED : Implement drag tracking for moving tabs around.
 //  Implementation hints : Track drags ;)

Can this be removed now?

Also be sure to run leaks over this (already mentioned to stuart).

Comment 80

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 349662 [details] [diff] [review]
Patch, v3

sr=pink with nits addressed. No need to go through sr again.

Yay!!!!! I can't wait for this!

Comment 81

[Sean Murphy]

Attachment: Patch, for Check-In

All items from sr addressed.  Thanks for the reviews!

I ran the patch through Leaks before I submitted the initial version, and have followed up in Leaks with each revision.  I double checked again today and it did not detect anything :).

Comment 82

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

Landed on cvs trunk. :D

Thanks for all the work on this Sean, and thanks to everyone who contributed on the road.  Third time really is the charm!

I'll spin comment 78 off into a new bug.

Comment 83

[Stuart Morgan]

And there was much rejoicing.

Comment 84

[Christopher Henderson]

Attachment: Tab drawing issues on Tiger

I'm seeing 'ghosts' of background tabs when dragging with 2.0b1 on 10.4.11 (Intel iMac). The background tabs don't slide either; they just snap into their new positions and leave behind remnants. The current tab slides smoothly, however.

Comment 85

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

(In reply to comment #84)
> Created an attachment (id=354479) [details]
> Tab drawing issues on Tiger
> 
> I'm seeing 'ghosts' of background tabs when dragging with 2.0b1 on 10.4.11
> (Intel iMac). The background tabs don't slide either; they just snap into their
> new positions and leave behind remnants. The current tab slides smoothly,
> however.

Let's follow that as a separate bug blocking this one.