Skip to content

On macOS, fix WindowEvent::Destroyed isn't emitted #2544

Closed
wusyong wants to merge 4 commits into
rust-windowing:masterfrom
wusyong:mac-destroy
Closed

On macOS, fix WindowEvent::Destroyed isn't emitted #2544
wusyong wants to merge 4 commits into
rust-windowing:masterfrom
wusyong:mac-destroy

Conversation

@wusyong
Copy link
Copy Markdown
Contributor

@wusyong wusyong commented Nov 2, 2022

It seems windowWillClose can only be called when windowShouldClose returns true.
So the WindowEvent::Destroyed is never called when the window is dropped.
This PR add another queue event when the window is dropped during Drop trait is called.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, odd that this hasn't been discovered before! Does this mean that our overriding of windowWillClose: is effectively useless?

However, I doubt this fixes the problem completely, since the window may be closed before the last reference to it is destroyed (for example in the window_debug example). Do you have an idea for how to fix this?

Comment thread src/platform_impl/macos/util/async.rs Outdated
Comment on lines +231 to +235
let event = Event::WindowEvent {
window_id: WindowId(window.id()),
event: WindowEvent::Destroyed,
};
AppState::queue_event(EventWrapper::StaticEvent(event));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Use window.emit_event

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only WinitWindowDelegate has this method. Should I add such method to WinitWindow too? Or could I just access WinitWindowDelegate by pub(crate) it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do pub(crate)

Copy link
Copy Markdown
Contributor Author

@wusyong wusyong Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the delegate has to be used in exec_async somehow, so windowWillCose can be invoked.
So I call delegate.window.close() instead which should look a little bit cleaner.

@wusyong
Copy link
Copy Markdown
Contributor Author

wusyong commented Nov 3, 2022

Does this mean that our overriding of windowWillClose: is effectively useless?

I couldn't find any other way to trigger windowWillClose: than returning true in windowShouldClose:.

However, I doubt this fixes the problem completely, since the window may be closed before the last reference to it is destroyed (for example in the window_debug example). Do you have an idea for how to fix this?

Yeah, Destroyed event won't be sent in this case but it's consistent across platforms (at least on x11 and windows). But I assume that's okay if you are already going to exit. In my use case, I treat Destroyed event as a way to gracefully shutdown. I have a collection of window (HasMap for example). CloseRequested will drop the window in the collection and set_exit if the collection is empty during Destroyed.

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Nov 3, 2022

I couldn't find any other way to trigger windowWillClose: than returning true in windowShouldClose:.

Then we should maybe remove the code in it, along with a note? Though I'm a bit weary about doing so, since it seems like, see this commit from 2018 that added the note about El Capitan goddessfreya@8e9fc01 (from PR #853).

@francesca64 do you remember anything about this?

Yeah, Destroyed event won't be sent in this case but it's consistent across platforms (at least on x11 and windows).

Would you mind documenting this on Event::Destroyed, then?

@francesca64
Copy link
Copy Markdown
Member

@madsmtm I sadly don't remember, sorry!

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Nov 4, 2022

No worries, thanks for the quick response!

@kchibisov kchibisov requested a review from madsmtm November 20, 2022 13:03
@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Nov 29, 2022

I've discovered that windowWillClose: is called if you run close: synchronously, which means this should be fixed by #2574, could I get you to verify that @wusyong?

@madsmtm madsmtm added the DS - appkit Affects the AppKit/macOS backend label Nov 29, 2022
@wusyong
Copy link
Copy Markdown
Contributor Author

wusyong commented Nov 30, 2022

@madsmtm I slightly modified multiwindow example and check windows hash map on Destroyed event.
It can work based on your PR! I can close this one if you are going to merge #2574.

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Nov 30, 2022

Cool, I went and merged that so will close this now.

Thanks for your help!

@madsmtm madsmtm closed this Nov 30, 2022
@wusyong wusyong deleted the mac-destroy branch November 30, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DS - appkit Affects the AppKit/macOS backend

Development

Successfully merging this pull request may close these issues.

3 participants