gnustep-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Release critical bug in NSToolbarItem


From: Fred Kiefer
Subject: Re: Release critical bug in NSToolbarItem
Date: Mon, 10 Mar 2008 09:31:13 +0100
User-agent: Thunderbird 2.0.0.9 (X11/20070801)

Quentin Mathé wrote:
> 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:].
> 

Thank you for this long explanation. I will keep it in case I do more
changes on NSToolbar.
Could you please test, whether a tool bar after my change still behaves
as expected in all relevant cases?
I tried to run the example code your pointed me to (Thank you for that
as well!), but this horribly fails with issues that seem unrelated to my
change.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]