Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

whenWillCloseDo: block is evaluated twice when closing a window #1547

Open
koendehondt opened this issue May 25, 2024 · 5 comments
Open

whenWillCloseDo: block is evaluated twice when closing a window #1547

koendehondt opened this issue May 25, 2024 · 5 comments

Comments

@koendehondt
Copy link
Contributor

koendehondt commented May 25, 2024

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
Copy link
Contributor

Ducasse commented Jun 28, 2024

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
Copy link
Contributor

Ducasse commented Jun 28, 2024

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

Ducasse added a commit that referenced this issue Jun 28, 2024
@estebanlm
Copy link
Member

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
Copy link
Contributor

Ducasse commented Jul 3, 2024

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

@koendehondt
Copy link
Contributor Author

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

Screen.Recording.2024-10-18.at.15.10.27.mov

@Ducasse Ducasse reopened this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants