Add resizable flag and windowmode option to window creation #149
Conversation
karroffel
added
C-Feature
|
Welcome to bevy @joejoepie and thanks so much for your first open source contribution! This looks pretty nice and useful! (unfortunately it might take a tiny bit before it gets reviewed/merged, things are a bit busy with the sudden interest of so many awesome people 💙 ) |
Thank you @karroffel ! Of course i understand these things take time. But I'm very happy that the project gets the attention it deserves. I've rarely encountered a project where it's this easy to jump in and contribute! |
|
Hi @joejoepie, My #160 was just merged in that conflicts with this PR. If you use the Thanks! |
Thanks for letting me know, i've update the PR. |
| .unwrap(); | ||
| winit_window_builder = winit_window_builder.with_title(&window.title); | ||
|
|
||
| let winit_window = match window.mode { |
There was a problem hiding this comment.
I would recommend making this match return the builder so you can just call build once at the end.
| let winit_window = match window.mode { | ||
| WindowMode::BorderlessFullscreen => { | ||
| let winit_window = winit_window_builder.build(&event_loop).unwrap(); | ||
| winit_window.set_fullscreen(Some(winit::window::Fullscreen::Borderless( |
There was a problem hiding this comment.
instead of set_fullscreen you can call with_fullscreen to continue chaining onto the builder.
I imagine as it is this would probably flash a non-fullscreen window first.
I think you can get the monitors from EventLoopWindowTarget since they don't exist on the builder. It has available_monitors and primary_monitor.
| @@ -17,18 +17,39 @@ impl WinitWindows { | |||
| #[cfg(target_os = "windows")] | |||
| let winit_window_builder = { | |||
There was a problem hiding this comment.
This would have to be mut too for the windows configuration.
|
@jakerr Thanks so much for reviewing my PR! I didn't realize the video mode could be retrieved using the event_loop. The resulting code is a lot more compact as well! |
|
@joejoepie My pleasure! You might want to squash your commits since the first one would be a compiler error on windows. It's nice to try to avoid any broken commits even if part of the same PR so that if someone goes digging in history with I'm as new to this project as you are so I don't have much of an informed opinion on tradeoff with the breaking API change so I'll defer to the maintainers there. Code looks good to me though 👍 |
|
I'd rather do a squash merge to master than in my PR. I don't think rewriting the history in a PR has much use. It might also be less clear for people trying to follow along the comment and commit history :) |
Interesting! I wasn't so familiar with that feature. Haven't been on github in quite a while. That said isn't it up to the person accepting the pull request to select the merge option? I wonder what the culture is around that. Do people just put a note in the comments like this or leave it to person accepting the PR to determine if the history should be kept or squashed? Your point about making the review more clear is a good one though. It definitely was convenient for me reviewing this PR to see the progress. |
Now that i look at it, the default on github isn't squash merge, but rather merging into a single commit. So this means the branches are still merged, but it will always only result in one commit on the target branch. So for your use case - git bisect - this would still perfectly work, since you'll get all the changes in one commit. Nobody wants to visit every single branch ever used when doing history research work anyway 😅 |
|
Just tested on my end. Looks good! |
Hello, I'm very impressed by the project and it was very easy to understand the code.
That is why this is my first ever open source contribution!
I've tested the changes on the windowing examples.
The only issue i see is that this change is non-backwards compatible.
Short explanation for (very basic) video mode selection when using Fullscreen: