On 18 Jan 2014, at 12:09, btc@openinworld.com wrote:

PharoLauncher is currently leaking AnnouncementSubscriptions when its window closes. I'm not sure with Spec how to ensure actions are performed when the window is closed, but I have been able to achieve this with the following modification [1] which is...
-----
  WindowModel>>windowIsClosing
      isClosedHolder value: true
      self model windowIsClosing "<----proposed modification"
-----

I investigated with a halt in SpecWindow>>close to trace through and discover that it calls...
SpecWindow(StandardWindow)>>delete, which in my non-fullscreen-case calls...
SpecWindow(SystemWindow)>>delete, which "model windowIsClosing" calls...
MorphicWindowAdaptor>>windowIsClosing, which "self model windowIsClosing" calls...
WindowModel>>windowIsClosing, which does only "isClosedHolder value: true"

That last line happens to hold aPharoLauncher in its 'model' instance variable, so with the above proposal I can implement PharoLauncher>>windowIsClosing to unregister the announcer.
Any problem expected with that modification?

[1] https://pharo.fogbugz.com/f/cases/12677

cheers -ben

Benjamin wrote:

I think i would be more elegant that the model register to the isClosedHolder changes :)

Ben

Hi Ben,

I've gave that a go and had some success, but it raised an architectural question I ask near the end. I've documented my investigation here in case it is of use to others new to Spec or the Spec documentation being developed.

So references to i-var 'isClosedHolder' indicate this operates through #whenClosedDo:
which has three senders:
* InspectorNavigator>>initialize
* SpecDebugger>>initialize
* SpecPreDebugWindow>>initialize
All of which use the pattern...
    self whenWindowChanged: [ :w | w whenClosedDo: [ "do stuff" ] ].

I understand this I inserted the following breakpoints...
InspectorNavigator>>initialize
    ...
    self haltOnce. self whenWindowChanged: [ :w | "outerBlock" self halt. w whenClosedDo:
            [ "innerBlock" self halt. self inspector close ] ].

After enabling haltOnce and doing "1 inspect"...

Trace Part 1.
The first breakpoint (haltOnce) occurs in InspectorNavigator(ComposableModel)>>whenWindowChanged:
At this point before the window is created, the i-var 'window' holds "NewValueHolder [ nil ]".
A few steps later it registers "outerBlock" on i-var 'window' for announcement ValueChanged .
It is interesting that the action is registered before the window is opened/exists.

Trace Part 2.
After a <proceed>, the "outerBlock" breakpoint occurs.
Down(or up?) the stack InspectorNavigator(ComposableModel)>>openWithSpecLayout:
does "window value: (WindowModel new model: self)"
so that a few steps up NewValueHolder>>valueChanged: effectively announces ValueChanged on i-var 'window' .
such that WindowModel>>whenClosedDo:
executes to register the "innerBlock" on WindowModel's i-var 'isClosedHolder' for announcement ValueChanged.
The inspector then appears.

Trace Part 3.
When the close button on the inspector is clicked, the "innerBlock" breakpoint occurs.
Down the stack WindowModel>>windowIsClosing
does "isClosedHolder value: true"
so that a few steps up NewValueHolder>>valueChanged: announces ValueChanged on i-var 'isClosedHolder'
such that "self inspector close" is executed.

Armed with that understanding I found the following worked fine...
    PharoLauncher>>initialize
        super initialize.
        self whenWindowChanged: [ :w | w whenClosedDo: [ self releaseSubscriptions ] ].
with support from
    PharoLauncher>>releaseSubscriptions
        imagesModel releaseSubscriptions
    PhLTitledTreeModel>>releaseSubscriptions
        self repository unsubscribe: self.

Where PharoLauncher and PhLTitledTreeModel are both subclassed from ComposableModel, and PhLTitledTreeModel is composed into PharoLauncher by the i-var 'imagesModel'.

However I could not help make the comparison of the simplicity of:
    PharoLauncher>>windowIsClosing
       self releaseSubscriptons
which is 1 stack level in the debugger away from WindowModel>>windowIsClosing and took about an hour to track down and understand...

versus isClosedHolder:
which has 22 stack levels for both Trace Part 1 and Trace Part 2 from the two levels of redirection with Announcements and taken many hours of play to get to my current point of understanding.

So what is gained in elegance from doing it the isClosedHolder way?

I thought that maybe... since all these components are meant to be composable, PhLTitledTreeModel should be self-contained about about releasing its subscriptions, by registering on isClosedHolder itself at the point where the subscription-of-concern is created. Otherwise relying on its owner PharoLauncher to call PhLTitledTreeModel >>releaseSubscriptions means if PhLTitledTreeModel is composed elsewhere in ClassX, then the onus is on ClassX to do the same thing. I had hoped that adding the last line here would work...
    PhLTitledTreeModel>>repository: aRepository
        repositoryHolder value: aRepository.
        self repository whenChangedSend: #refresh to: self. "<--subscription-of-concern"
        self refresh.
        self whenWindowChanged: [ :w | w whenClosedDo: [ self repository unsubscribe: self ] ]. "<--added line"

since that would be quite elegant! However it doesn't work since tracing through I find that the PhLTitledTreeModel i-var 'window' never changes from nil. To work it seems like PhLTitledTreeModel i-var 'window' should be the same WindowModel as its owner PharoLauncher's i-var 'window'.

So the question this leads up to is...
    Should child ComposableModels all reference the same WindowModel as their owner?

To my naive view (and direct need here) it makes intuitive sense that they should. I envisage the case in a dockable UI where ComposableModels might be dragged out from being a subpane to having a window of their own, where you would want the call to #releaseSubscriptions to be independent of the owner.

Thanks for your patience on that long writeup. I hope its clear - otherwise maybe we could communicate on IM or screenshare.
cheers -ben