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

[koendehondt]

@estebanlm This issue cannot be closed. The original issue is not fixed, as you can see in this video:

https://github.com/user-attachments/assets/a679a91d-2078-4068-bf47-4c1989edc2d0

[koendehondt]

@estebanlm Please take a look at this issue again because readers of the Spec book stumbled against it: https://github.com/SquareBracketAssociates/BuildingApplicationWithSpec2/issues/124

cc @Ducasse

[estebanlm]

this thing is still there? I thought it was fixed, sorry. I will check at it this monday.

[estebanlm]

this problem is fixed in P13. Do we need a backport? Is that what you ask for?

[estebanlm]

https://github.com/pharo-project/pharo/pull/17834

[Ducasse]

Ah then probably. Because people are reading the P12 book.

[koendehondt]

Thank you for the backport, @estebanlm. It will help the readers of the book, which is based on P12.

[koendehondt]

I verified the fix in the latest stable version of Pharo 12. All ok. I will close this ticket.