Skip to content

Move Window creation from plugin build time to PreStartup #20019

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Jul 7, 2025

Objective

Our PrimaryWindow and its associated Window are currently spawned on plugin build time, hence making it very hard to write an observer for them like On<Add, PrimaryWindow>

Solution

  • Move them to PreStartup. Using that schedule instead of Startup because the latter is more user-facing, and the user should not have to know about the details of when exactly the window is created. It should just exist.

Testing

  • Ran a couple of window examples like the desk toy on Wayland

Additional Info

If all goes well, this change should make no difference to the user whatsoever.

@janhohenheim janhohenheim added A-Windowing Platform-agnostic interface layer to run your app in D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2025
entity_commands.insert(primary_cursor_options.clone());
}
let primary_window = primary_window.clone();
let primary_cursor_options = self.primary_cursor_options.clone().unwrap_or_default();
Copy link
Member Author

@janhohenheim janhohenheim Jul 7, 2025

Choose a reason for hiding this comment

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

This is the same behavior as before because CursorOptions is a required component of Window

@janhohenheim janhohenheim added this to the 0.17 milestone Jul 7, 2025
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I'm definitely a bit worried about this maybe breaking stuff, but the code looks good and it's really small so it will be easy to bisect.

@janhohenheim janhohenheim added the X-Contentious There are nontrivial implications that should be thought through label Jul 7, 2025
@janhohenheim janhohenheim added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2025
@janhohenheim
Copy link
Member Author

Blocking until we figure out why this even works even though RenderPlugin looks for RawHandleWrapperHolder in its build method, haha

@janhohenheim janhohenheim added the S-Blocked This cannot move forward until something else changes label Jul 7, 2025
@janhohenheim
Copy link
Member Author

Blocked by #20021

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Blocked This cannot move forward until something else changes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants