Re: [PATCH] Minor fix in NSWindowController

[Quentin Mathé]

Le 19 avr. 06 à 02:36, Fred Kiefer a écrit :

> Quentin Mathé wrote:
> > I think you wrote the comments, here is the ChangeLog entry which
> > introduced this 'retain' line : 2003-08-30  Fred Kiefer
> > <address@hidden>
> >
> > * 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?

I have done new tests with other applications today like Notebook,  
Ink and ToolbarExample (which is document based). With the two last  
ones, the result is a segmentation fault on a document window  
close :-/. It's retain/release issue. Now I think that's the extra  
'retain' I removed in NSWindowController is just covering another bug  
(may be more). I would say it's probably related to - 
isReleasedWhenClosed value not properly ignored everywhere when  
NSWindowController is used in NSDocument architecture context. What's  
your take on this ?

> > 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:.

Yes, I just forgot to look for 'ASSIGN' in the code.

> 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...

You are right, both problems are quite similar, I think bug 4840  
isn't reintroduced by this extra retain call because _window ivar is  
set to nil and the code is probably checking this ivar (or the  
related NSDocument instance) in order to know whether any documents  
are still open (before displaying the 'save' alert).

In the bug report, you made the following comment : 'This behaviour  
is actually correct, as long as the window hasn't set  
isReleasedWhenClosed. Then the document and the window controller and  
the window will hang around invisible. This does not make any sense  
to me, but is acording to the spec.'
Interestingly it seems Apple finally takes in account this border  
case. In the latest Cocoa documentation, it is stated that  - 
isReleasedWhenClosed value is ignored when you are using a window  
controller together with NSDocument and related classes.

Quentin.

--
Quentin Mathé
address@hidden