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

Wayland early display window #187

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

Conversation

wired-tomato
Copy link

Wayland early display window implementation, requested in #98

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@shartte
Copy link
Contributor

shartte commented Jul 22, 2024

Just to clarify: this does not actually work unless you manually update GLFW To 3.4?

@wired-tomato
Copy link
Author

wired-tomato commented Jul 22, 2024

the window will load up with wayland on GLFW 3.3+ (default for 1.20.1+), the icon won't load unless GLFW version 3.4+ (default for mc 1.21) is in use, everything else should work fine

@wired-tomato
Copy link
Author

notably there is a GLFW error printed to the console 65548: Wayland: The platform does not support setting the cursor position when opening inventories & some other screens, however it does not cause any crashes or actual issues and can be solved with a mixin as demonstrated in my fabric wayland compat mod WayGL

@shartte
Copy link
Contributor

shartte commented Jul 22, 2024

I am against merging this like this, since it feels extremely convoluted to set a window icon using an externally written file & spawned process.

There's apparently a Wayland proposal to be sane about setting window icons. Once that has been merged, I'd hope GLFW will just support this natively: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/269

Regardless of the window icon, setting the app_id is fine.

The rest of it, I do not want a separate window provider for this. It feels unnecessary and just complicates the code.

glfwWindowHint(GLFW_FOCUS_ON_SHOW, GLFW_FALSE);

Why is this actually necessary for Wayland support?

p.s.: Anything that is actually necessary for basic support of Wayland seems to just require a few lines in the current earlydisplay provider, and we can do that. Enabling wayland, for example, could simply be done via CLI args.

@shartte
Copy link
Contributor

shartte commented Jul 23, 2024

I will try to test this on WSL2 l. Wish me luck.

@@ -124,6 +127,7 @@ public Runnable initialize(String[] arguments) {
.withRequiredArg().ofType(Integer.class)
.defaultsTo(FMLConfig.getIntConfigValue(FMLConfig.ConfigValue.EARLY_WINDOW_HEIGHT));
var maximizedopt = parser.accepts("earlywindow.maximized");
var waylandopt = parser.accepts("earlywindow.wayland");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should just call this wayland since it enables Wayland not only for the early display window, but for the entirety of the Minecraft session, right?

@shartte
Copy link
Contributor

shartte commented Jul 25, 2024

I think this is way easier to merge now. I'll test it out as soon as I can.

@theendercore
Copy link

Soooo..
How's the testing going :3

@DerCommander323
Copy link

glfwWindowHint(GLFW_FOCUS_ON_SHOW, GLFW_FALSE);

Why is this actually necessary for Wayland support?

After some testing, it seems that currently KDE Plasma incorrectly reports the Window as "not responding" on initial focus. After alt tabbing away and back onto the window this goes away. I assume that's what this hint is for.

@wired-tomato
Copy link
Author

I had initially removed the hint after testing for that issue on my own system, I've added it back to see if it fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants