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

Don't panic when reentering wnd_proc due to mouse move or timer #130

Closed
wants to merge 1 commit into from

Conversation

helgoboss
Copy link

Explanation:

  • Reentering wnd_proc can happen, e.g. when opening the system web browser in an event handler via webbrowser crate.
  • That's why this PR uses try_borrow_mut to fall back to the default window procedure if wnd_proc is reentered.
  • I didn't convert each borrow_mut to try_borrow_mut, just the ones necessary to make the webbrowser crate work, see this PR for egui-baseview: Add support for opening egui links in browser BillyDM/egui-baseview#12.

- This can happen, e.g. when opening the system web browser in an event
  handler via webbrowser crate.
- Execute the default window procedure in that case.
@glowcoil
Copy link
Member

glowcoil commented Oct 9, 2022

This is a similar issue to what occurs when using an external library to open a native file dialog (e.g. with the rfd crate). While obviously panicking is not ideal, simply falling back to the default WNDPROC is not really a complete solution either, since it will result in those reentrant events simply getting dropped on the floor without even being logged. As it is, at least panicking makes it obvious when this happens.

I'm also reluctant to merge this PR since it only changes some window events to use try_borrow_mut but leaves others as borrow_mut, so it's still perfectly possible to cause a panic with a reentrant event loop, it will just happen less often than timer and mouse move events.

One possible idea for a more complete solution would be to add a defer method to Window that takes a closure which will run after the current event, so that if that deferred closure results in reentrant events being delivered to the WNDPROC, the WindowHandler RefCell can be borrowed mutably to handle those events without issue.

A less involved stopgap solution would be to replace all uses of borrow_mut with try_borrow_mut, bring in the log crate, and log a warning when reentrant events are unable to be handled.

@helgoboss
Copy link
Author

Thanks for the reply. Yes, I'm aware of these downsides. It was intended as a quick solution that improves one particular issue, without going "all in" ;) But of course, I would also prefer a more thorough solution.

For me, the defer approach sounds most reasonable.

@helgoboss
Copy link
Author

@glowcoil I would like to work on the "defer" approach. Or are you already at it? If not, I think this defer mechanism can be something purely internal. I would check via try_borrow_mut() if it's a reentrant event and in that case enqueue the code for later execution, triggered by the timer. As queue data structure I could use VecDeque. Any objections?

@glowcoil
Copy link
Member

I won't have a chance to work on this in the near future, so feel free to give it a try. However, I don't think that internally deferring events is an acceptable solution.

In the case of long-running reentrant event loops, like file dialogs, that approach will queue up events for as long as the file dialog is open and then dispatch them all at once immediately after the dialog is closed, which is definitely not desirable behavior. A secondary issue is that some functionality requires synchronously responding to events before returning from the WNDPROC, e.g. deciding whether to capture keypresses or allow them to propagate (see the EventStatus enum returned from the on_event callback), as well as proper synchronization between rendering and window resizes (see discussion here and some relevant code here), so it's not a good idea to queue up events and defer them until later for that reason.

druid-shell does have a solution involving only internal buffering, but rather than buffering events, it buffers operations which can cause events reentrantly (see DeferredOp). However, this doesn't solve the problem of external libraries which enter reentrant event loops (like rfd's synchronous API or the webbrowser crate); it only solves it for druid-shell's own built-in implementations of file dialogs and similar, and using an external library which launches a reentrant event loop from inside an event handler will still result in events being dropped on the ground.

You can think of the Window::defer method I proposed above as basically a pluggable version of druid-shell's DeferredOp which can be used for integrating arbitrary third-party libraries.

@helgoboss
Copy link
Author

I'm going to open a new PR which uses the deferred_task queue added in #136.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants