Re: Release critical bug in NSToolbarItem

[Quentin Mathé]

Hi Fred,

Le 9 mars 08 à 16:28, Fred Kiefer a écrit :

> Putting it into the bug tracker wont help as long as nobody takes up  
> the
> task to solve the issue :-)
> Which is what I did myself already. I provided a workaround in the
> toolbar code right after reporting the issue. What I wanted to prevent
> was Adam doing a release with this problem not addressed.
> In general you are of course correct, we should put all the issues we
> find in the bug tracker, even if we are the ones o work on them  
> later. I
> already have taken up this habit for all more complex issues.
>
> My solution is currently only a hack. This is due to my missing
> understanding of the NSToolbar related code. To me it is even unclear,
> why there is GSToolbar and NSToolbar.

When I wrote NSToolbar implementation, I introduced GSToolbar  
superclass in order to allow inserting a toolbar anywhere in a window  
and not just underneath the title bar as NSToolbar only supports it.
If you run ToolbarExample from gui examples, you can see a demo of  
this feature. May be I stretched the API a bit too far and we should  
either remove this feature or merge NSToolbar and GSToolbar into a  
single class. GSToolbar doesn't allow to change display and size modes  
once it has been initialized. That's the main difference with  
NSToolbar that mainly adds methods --setDisplayMode:, -setSizeMode:  
which alters the toolbar view frame. This resizing can easily be  
handled automatically in NSToolbar case because the toolbar is always  
located at the top of the window.

> As for GSToolbarButton, I have the
> impression that this class uses an ivar to store the action, instead  
> of
> passing it to the cell as would be the normal case, because of the
> recursion to bumped into. With that properly resolved it looks like we
> could clean up more of the NSToolbar cluster. But is my hack a proper
> solution? I am not sure. While investigating the code I found a second
> recursion, also stopped by my patch.

I would need to take a deeper look at it, but iirc I introduced these  
not-so-clean workarounds because of issues I encountered with actions  
and -mouseDown: when using NSButton and NSButtonCell three years ago.  
Toolbar buttons have to be highlighted on mouse down and send the  
action on mouse up. If they are selectable (like tabs), the same  
behavior must occur on -[NSToobar setSelectedItemIdentifier:] but the  
highlighting must be retained until another selectable item in the  
toolbar is clicked or or selected with the previous method in code.  
The code can probably be rewritten in a more natural way today.

> To me this looks like the tasks are not clearly separated between the
> different components. But then I am not sure, there may be some code  
> out
> there, that relies on any of the strange looking features here.

-_setSelected: shouldn't invoke -performClick: probably but rather  
just changes the state of the toolbar button to be highlighted,  
redraws it and sends the action. Calling -performClick: was convenient  
because -_setSelected: must do precisely what happens when the user  
clicks a toolbar item that is selectable. I think that was my  
reasoning at the time I wrote this code.
Another point is that it could be more correct or cleaner to handle  
most of -_setSelected: logic in -[GS/NSToolbar   
setSelectedItemIdentifier:].

> Perhaps
> Quentin is able to shed some light here. I was hoping for him to reply
> to my mail. Perhaps I need to integrate his changes to NSOutlineView
> first :-)

:-)

Quentin.