-
-
Notifications
You must be signed in to change notification settings - Fork 287
Fix missing egui file drag and drop events #375
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for the PR, this is a very welcome addition! I added a minor comment, but apart from that and failing CI for some reason, the PR looks good to me.
With regards to the CI stuff, I have a commit ready that would put the relevant code behind a Is that a preferable action for this? I am not well versed with wasm, ios, nor android (and I think the latter 2 wouldn't support file selection easily?) so I am curious to hear your opinion. |
Yes, let's just disable the code for those targets with |
Got CI checks just about working but running |
Not sure what's the best way to deal with it, but I usually go to the cache page and just nuke everything one by one :D |
Cargo.toml
Outdated
log_file_dnd_events = [] | ||
|
||
# specifically for feature gating `rfd` from tests | ||
file_browse = ["rfd"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure I like adding a feature that doesn't really affect the functionality, but only brings a new dependency in, which is not used outside of the dev stuff anyway... It might be confusing to users, as they would intuitively expect that this feature should either enable or disable the associated functionality in their apps.
Is there probably another way around this?
Cargo.toml
Outdated
# `file_browse` feature only, workaround dev-dependency | ||
[target.'cfg(not(any(target_os = "android", target_os = "ios", target_arch = "wasm32")))'.dependencies] | ||
rfd = { version = "0.13", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work instead? Maybe this way we can get rid of the file_browse
feature: #375 (comment).
# `file_browse` feature only, workaround dev-dependency | |
[target.'cfg(not(any(target_os = "android", target_os = "ios", target_arch = "wasm32")))'.dependencies] | |
rfd = { version = "0.13", optional = true } | |
[target.'cfg(not(any(target_os = "android", target_os = "ios", target_arch = "wasm32")))'.dev-dependencies] | |
rfd = "0.13" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I can see this was even your initial version. I assume you were trying to workaround the CI weirdness?..
So basically here the build was already failing due to the out of space error: https://github.com/MickHarrigan/bevy_egui/actions/runs/14431554045/job/40467118849
So maybe we can partially revert this one?
I've merged your PR via #377 - it takes your branch except for the last two commits. That's basically the code I wanted to see, sorry if I my comments were confusing 😄 |
Description
This PR adds functionality to pass
bevy_window::FileDragAndDrop
events down to egui. Additionally adds a bevy-ified version of the egui file_dialog example.Changes:
log_file_dnd_events
feature similar in functionality to thelog_input_events
featureWhy this is needed:
Currently when using
bevy_egui
any files that get dragged and dropped (or hovered) onto the window can only be read from directly via bevy. With this it allows passing down into the egui context for usage there.Limitations:
I don't believe that there is a concise way to handle the location of a file drop / hover, only that it exists.
This is to be fixed with the release of
winit 0.31
afaik and should eventually be covered.