whenWillCloseDo: block is evaluated twice when closing a window

[koendehondt]

Context: Pharo 12.

Consider this code:

| windowPresenter presenter  |
presenter := SpPresenter new.
presenter layout: SpBoxLayout newTopToBottom.
windowPresenter := presenter open.
windowPresenter whenWillCloseDo: [ :announcement |
    (presenter confirm: 'Are you sure that you want to close the window?')
        ifFalse: [ announcement denyClose ] ]

Be aware that a SpWindowWillClose announcement has a default of allowing a window to close (see SpWindowWillClose>>#initialize). That is why the code above does not send announcement allowClose.

• Run the code
• Click the close box of the window.
• A confirmation dialog appears.
• Click the "Yes" button in the confirmation dialog.
• (Bug) The confirm dialog is opened again.

Here is the reason:

SystemWindow>>closeBoxHit
    "The user clicked on the close-box control in the window title.
    For Mac users only, the Mac convention of option-click-on-close-box is obeyed if the mac option key is down.
    If we have a modal child then don't delete.
    Play the close sound now since this is the only time we know that the close is user-initiated."

    self allowedToClose ifFalse: [^self].
    self playCloseSound.
    self close

The method closeBoxHit sends close:

SpWindow>>close 

    self announceWillClose ifFalse: [ ^ self ].
    super close.

close sends the announcement:

SpWindow>>announceWillClose
    | announcement |

    announcement := SpWindowWillClose new
        window: self;
        yourself.
    self announce: announcement.
    self currentWorld announcer announce: announcement.
    ^ announcement canClose 

That will trigger the whenWillCloseDo: block in the code snippet.

SpWindow>>close does a super send, because self announceWillClose evaluates to true (the user clicked "Yes").

The method in the superclass is:

SystemWindow>> close
    ^ self delete

and delete is:

StandardWindow>>delete
    "If fullscreen remove the owner too."

    self mustNotClose ifTrue: [^ self].
    self model ifNotNil: [
        self model okToChange ifFalse: [ ^ self ].
        self model okToClose ifFalse: [ ^ self ] ].
    self isFullscreen
        ifTrue: [self owner delete]
        ifFalse: [super delete]

It does super send on the last line:

SystemWindow>>delete
    "Should activate window before asking model if okToChange
    since likely that a confirmation dialog will be requested.
    Don't if not owned by the world though."

    "in case we add some panes and reopen!"
    self isCloseable
        ifFalse: [ ^ self ].
    self deleteDiscardingChanges

The last line sends deleteDiscardingChanges:

SpWindow>>deleteDiscardingChanges

    self announceWillClose.
    ^ super deleteDiscardingChanges

This method announces the window close again, as SpWindow>>close already did. That results in opening the confirmation dialog again.

[Ducasse]

I extracted

announceWillClose

    | announcement |
    announcement := WindowClosed new
        window: self;
        yourself.
    self announce: announcement.
    self currentWorld announcer announce: announcement

deleteDiscardingChanges
    | thisWorld announcement |
    self removePaneSplitters.   "in case we add some panes and reopen!"
    thisWorld := self world.
    self isFlexed
        ifTrue: [ owner delete ]
        ifFalse: [ super delete ].
    model
        ifNotNil: [ model
                windowIsClosing;
                releaseActionMap ].
    model := nil.
    SystemWindow noteTopWindowIn: thisWorld.
    self announceWillClose

[Ducasse]

Now this is not working because the the SpDialogPresenter calls SpDialogWindowMorph which is a subclass of WindowMorph and not SpWindow

[estebanlm]

I need to discuss this, I am not sure we need deleteDiscardingChanges (and even delete is not recommended in spec, there is a reason why is on private method). For me this is a bug on the closeBoxHit method, that needs to be override in SpWindow to fix the problem, other than make a pump up of a specific functionality/problem of the morphic backend.

[Ducasse]

Ok fine by me. We struggled a lot on this bug and discussed also with @tesonep