-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Merged by Bors] - Added documentation to WindowMode to better document what 'use_size' … #3216
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.
Looks great! Thanks for your first PR :)
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.
👍
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.
With this change, the field is documented on the enum and on the variant.
I think it would be best to keep the documentation on only one place, and that would be on the field itself. This would be better presented on docs.rs, and will also be the doc that will show up in an IDE. Could you move it there?
Also, "best" is not very explicit on how it is chosen. Could you expand the explanation? It seems "best" is from
bevy/crates/bevy_winit/src/winit_windows.rs
Lines 213 to 227 in 3de391b
pub fn get_best_videomode(monitor: &winit::monitor::MonitorHandle) -> winit::monitor::VideoMode { | |
let mut modes = monitor.video_modes().collect::<Vec<_>>(); | |
modes.sort_by(|a, b| { | |
use std::cmp::Ordering::*; | |
match b.size().width.cmp(&a.size().width) { | |
Equal => match b.size().height.cmp(&a.size().height) { | |
Equal => b.refresh_rate().cmp(&a.refresh_rate()), | |
default => default, | |
}, | |
default => default, | |
} | |
}); | |
modes.first().unwrap().clone() | |
} |
so it's picking the one with the bigger height / refresh rate I think?
Or better, add the explanation as doc to this function, then link to it from the field doc.
And from CI, it seems you could run cargo fmt
😃
crates/bevy_window/src/window.rs
Outdated
/// E.g. when use_size is set to false the best video mode possible is chosen. | ||
#[derive(Debug, Clone, Copy, PartialEq)] | ||
pub enum WindowMode { | ||
Windowed, | ||
BorderlessFullscreen, | ||
/// When set to true, the window uses the given size. |
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.
This is not true, it will pick the resolution supported by the monitor that is closest to the given resolution, not the given one
…xed the code to ensure it works when creating a window
I've updated the documentation and ensured the new changes are compatible with bevy. I've tested it on a few examples and it seems to have run pretty well, but a review would be appreciated! |
bors r+ |
Nice improvements! |
This pull request aims to solve the issue of a lack of documentation in the enum WindowMode
Objective
bevy_window::window::WindowMode::Fullscreen
#3136Solution