Re: [Gnustep-cvs] r28392 - in /libs/gui/trunk: ChangeLog Source/NSAlert.m Source/NSApplication.m Source/NSDrawer.m

[Fred Kiefer]

Gregory Casamento schrieb:
> Author: gcasa
> Date: Mon Jul 13 20:12:52 2009
> New Revision: 28392
> 
> URL: http://svn.gna.org/viewcvs/gnustep?rev=28392&view=rev
> Log:
>       * Source/NSAlert.m: Implementation of GSAlertSheet.
>       * Source/NSApplication.m: Change order in which setWindowParent:
>       and runModalForWindow: are called in beginSheet:... method.
>       * Source/NSDrawer.m: Remove notifications when drawer is closed
>       in dealloc.
> 
> Modified:
>     libs/gui/trunk/ChangeLog
>     libs/gui/trunk/Source/NSAlert.m
>     libs/gui/trunk/Source/NSApplication.m
>     libs/gui/trunk/Source/NSDrawer.m

I have quite a few issues with this change and I would like these
removed before we ship a new release of gui. So let's get discussing them.

- GSAlertSheet has an instance variable _parentWindow this duplicates
the ivar _parent found in NSWindow. Why do we need this twice?
The only difference I see is that it gets retained here. Is this needed
and if so, should we do it in NSWindow? It might well be that soem of
the code from here rather belongs into NSWindow.

- frameFromParentWindowFrame uses the _parentWindow ivar without
checking whether it is set. This will horribly fail on Sparc machines.
Currently this is save as the method will only be called when this
variable is set, but in the long run this implementation is asking for
trouble.

- GSAlertSheet has an ivar _container that never gets used. What is it for?

- The init... method on GSAlertSheet creates an NSBox object overlapping
its normal content views. This may lead to drawing problems. The NSBox
also never gets released and the size computation should use the proper
methods from GSTheme. The rect variable here never gets used.

- frameFromParentWindowFrame, why does the frame of the content view of
the parent window come into play here?

- NSReleaseAlertPanel() should reset the parent window to nil.

- With all that said, I don't think that we need a separate GSAlertSheet
class, GSAlertPanel should do. Which woudl remove a lot of code duplication.

- All the NSBeginAlertSheet() functions duplicate the sending of the
didEndSelector to the delegate found in NSApplication beginSheet:..., we
should only send this once.


Apart from all that, it is great to see some progress on the sheet code,
although the main part, event handling in NSApplication is still missing.

Cheers,
Fred