PolyMeilex / rfd

Rusty File Dialog
MIT License
574 stars 64 forks source link

Spawning an `AsyncMessageDialog` in response to a winit WindowEvent::DroppedFile event does not work properly #6

Closed francesco-cattoglio closed 3 years ago

francesco-cattoglio commented 3 years ago

I don't know if this is a rfd or a winit bug, but if you drop a file onto a winit window on MacOS and try to show a dialog as a response, the AsyncMessageDialog does not work. Instead, the "Hi! It looks like you are running async dialog in unsuported environment, I will fallback to sync dialog for you." gets printed. I think this is because the app.key_window() at line 58 of modal_future.rs is actually null. It is easy to reproduce this by making a small change to the winit example, see my branch: https://github.com/francesco-cattoglio/rfd/tree/dropped_file_bug To reproduce, just start the example and drag any file from the MacOS Finder onto the window.

PolyMeilex commented 3 years ago

First, I haven't checked if sdl2::event::Event::DropFile works the same way. (yet)

So... I don't see any solution for this yet, the only solution I have in mind now is adding rfd::set_root_window() fn to the API, that user would call after winit window creation. Or adding set_root_window to dialog builder, but that's problematic as users don't always have access to their winit window.

But this sounds more like yet another workaround rather than a fix... And I'm kinda tired of adding more and more workarounds one on top of another in macOS backend.

francesco-cattoglio commented 3 years ago

First, I haven't checked if sdl2::event::Event::DropFile works the same way. (yet)

That might be interesting as an alternative to winit, if you happen to take a look let me know the results.

But this sounds more like yet another workaround rather than a fix... And I'm kinda tired of adding more and more workarounds one on top of another in macOS backend.

I can agree. The entire idea that we cannot run sync dialogs on OsX on Winit feels like something that should be fixed in Winit itself.

PolyMeilex commented 3 years ago

Speaking of sync dialogs... every occasion is a good occasion to bump https://github.com/rust-windowing/winit/issues/1779 XDD

Jokes aside, I will try to research this one more, (my hope is that it is not winit related) and If I figure something out I'll let you know.

PolyMeilex commented 3 years ago

So... it's not winit related, I acually like the way winit handles this a lot more than SDL2 (don't want to go to deep in to details)

The problem is purely related to the use of keyWindow, as long as you have your window in focus everything works as expected.

mainWindow sadly works the same way, so we can't use it. windows array is our last chance, but it will be problematic in multi window setups. User specified parent window is also an option, but that also gets problematic fast, for example if someone is using something like iced, winit windows is not so easy to acquire, and as far as I remember it's also no clonable.

francesco-cattoglio commented 3 years ago

So, if I understand this correctly, this issue would happen every time we open any dialog while the window is in the background, or minimized (e.g: when opening a dialog on a timer), right? This sounds like a big thorn in the side for any user that might need to do anything out of the ordinary. Perhaps this really needs attention from winit devs, since workarounds are getting ugly really quickly.

PolyMeilex commented 3 years ago

Just to be clear all the async shenanigans has nothing to do with winit, it's entirely my responsibility to make this work, but the question is, would rfd ever need async support if winit sync dialogs were not fundamentally broken... probably not.

So with that out of the way (once again it's not winit related), my plan is to...

PolyMeilex commented 3 years ago

So, I introduced 2 fixes for this:

  1. If you are using a single window, you can leave everything as is, and it should just work.
  2. If you need to specify which window rfd should use, you can by calling AsyncDialog::set_parent(&winit_window)

Both are available on master, I would appreciate if you could cross check those on your machine, just to make sure that everything works as expected.

The quickest way would probably be by playing around with winit-example, comment out set_parent to check if autodetection works, and uncomment to check if explicit parent works.

francesco-cattoglio commented 3 years ago

I have tested this on my three machines, one Linux, one W10, one MacOS, and I can confirm that the winit-example works, both with and without the set_parent() line. I also tested my program and can confirm that the issue is gone!

PolyMeilex commented 3 years ago

Thanks, I will publish a new version shortly

francesco-cattoglio commented 3 years ago

before you do, there is one other save-file related thing I am working on windows, I will post an issue and a pull request within a couple hours at most

PolyMeilex commented 3 years ago

Sure, thanks for a notice