PSMTabBar sends close notifications improperly

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Run demo app
2. Notice output in Console includes: "didCloseTabViewItem: Tab"

What is the expected output? What do you see instead?
There should be no output in the Console.  Remove is not the same thing as 
close.  See 
discussion below.

What version of the product are you using? On what operating system?
I pulled the code on 6/2/2008.  I am running on Leopard.

Please provide any additional information below.
PSMTabBarControl should NOT inform the delegate that a tab has been closed when 
that tab has 
been simply removed.  There is a difference.  A tab might be removed for a 
variety of reasons, 
like during DND for example.  Closing a tab means that the tab is being 
destroyed and indicates 
that resources should be deallocated.  John Pannel's code written in 2006 
notified the delegate 
of close notifications correctly.  It seems someone afterwards wanted to be 
notified when a tab 
was being removed and changed the semantics of the close notifications.  Well, 
we should simply 
add another notification for removal if we feel the need to provide that 
notification.  We should 
not change the meaning of "close".

Original issue reported on code.google.com by keithmal...@gmail.com on 3 Jun 2008 at 12:27

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Here are the diffs needed to fix this:

===================================================================
--- WindowController.m  (revision 260)
+++ WindowController.m  (working copy)
@@ -93,7 +93,23 @@

 - (IBAction)closeTab:(id)sender
 {
-    [tabView removeTabViewItem:[tabView selectedTabViewItem]];
+    NSTabViewItem *tabViewItem = [[[tabView selectedTabViewItem] retain] 
autorelease];
+
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)])){
+        if(![[tabBar delegate] tabView:tabView 
shouldCloseTabViewItem:tabViewItem]){
+            return;
+        }
+    }
+    
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:willCloseTabViewItem:)])){
+        [[tabBar delegate] tabView:tabView willCloseTabViewItem:tabViewItem];
+    }
+    
+    [tabView removeTabViewItem:tabViewItem];
+    
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)])){
+        [[tabBar delegate] tabView:tabView didCloseTabViewItem:tabViewItem];
+    }
 }

 - (void)stopProcessing:(id)sender
Index: PSMTabBarControl.h
===================================================================
--- PSMTabBarControl.h  (revision 260)
+++ PSMTabBarControl.h  (working copy)
@@ -193,6 +193,7 @@

 //Standard NSTabView methods
 - (BOOL)tabView:(NSTabView *)aTabView shouldCloseTabViewItem:(NSTabViewItem *)tabViewItem;
+- (void)tabView:(NSTabView *)aTabView willCloseTabViewItem:(NSTabViewItem 
*)tabViewItem;
 - (void)tabView:(NSTabView *)aTabView didCloseTabViewItem:(NSTabViewItem *)tabViewItem;

 //"Spring-loaded" tabs methods
Index: PSMTabBarControl.m
===================================================================
--- PSMTabBarControl.m  (revision 260)
+++ PSMTabBarControl.m  (working copy)
@@ -1532,23 +1532,29 @@

 - (void)closeTabClick:(id)sender
 {
-   NSTabViewItem *item = [sender representedObject];
     [sender retain];
     if(([_cells count] == 1) && (![self canCloseOnlyTab]))
         return;

-    if ([[self delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)]) {
-        if (![[self delegate] tabView:tabView shouldCloseTabViewItem:item]) {
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)])){
+        if(![[self delegate] tabView:tabView shouldCloseTabViewItem:[sender 
representedObject]]){
             // fix mouse downed close button
             [sender setCloseButtonPressed:NO];
             return;
         }
     }
-   
-    [item retain];

-   [tabView removeTabViewItem:item];
-    [item release];
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:willCloseTabViewItem:)])){
+        [[self delegate] tabView:tabView willCloseTabViewItem:[sender 
representedObject]];
+    }
+    
+    [[sender representedObject] retain];
+    [tabView removeTabViewItem:[sender representedObject]];
+    
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)])){
+        [[self delegate] tabView:tabView didCloseTabViewItem:[sender 
representedObject]];
+    }
+    [[sender representedObject] release];
     [sender release];
 }

@@ -1769,10 +1775,6 @@
     while ( (cell = [e nextObject]) ) {
        //remove the observer binding
         if ([cell representedObject] && ![tabItems containsObject:[cell representedObject]]) {
-           if ([[self delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)]) {
-               [[self delegate] tabView:aTabView didCloseTabViewItem:[cell 
representedObject]];
-           }
-           
             [self removeTabForCell:cell];
         }
     }

Original comment by keithmal...@gmail.com on 3 Jun 2008 at 1:08

[GoogleCodeExporter]

In a separate thread, Ryan raised the concern that Adium may be relying upon 
the existing behavior.  I would suggest that we still make the change I 
suggested.  Adium can easily change to work with my change using another 
delegate method.  Here is some code to demonstrate:

- (void)tabViewDidChangeNumberOfTabViewItems:(NSTabView *)TabView
{
  NSArray* current = [TabView tabViewItems];
  NSTabViewItem* tabViewItem;
  NSEnumerator* enumerator = [[mTabViewItems arrayByRemovingObjectsInArray:current] objectEnumerator];
  while ((tabViewItem = [enumerator nextObject]) != nil)
    [self tabView:TabView didRemoveTabViewItem:tabViewItem];
  enumerator = [[current arrayByRemovingObjectsInArray:mTabViewItems] objectEnumerator];
  while ((tabViewItem = [enumerator nextObject]) != nil)
    [self tabView:TabView didAddTabViewItem:tabViewItem];

  NSArray* array = [current copy];
  [mTabViewItems release];
  mTabViewItems = array;
}

Alternatively, if this is too much work to require of Adium, we can change 
PSMTabBarControl so that it still provides the same functionality it currently 
does, but 
we should give the delegate method a different name:

Index: PSMTabBarControl.h
===================================================================
--- PSMTabBarControl.h  (revision 260)
+++ PSMTabBarControl.h  (working copy)
@@ -193,7 +193,7 @@

 //Standard NSTabView methods
 - (BOOL)tabView:(NSTabView *)aTabView shouldCloseTabViewItem:(NSTabViewItem *)tabViewItem;
-- (void)tabView:(NSTabView *)aTabView didCloseTabViewItem:(NSTabViewItem 
*)tabViewItem;
+- (void)tabView:(NSTabView *)aTabView didDetachTabViewItem:(NSTabViewItem 
*)tabViewItem;

 //"Spring-loaded" tabs methods
 - (NSArray *)allowedDraggedTypesForTabView:(NSTabView *)aTabView;
Index: PSMTabBarControl.m
===================================================================
--- PSMTabBarControl.m  (revision 260)
+++ PSMTabBarControl.m  (working copy)
@@ -1769,8 +1769,8 @@
     while ( (cell = [e nextObject]) ) {
        //remove the observer binding
         if ([cell representedObject] && ![tabItems containsObject:[cell representedObject]]) {
-           if ([[self delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)]) {
-               [[self delegate] tabView:aTabView didCloseTabViewItem:[cell 
representedObject]];
+           if ([[self delegate] 
respondsToSelector:@selector(tabView:didDetachTabViewItem:)]) {
+               [[self delegate] tabView:aTabView didDetachTabViewItem:[cell 
representedObject]];
            }

             [self removeTabForCell:cell];

Original comment by keithmal...@gmail.com on 4 Jun 2008 at 4:27

[GoogleCodeExporter]

Do we have the ability to run with the old and new way of doing this, but 
deprecating
the old way so that apps don't fail if they update psmtabbarcontrol?

Original comment by cforsy...@gmail.com on 4 Jun 2008 at 4:51

[GoogleCodeExporter]

Hey Chris, if we update the code to also include the changes I suggested in 
comment #3, then the only 
change clients might have to make would be to rename one word in their source 
(replace 
"didCloseTabViewItem" with "didDetachTabViewItem").  Is that too much to ask?

If it is, I suppose we could rename the callbacks I need.  Specifically, I 
could rename -
tabView:willCloseTabViewItem: to something like 
-tabView:willReallyCloseTabViewItem: and -
tabView:didCloseTabViewItem: to -tabView:didReallyCloseTabViewItem:.  However, 
I think this is confusing.  
Furthermore, it deviates from the 2006 baseline unnecessarily.

Can you explain why you and Ryan are hesitant to introduce a change that will 
require a change by Adium?  It 
seems to me that the maccode should strive to remain true to the original code 
base and should strive to be 
the best it can be.  It should not strive to serve any particular client I 
should think...

Original comment by keithmal...@gmail.com on 4 Jun 2008 at 5:30

[GoogleCodeExporter]

I'm concerned about all projects. There are more projects than just Adium using
psmtabbarcontrol. Notice how I said apps and not adium?

Multiple more apps are using psmtabbarcontrol now than were using them in 2006, 
so my
concern is that we'll have to warn a bunch of devs to watch out whenever they 
update
their code.

I'm hesitant whenever anyone mentioned any kind of change that makes a coder 
not just
update a framework, but makes them go find their code and then fit in the new 
stuff
into their existing codebase. Yes, it is one or two changes, but it's still 
work, and
still of concern.

I don't have the ability to review the patches, however I would rather try to
understand why this change was made, rather than assume that it was done with 
poor
judgement. Do you know what revision changed this? svn log should be of use 
here.

Original comment by cforsy...@gmail.com on 4 Jun 2008 at 6:06

[GoogleCodeExporter]

Hey Chris, I share your method of thinking, but we are looking at two different 
points in time.  Your two 
points in time are the current revision and the 2008 release, while my two 
points in time are the 2006 release 
and the 2008 release.  My code base is using the 2006 release and I want to 
update to the maccode version.  
When I do this, my code breaks because the -tabBar:willCloseTabViewItem: 
callback was removed and the 
semantics of the -tabBar:didCloseTabViewItem: callback was changed.

I did research this as far as I could.  Unfortunately, the change was made 
before maccode's initial import of 
the project, so I can't determine who made the change.  But from looking at the 
change, I can tell you that it 
was poorly implemented.  If the author of the change wanted to be notified 
whenever a tab was removed, he 
could have implemented what I described in comment #3.  This would have 
accomplished his goal without 
breaking clients who were relying on the 2006 behavior (which is also the 
logical behavior in my eyes).

Please let me know how you would like to proceed.  If I have to modify my 
changes in order to get them 
approved, then I will do that...

Original comment by keithmal...@gmail.com on 4 Jun 2008 at 6:41

[GoogleCodeExporter]

Bear in mind that I'm a project manager and not a developer by any means, so 
here's
what I would like. I want what is best for the framework. To be blunt, if the
codebase we are using is much newer, and doesn't have the stuff even on import 
that
you were using, then that needs to be ignored.

However, if we ignore that, there is still the "poor implementation" that needs 
to be
looked at, and the issue of semantics.

So here's what I would like to see:

1 - A summary of the best option to go with for the framework itself, that would
affect the most applications in the least critical way. My assumption is that 
more
applications use the newer code than the 2006 code, but maybe I'm wrong. We 
don't
have a way to know this though.

2 - A list of the changes that an application with the 2006 codebase would need 
to
make to make this new change work, and a list of changes that the current 
codebase
users would need to make this work, in howto form. rtf+screenshots is always 
fun for
extra credit of course.

Someone will need to review both the code changes and the documentation to go 
along
with it. You can make a new ticket on the docs, but I think it's beneficial to 
have
them in general. I don't remember if we have implementation docs already, but 
if we
do a diff against those would probably do just fine.

Does this make sense as to what my stance is/has been?

Original comment by cforsy...@gmail.com on 4 Jun 2008 at 6:58

[GoogleCodeExporter]

Hey Chris, I truly believe the changes I posted already are what's best for the 
framework.  Let me describe the 
problem again now that I know you are not a developer.

The 2006 framework notified clients when a tab was being CLOSED.  The existing 
documentation describes 
this behavior.  Furthermore, this behavior is necessary because it gives 
clients an opportunity to clean up 
things it has associated with the tab.

The 2008 framework changed the meaning of the close notification.  The 
notification still has the same name, 
but it is now sent whenever a tab is being REMOVED.  (As I mentioned before, 
removal and closure are not the 
same thing.)  Changing the semantics of close was bad for two reasons.  First, 
the name is now misleading.  
Second, and more importantly, clients are now no longer informed when a tab is 
being closed, and they need 
to know this because they may have things associated with the tab that need to 
be cleaned up.

Fortunately, there is an easy solution.  We can support notifications of both 
kinds of events.  One way or 
another, I am pretty sure we will be adding back the close notifications.  The 
only question at this point is 
how to name all of the notifications.  Do we want to continue calling remove 
"close", or do we want to use a 
more accurate term like "remove" or "detach"?  If we continue to call remove 
"close" then how should we name 
the close notification?  (The name I came up with was "really close".)

I think we should name the notifications accurately.  Doing so will be 
consistent with 2006 release; it will 
match the existing documentation; and it will make sense to users of the 
framework.  The only consequence 
will be that existing clients of the maccode version will have to change one 
word in their source code if they 
were using the misnamed notification.

If runtime compatibility is a requirement, let me know and I will resubmit my 
changes such that they will be 
guaranteed to work with clients of the current codebase.  The changes will be 
almost identical to those I 
submitted in comment #2, but a couple names will be different.  The consequence 
of Plan B here is that the 
naming of some callbacks will be confusing, and the documentation will not 
match the source code unless 
someone else works on that.  The latest codebase would also not be runtime 
compatible with the 2006 
release.

Original comment by keithmal...@gmail.com on 4 Jun 2008 at 8:41

[GoogleCodeExporter]

Ah, that makes sense. I think going with detach is a good idea, since it 
describes
the dragging off bit.

I'd rather go with long term gains here. It's up to whoever reviews the patch 
though
to make the final decision on it, but my vote is to go with the one that names 
the
methods for what they are doing.

Good work Keith.

Original comment by cforsy...@gmail.com on 4 Jun 2008 at 8:53

[GoogleCodeExporter]

Thanks Chris.  Please let me know if I can be of further assistance!

Original comment by keithmal...@gmail.com on 4 Jun 2008 at 9:33

[GoogleCodeExporter]

For convenience, I have merged the changes from comments #2 and #3 into one:

Index: WindowController.m
===================================================================
--- WindowController.m  (revision 261)
+++ WindowController.m  (working copy)
@@ -93,7 +93,23 @@

 - (IBAction)closeTab:(id)sender
 {
-    [tabView removeTabViewItem:[tabView selectedTabViewItem]];
+    NSTabViewItem *tabViewItem = [tabView selectedTabViewItem];
+
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)])){
+        if(![[tabBar delegate] tabView:tabView 
shouldCloseTabViewItem:tabViewItem]){
+            return;
+        }
+    }
+    
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:willCloseTabViewItem:)])){
+        [[tabBar delegate] tabView:tabView willCloseTabViewItem:tabViewItem];
+    }
+    
+    [tabView removeTabViewItem:[[tabViewItem retain] autorelease]];
+    
+    if(([tabBar delegate]) && ([[tabBar delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)])){
+        [[tabBar delegate] tabView:tabView didCloseTabViewItem:tabViewItem];
+    }
 }

 - (void)stopProcessing:(id)sender
Index: PSMTabBarControl.h
===================================================================
--- PSMTabBarControl.h  (revision 261)
+++ PSMTabBarControl.h  (working copy)
@@ -193,7 +193,9 @@

 //Standard NSTabView methods
 - (BOOL)tabView:(NSTabView *)aTabView shouldCloseTabViewItem:(NSTabViewItem *)tabViewItem;
+- (void)tabView:(NSTabView *)aTabView willCloseTabViewItem:(NSTabViewItem 
*)tabViewItem;
 - (void)tabView:(NSTabView *)aTabView didCloseTabViewItem:(NSTabViewItem *)tabViewItem;
+- (void)tabView:(NSTabView *)aTabView didDetachTabViewItem:(NSTabViewItem 
*)tabViewItem;

 //"Spring-loaded" tabs methods
 - (NSArray *)allowedDraggedTypesForTabView:(NSTabView *)aTabView;
Index: PSMTabBarControl.m
===================================================================
--- PSMTabBarControl.m  (revision 261)
+++ PSMTabBarControl.m  (working copy)
@@ -1533,23 +1533,29 @@

 - (void)closeTabClick:(id)sender
 {
-   NSTabViewItem *item = [sender representedObject];
     [sender retain];
     if(([_cells count] == 1) && (![self canCloseOnlyTab]))
         return;

-    if ([[self delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)]) {
-        if (![[self delegate] tabView:tabView shouldCloseTabViewItem:item]) {
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:shouldCloseTabViewItem:)])){
+        if(![[self delegate] tabView:tabView shouldCloseTabViewItem:[sender 
representedObject]]){
             // fix mouse downed close button
             [sender setCloseButtonPressed:NO];
             return;
         }
     }
-   
-    [item retain];

-   [tabView removeTabViewItem:item];
-    [item release];
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:willCloseTabViewItem:)])){
+        [[self delegate] tabView:tabView willCloseTabViewItem:[sender 
representedObject]];
+    }
+    
+    [[sender representedObject] retain];
+    [tabView removeTabViewItem:[sender representedObject]];
+    
+    if(([self delegate]) && ([[self delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)])){
+        [[self delegate] tabView:tabView didCloseTabViewItem:[sender 
representedObject]];
+    }
+    [[sender representedObject] release];
     [sender release];
 }

@@ -1770,8 +1776,8 @@
     while ( (cell = [e nextObject]) ) {
        //remove the observer binding
         if ([cell representedObject] && ![tabItems containsObject:[cell representedObject]]) {
-           if ([[self delegate] 
respondsToSelector:@selector(tabView:didCloseTabViewItem:)]) {
-               [[self delegate] tabView:aTabView didCloseTabViewItem:[cell 
representedObject]];
+           if ([[self delegate] 
respondsToSelector:@selector(tabView:didDetachTabViewItem:)]) {
+               [[self delegate] tabView:aTabView didDetachTabViewItem:[cell 
representedObject]];
            }

             [self removeTabForCell:cell];

I hope someone will merge this before the first release.

Original comment by keithmal...@gmail.com on 5 Jun 2008 at 6:26