The Case of the Missing Check

July 20th, 2006

Things are getting ever-nearer with FlexTime, but as is the case with every project of relative complexity, the number of niggling details expands in proportion to the number of days of development ostensibly remaining.

Today I spent a lot longer than I wish I had working on a confounding bug report with my NSToolbar implementation: the contextual menu doesn’t update to reflect the changed state. If you control or right-click in any standard Cocoa application’s toolbar, you’ll see a contextual menu containing common configuration options. The indicate the enabled state of these options, a check mark is supposed to appear next to them. This is what the menu looks like in FlexTime 1.0b5:

No check marks! Obviously the icons are small, and I’m seeing “Icon and Text.” Something is definitely wrong here. But I’m not responsible for managing this menu. Who is?

If you guessed NSToolbar, don’t feel stupid. I guessed that too, but we’re wrong. My first reaction in debugging this problem was to search the documentation for NSToolbar to see if there are any funny delegate methods I’m supposed to implement. NSToolbar has nary a mention at all of even the existence of this contextual menu! Before getting too deep, I tried a few web searches, to see if anybody else had already run into the problem. I found this from 2001 describing an identical problem. And this from last year following up on it. But where is the fix?

The next phase in my debugging process was to start hacking out code until things work. I figured there must be something funny in my delegate methods, so I just unhooked the delegate and let ‘er rip. Result? An empty toolbar with exactly the same problem.

The Xcode developer tools ship with a source code example, Simple Toolbar (local link). I was running out of patience so I decided to attack the problem from the complete opposite direction. Instead of asking “why doesn’t my menu work right?” I would ask “why do all the other ones work right?” Who is responsible for setting those damn check marks, anyway?

From the SimpleToolbar project, I first launched and confirmed that the check marks show up as expected. Then, I “Paused” execution to get access to the gdb console, and set a breakpoint:

(gdb) b -[NSMenuItem setState:]
Breakpoint 6 at 0x9373b5f8
(gdb)

Somebody is setting those checkmarks, and unless they’re doing it in an extremely unsanctioned way, I’ll catch them at NSMenuItem’s setState: method. The only problem is everytime I “continue,” it reactivates the application and causes my breakpoint to be hit at a spot where I don’t care about it. Once the breakpoint hits I’m stuck back in the debugger and never get a chance to right-click in the toolbar! This is overcome with a simple conditional on the breakpoint. It turns out that it’s just one menu item that is always getting set when I activate the app, so I configure gdb to skip it in that case:

Breakpoint 6 at 0x9373b5f8
(gdb) po $r3
<MenuItem: 0x36b660 Untitled>
(gdb) cond 1 $r3!=0x36b660
(gdb) c

Now I’m able to right-click and provoke the anticipated action. I’m interrupted again by the debugger, this time pointing at one of my items of interest:

Breakpoint 6 at 0x9373b5f8
(gdb) po $r3
<MenuItem: 0x3ab160 Icon & Text>
(gdb) bt
#0 0x9373b5f8 in -[NSMenuItem setState:] ()
#1 0x9382cf90 in -[NSWindow validateUserInterfaceItem:] ()
#2 0x9382ca04 in -[NSWindow validateMenuItem:] ()
#3 0x937e931c in -[NSMenu _enableItems] ()
#4 0x937e72d4 in AppKitMenuEventHandler ()

Note: you don’t have to use commands like “bt” at this point, since Xcode’s debugger is mimicking the same thing in the UI, but once I get started in the console I tend to stay there for a while.

So what have we learned by now? NSToolbar is not responsible for setting those menu item states, NSWindow is! It makes a sort of sick sense, if you consider the overlapping responsibilities of the toolbar and the window. After all, the window is what you “set” a toolbar on. The window has to know how to collapse and expand it. It’s a sick little behind-the-scenes game that fortunately we don’t have to see much of.

So why isn’t validateUserInterfaceItem: called in my app? I set a breakpoint on the method of NSWindow and it never triggers. Hmm, suspicious indeed. But when I, on a whim, search for the method in my sources, my memory is thankfully jostled. Of ourse! FlexTime uses a subclass of NSWindow, and within it, overrides validateUserInterfaceItem! It’s been a while since I did this, so it’s not fresh in my mind. This is what the method looks like:

// MENU Validation
- (BOOL)validateUserInterfaceItem:(id <NSValidatedUserInterfaceItem>)anItem
{
	BOOL validItem = YES;

	// We use validation to keep the state of NSMenu targets up to date
	SEL thisSelection = [anItem action];
	if ((thisSelection == @selector(undo:)) ||
		(thisSelection == @selector(redo:)))
	{
		validItem = (mUndoDisabled == NO);
	}
	return validItem;
}

At some point I figured out that if I override NSWindow’s menu validation, I get a chance to disable undo and redo. I wanted to do this because it turns out to be very inconvenient timing for a user to suddenly try to “undo” during the middle of a FlexTime routine playback. So what’s wrong with the above method? It cuts NSWindow out of the loop.

Simply adding a “[super validateUserInterfaceItem:anItem]” call at the top of the method does the trick functionally, but it leaves me with a bad taste in my mouth. Specifically, a bad compiler warning. The method is defined as the sole member of the formal NSUserInterfaceValidations protocol, but NSWindow isn’t declared as conforming to that protocol. So I’m stuck in a tough situation. I know my superclass implements this method – I saw it in gdb! I also know that if it doesn’t get its implementation called, my functionality suffers. I decide to code defensively and plan for the best while expecting the worst. If the superclass responds to the method in question, we’ll cast it as being protocol compliant, to satisfy the compiler:

// Let the standard NSWindow validation take place
if ([super respondsToSelector:@selector(validateUserInterfaceItem:)] == YES)
{
	validItem = [(id )super validateUserInterfaceItem:anItem];
}

Now, even in the unlikely event that Apple completely changes the way they handle this, we’ll probably still be safe. Added bonus? No compiler warnings.

But more importantly, the change works! My check marks now appear as expected. One more bug, laboriously down the drain.

How did this bug happen? Several factors played into the mistake on my part. Ultimately it came to an underestimation on my part of the method’s significance to NSWindow. Since NSWindow is not documented as using the method, it was a “lucky break” for me that implementing it in my subclass got me the short-term results I was seeking. But alarm bells should have been going off. I’m overriding a method in a standard system class and seeing interesting results. That means the system must have good reason to call it. I will be much more careful in the future when subclassing and overriding system classes.

The other problems here were with naming convention and habitual expectations. It’s important to remember that, although validateUserInterfaceItem’s main purpose is to enable or disable a part of the UI, its secondary purpose is to allow clients arbitrary access to the UI item. If it was called “validateAndMuckWithUserInterfaceItem,” I might have paused a second or two and thought more carefully about the implications of overriding it. Worse, the typical scenarios where this method is implemented are those where the system has set aside some number of UI elements as being “your responsibility.” I’m not in the habit of needing to defer to the system about whether my UI elements are enabled or not, but in this case by becoming the system, I acquired a whole new set of responsibilities.

Update: Chris Liscio pointed out that the behavior of validateUserInterfaceItem: is to bounce up the responder chain until somebody says “YES” to an item. So another failure here is based no my misunderstanding of the method. It’s well documented, but I never paid close attention:

To validate a control, the application calls validateUserInterfaceItem: for each item in the responder chain, starting with the first responder. If no responder returns YES, the item is disabled.

This means my typical approach to implementing the method needs to change. I have assumed that everything would be enabled unless I said otherwise, when in fact the opposite is true. I should insist that everything I don’t recognize gets a “NO” result from the validation method, as a general rule.

12 Responses to “The Case of the Missing Check”

  1. charles Says:

    So, question: in the actual code, you would actually start with

    BOOL validItem = NO;

    is that what you mean in the update to your post?

    Btw, did you see the review of FlexTime in 43Folders?

  2. Daniel Jalkut Says:

    charles: Yep – I think that would be the correct general approach. Of course, realizing that accidentally marking something as valid is probably less damaging than accidentally neglecting to mark it as valid.

    So I think a combination of “default to NO” and “carefully cover every UI item for which we’re responsible” is in order.

  3. Andy Lee Says:

    Should that be “expands in inverse proportion”?

  4. Daniel Jalkut Says:

    Andy: Yep – I think you’re right. Somehow I knew I’d get the poetry wrong there :)

  5. Conor Dearden Says:

    In most cases one should call super in methods you override.

  6. Adam Maxwell Says:

    Shouldn’t you use [[self superclass] instancesRespondToSelector:@selector(validateUserInterfaceItem:)] instead of [super respondsToSelector:]? Using the super keyword here seems wrong.

  7. Daniel Jalkut Says:

    Good catch Adam, you’re right.

  8. ken Says:

    [[self superclass] instancesRespondToSelector:@selector(validateUserInterfaceItem:)] is rarely correct either.  It means that if you ever create a subclass of your (currently) leaf class, the check stops being meaningful.

    I think you want to hardwire the superclass name.

  9. Daniel Jalkut Says:

    Ken: Aha! Jeez, I’m glad I blogged this since I’m sure this is a series of bad habits I would have otherwise held onto for a long time.

  10. Marcel Weiher Says:

    The [super respondsToSelector…] fragement probably doesn’t have the effect that you’re thinking. It will certainly NOT check wether your superclass implements this method, but actually check wether your class implements that method.

    super sends the message to self, but starting the message lookup at the superclass of the class where the super send occurs. So it will start looking for respondsToSelector: in NSWindow, in all likelyhood hitting the exact same respondsToSelector: implementation in NSObject that you would have gotten had you sent the message to self. That implementation will then check if the receiver implements the method, which it does (the receiver being your instance of your NSWindow subclass).

    What you probably want to do is [[self class] superclass] instancesRespondToSelector: ]

  11. matt neuburg Says:

    Funny this should arise just now, since I just got thru posting a note to Cocoa-Dev about this same issue in awakeFromNib. Sometimes super is a built-in Cocoa class and does significant work in awakeFromNib. So that’s two bugs in the Cocoa docs, because (1) the Cocoa docs explicitly tell you not to call [super awakeFromNib] if super is a built-in Cocoa class, and (2) the Cocoa docs don’t document when a built-in Cocoa class implements awakeFromNib in a significant way. A useful and compact formulation is:

    if([[self superclass] instancesRespondToSelector:_cmd])
      [super awakeFromNib];

  12. Jon Hess Says:

    Hey Matt –

    As others have noted, you probably don’t mean to invoke ([[self superclass] instancesRespondToSelector:…]).

    If you are a Foo, and Foo is an Object, this works. If someone subclasses Foo as Bar, then ‘[self superclass[‘ is Foo, and Foo always responds to that selector. As others have noted, you probably want to hardwire the classname.

Comments are Closed.

Follow the Conversation

Stay up-to-date by subscribing to the Comments RSS Feed for this entry.