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.

Sign up for GitHub

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

Jump to bottom

Make the default window title more descriptive #3404

Closed
wants to merge 3 commits into from
Closed

Make the default window title more descriptive #3404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1885bf6
Make the default window title more descriptive
DJMcNab Dec 20, 2021
c9b781c
Migrate to a sentinal value
DJMcNab Dec 20, 2021
36e6908
Rename to `title_or_default`
DJMcNab Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_window/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ bevy_app = { path = "../bevy_app", version = "0.5.0" }
bevy_math = { path = "../bevy_math", version = "0.5.0" }
bevy_utils = { path = "../bevy_utils", version = "0.5.0" }
raw-window-handle = "0.4.2"
once_cell = "1.9.0"

# other

Expand Down
23 changes: 21 additions & 2 deletions crates/bevy_window/src/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use bevy_math::{DVec2, IVec2, Vec2};
use bevy_utils::{tracing::warn, Uuid};
use once_cell::sync::Lazy;
use raw_window_handle::RawWindowHandle;

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -216,7 +217,7 @@ impl Window {
resize_constraints: window_descriptor.resize_constraints,
scale_factor_override: window_descriptor.scale_factor_override,
backend_scale_factor: scale_factor,
title: window_descriptor.title.clone(),
title: window_descriptor.title_or_default().to_owned(),
vsync: window_descriptor.vsync,
resizable: window_descriptor.resizable,
decorations: window_descriptor.decorations,
Expand Down Expand Up @@ -540,6 +541,7 @@ pub struct WindowDescriptor {
pub position: Option<Vec2>,
pub resize_constraints: WindowResizeConstraints,
pub scale_factor_override: Option<f64>,
/// For reading, prefer title_or_default
pub title: String,
pub vsync: bool,
pub resizable: bool,
Expand All @@ -559,10 +561,27 @@ pub struct WindowDescriptor {
pub canvas: Option<String>,
}

static DEFAULT_WINDOW_TITLE: Lazy<String> = Lazy::new(|| {
std::env::current_exe()
.ok()
.and_then(|it| Some(format!("{} - bevy", it.file_stem()?.to_string_lossy())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually very cool with "debranding" here / only using "bevy" in cases where the executable name cannot be found. I think we should remove the "- bevy" suffix unless someone has a counterargument.

Copy link
Member Author

@DJMcNab DJMcNab Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My argument would be that developers should change the default title; - bevy makes it clear that it's a 'temporary/working' title.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it does help anchor the window to a "bevy development" context and provides incentive for people to manually set the title. It just also increases the odds of people accidentally leaking the "bevy" context in their game releases, which is something I want to avoid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah having thought about this more, I really think we should just use the binary name by itself to avoid this class of mistake / avoid leaking the bevy context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the default is still a risk of bevy context leakage no matter what; std's docs on current_exe state that it is not unlikely that it will fail, and whenever it does it would rename to bevy.

That is, I'm still not convinced.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't see the benefit to including - bevy. "Making it clear that its a temporary / working title" also translates to "makes it look more like a temporary / working title if you deploy with it". Why actively make apps "less release ready" when there is a very solid chance that games will be released without setting the title, even with the - bevy? If the goal is to make it clear a title is temporary and create incentives for people to change it, should we just go all in and call it TEMPORARY_BEVY_WINDOW_TITLE_PLEASE_CHANGE? (clearly we shouldn't, I'm just going to extremes to make a point).

.unwrap_or_else(|| "bevy".to_string())
});

impl WindowDescriptor {
pub fn title_or_default(&self) -> &str {
if self.title.is_empty() {
&DEFAULT_WINDOW_TITLE
} else {
&self.title
}
}
}

impl Default for WindowDescriptor {
fn default() -> Self {
WindowDescriptor {
title: "bevy".to_string(),
title: "".to_string(),
width: 1280.,
height: 720.,
position: None,
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_winit/src/winit_windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ impl WinitWindows {
};

#[allow(unused_mut)]
let mut winit_window_builder = winit_window_builder.with_title(&window_descriptor.title);
let mut winit_window_builder =
winit_window_builder.with_title(window_descriptor.title_or_default());

#[cfg(target_arch = "wasm32")]
{
Expand Down