Re: [PATCH] Minor fix in NSWindowController
Hi Quentin, Quentin Mathé wrote: > Le 28 mars 06 à 00:57, Fred Kiefer a écrit : >> >> I am rather unsure about this patch.I don't quite remember, who did >> put in these commetn lines on retaining the window, but I would >> expect that there were reasons for it at that time. Still they may >> have been the now redundant solution of an already resolved >> problem. > > I think you wrote the comments, here is the ChangeLog entry which > introduced this 'retain' line : 2003-08-30 Fred Kiefer > <[EMAIL PROTECTED]> > > * Source/NSWindowController.m Changed [setWindow:] to manage the > notification connection to the window. [initWithWindow:], [dealloc] > and [_windowWillClose:] now use [setWindow:]. [setDocument:] now no > longer retains the document, as this is already retaining the window > controller. Simplified [loadWindow] by using the method > [windowNibPath]. > Oops, I must admit that I don't have any memory of this change any more. If your new code works for document based applications as well as for others, then it should be fine to submit it. Could you please test with Ink as well? > > The previous comment makes a bit more sense (unlike the most recent > one) : > > /* * If the window is set to isReleasedWhenClosed, it will release * > itself, so nil out our reference so we don't release it again * * > Apple's implementation doesn't seem to deal with this case, and * > crashes if isReleaseWhenClosed is set. */ */ _window = nil; > > However I think it's not needed because there is no 'retain' or > 'release' calls on _window in NSWindowController.m (not even in - > dealloc method). > This is not completly true, the retaining and releasing of _window gets done in setWindow:. This seems to have been the core of that patch, to move all code which changes the _window ivar into one place. This change was related to the bug report #4840, which was about a similar problem as the one you are facing now. If I only could remember... Fred ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: [PATCH] Minor fix in NSWindowController
Le 28 mars 06 à 00:57, Fred Kiefer a écrit : Hi Quentin, I am rather unsure about this patch.I don't quite remember, who did put in these commetn lines on retaining the window, but I would expect that there were reasons for it at that time. Still they may have been the now redundant solution of an already resolved problem. Hi Fred, I think you wrote the comments, here is the ChangeLog entry which introduced this 'retain' line : 2003-08-30 Fred Kiefer <[EMAIL PROTECTED]> * Source/NSWindowController.m Changed [setWindow:] to manage the notification connection to the window. [initWithWindow:], [dealloc] and [_windowWillClose:] now use [setWindow:]. [setDocument:] now no longer retains the document, as this is already retaining the window controller. Simplified [loadWindow] by using the method [windowNibPath]. You can take a look at the related diff with this link : svn.gna.org/viewcvs/gnustep/libs/gui/trunk/Source/ NSWindowController.m?r2=17574&rev=17574&r1=16917&dir_pagestart=150> The previous comment makes a bit more sense (unlike the most recent one) : /* * If the window is set to isReleasedWhenClosed, it will release * itself, so nil out our reference so we don't release it again * * Apple's implementation doesn't seem to deal with this case, and * crashes if isReleaseWhenClosed is set. */ */ _window = nil; However I think it's not needed because there is no 'retain' or 'release' calls on _window in NSWindowController.m (not even in - dealloc method). Did you do sufficent tests with your patch? As there aren't that many document based applications for GNUstep, we really shoud try all of them, before releasing the patch. I have done far more extensive tests right now and encountered no issues at all with applications like ProjectCenter, Project Manager, Gorm (going to try a few more). Thanks, Quentin. -- Quentin Mathé [EMAIL PROTECTED] ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
