[Top][All Lists]
[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.