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.

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

Add name to bevy::window::Window #7650

Merged
merged 22 commits into from
Feb 5, 2024

Conversation

VitalyAnkh
Copy link
Contributor

@VitalyAnkh VitalyAnkh commented Feb 13, 2023

Objective

Solution

  • Add name field to bevy::window::Window. Specifying this field adds different properties to the window: application ID on Wayland, WM_CLASS on X11, or window class name on Windows. It has no effect on other platforms.

Changelog

Added

  • Add name to bevy::window::Window.

Migration Guide

  • Set the bevy_window::Window's name field when needed:
App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                title: "I am a window!".into(),
                name: Some("SpaceGameCompany.SpaceShooter".into()),
                ..default()
            }),
            ..default()
        }))
        .run();

@VitalyAnkh VitalyAnkh force-pushed the add_wm_class branch 2 times, most recently from 8569e8a to 80d04b6 Compare February 13, 2023 05:03
@VitalyAnkh VitalyAnkh added O-Linux Specific to the Linux desktop operating system A-Windowing Platform-agnostic interface layer to run your app in labels Feb 13, 2023
crates/bevy_winit/src/winit_windows.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
@@ -191,6 +193,7 @@ impl Default for Window {
fn default() -> Self {
Self {
title: "Bevy App".to_owned(),
app_id: "bevy.app".to_owned(),
Copy link
Member

Choose a reason for hiding this comment

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

is it an issue if several apps running at the same time have the same app_id? Would it be better as an option, and only set it when the user actually put a value?

Choose a reason for hiding this comment

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

drive-by comment: It's not semantically invalid, but it's not useful to have a bunch of unrelated programs with the same app_id. So I'd say that it should be an Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this an Option<String> type and None by default now.

winit_window_builder = winit::platform::wayland::WindowBuilderExtWayland::with_name(
winit_window_builder,
window.app_id.clone(),
"",
Copy link
Member

Choose a reason for hiding this comment

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

this value should be explained

winit_window_builder = winit::platform::x11::WindowBuilderExtX11::with_name(
winit_window_builder,
window.app_id.clone(),
"",
Copy link
Member

Choose a reason for hiding this comment

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

this value should be explained

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the docs on the field be already enough?

@mockersf
Copy link
Member

is it an issue to have both x11 and wayland features enabled, and have both methods called?

@deifactor
Copy link

is it an issue to have both x11 and wayland features enabled, and have both methods called?

From looking at the winit code, no: both of them set platform_specific.name in the same way.

@james7132 james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 5, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 5, 2023
@mockersf mockersf removed this from the 0.11 milestone Jun 29, 2023
@VitalyAnkh
Copy link
Contributor Author

I'm very sorry for such a long delay! After almost a year, I finally remembered this PR. I've rebased it onto the latest bevy and updated some documentation. The concept of application ID or wm_class is only applicable to UNIX platforms, and currently, winit does not provide a way to modify it after the app has started. This feature is quite for many window managers to manage windows. Please review the current code again, thank you very much!

@musjj
Copy link

musjj commented Jan 31, 2024

The concept of application ID or wm_class is only applicable to UNIX platforms

The equivalent of this on Windows is window class names, which winit also supports:

with_class_name

@VitalyAnkh
Copy link
Contributor Author

VitalyAnkh commented Jan 31, 2024

@musjj Thanks! Didn't find this before. I will update this patch ASAP.

VitalyAnkh and others added 3 commits January 31, 2024 23:26
Co-authored-by: François <mockersf@gmail.com>
Co-authored-by: François <mockersf@gmail.com>
@VitalyAnkh VitalyAnkh changed the title Add app_id to bevy::window::Window Add name to bevy::window::Window Jan 31, 2024
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

unable to test, but code looks good

@VitalyAnkh VitalyAnkh added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 1, 2024
@VitalyAnkh VitalyAnkh removed the request for review from deifactor February 2, 2024 16:18
/// - **`macOS`**, **`iOS`**, **`Android`**, and **`Web`**: not applicable.
///
/// Notes: Changing this field during runtime will have no effect for now.
pub name: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a &'static str instead? Given that it's not supposed to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done in the following commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it failed to build after turning the type of name into Option<&'static str>:

error: implementation of `cursor::_::_serde::Deserialize` is not general enough
   --> crates/bevy_window/src/window.rs:121:35
    |
121 | #[derive(Component, Debug, Clone, Reflect)]
    |                                   ^^^^^^^ implementation of `cursor::_::_serde::Deserialize` is not general enough
    |
    = note: `window::Window` must implement `cursor::_::_serde::Deserialize<'0>`, for any lifetime `'0`...
    = note: ...but it actually implements `cursor::_::_serde::Deserialize<'static>`
    = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `bevy_window` (lib) due to previous error

How to #[derive(Reflect)] a type with a component whose type is Option<&'static str>? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bevy_reflect supports type with 'static lifetimes, but the situation here seems more complex. Reverted it because I'm not familiar with the mechanism of bevy_reflect. Maybe add this optimization later.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see, no worries. It seems like Reflect was implemented for &'static str in #11686 so it's a bit strange that it still won't work.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 5, 2024
Merged via the queue into bevyengine:main with commit 7705c1d Feb 5, 2024
23 checks passed
lynn-lumen pushed a commit to lynn-lumen/bevy that referenced this pull request Feb 5, 2024
# Objective
- Fixes  bevyengine#4188, make users could set application ID for bevy apps.

## Solution

- Add `name` field to `bevy::window::Window`. Specifying this field adds
different properties to the window: application ID on `Wayland`,
`WM_CLASS` on `X11`, or window class name on Windows. It has no effect
on other platforms.
---

## Changelog

### Added
- Add `name` to `bevy::window::Window`.

## Migration Guide

- Set the `bevy_window::Window`'s `name` field when needed:
```rust
App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                title: "I am a window!".into(),
                name: Some("SpaceGameCompany.SpaceShooter".into()),
                ..default()
            }),
            ..default()
        }))
        .run();
```

---------

Co-authored-by: François <mockersf@gmail.com>
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective
- Fixes  bevyengine#4188, make users could set application ID for bevy apps.

## Solution

- Add `name` field to `bevy::window::Window`. Specifying this field adds
different properties to the window: application ID on `Wayland`,
`WM_CLASS` on `X11`, or window class name on Windows. It has no effect
on other platforms.
---

## Changelog

### Added
- Add `name` to `bevy::window::Window`.

## Migration Guide

- Set the `bevy_window::Window`'s `name` field when needed:
```rust
App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                title: "I am a window!".into(),
                name: Some("SpaceGameCompany.SpaceShooter".into()),
                ..default()
            }),
            ..default()
        }))
        .run();
```

---------

Co-authored-by: François <mockersf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide O-Linux Specific to the Linux desktop operating system S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet