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

fix(window): improve wayland-display #979

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

Conversation

Zamoca42
Copy link

Fixes #977

Reproduce Errror Protocol 71

In the resizable example, when resizable is set to false and with_maximized(true) is used, it works fine on macOS, but an “Error Protocol 71” occurs on Linux using Wayland.
This behavior seems a bit unusual.

MacOS

reproduce-in-mac

Wayland on Linux

reproduce-in-wayland

Changes

  1. Confirmed that the resize state is true when maximizing.
  2. Added buttons for maximize and minimize in the Wayland title bar.
  3. Modified the event loop for resizing in Wayland:
    • Fixed an issue where the window was not resizing when dragging the window borders.
    • Noted that, except for window.window(), Wayland may not have the concept of GdkWindow like X11.
    • Managed the resizable state internally and removed the conditions.

1. Added conditions for maximizing in the Linux environment.

To prevent “Error Protocol 71,” I added a condition to check the resizable state when maximizing in the Linux environment.

maximize

2. Wayland Header

While X11 has built-in headers for maximize and minimize buttons, Wayland does not have these buttons by default. Therefore, I added headers specifically for Wayland.

before

before

after

after

3. resizing in Wayland

Unlike X11, in Wayland, even when the resizable condition is set to true, resizing was not possible, and moving the window was also not allowed. However, by modifying the conditions in the event loop, window movement is now enabled.

before

before-border
before-move

after

after-move
after-border

Issues still not resolved

In Linux, when creating a window, it should maximize on the screen with with_maximized(true), just like on macOS, even if resizable is set to false.

The changes add a custom header bar with window control buttons (close, maximize/restore, minimize) to the Linux Wayland platform implementation. This provides a consistent user experience for window management across platforms.

The header bar is implemented using GTK widgets and is added to the top of the application window. The drag area of the header bar is also used to enable window moving and resizing functionality.
## Explanation
The changes made in this commit ensure that the window is only maximized if it is also resizable. This is to prevent issues where a non-resizable window is maximized, which could lead to unexpected behavior.

The main changes are:

1. In the `set_maximized` function, the code checks if the window is resizable before maximizing it. If it's not resizable, the window is not maximized.
2. In the `is_maximizable` function, the code checks if the window is resizable and if the maximized state is true before returning `true`.

These changes improve the overall behavior and consistency of the window maximization functionality.
@Zamoca42 Zamoca42 requested a review from a team as a code owner September 23, 2024 11:25
Copy link
Contributor

Package Changes Through 342b42d

No changes.

Add a change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

@Zamoca42 thank you, this looks awesome, left a few comments

@@ -421,6 +422,33 @@ impl<T: 'static> EventLoop<T> {
| EventMask::SCROLL_MASK,
);

const TITLEBAR_HEIGHT: f64 = 30.0;
Copy link
Member

Choose a reason for hiding this comment

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

this feels like a magic number, shouldn't we query and check the actual height of the header bar?

Comment on lines -465 to -467
if !window.is_decorated()
&& window.is_resizable()
&& !window.is_maximized()
Copy link
Member

Choose a reason for hiding this comment

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

why was these checks removed?

@@ -87,6 +89,10 @@ impl Window {

let window = window_builder.build();

if is_wayland {
WlHeader::setup(&window);
Copy link
Member

Choose a reason for hiding this comment

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

how does this look like on Desktop environments that has server side decorations. I think KDE has server side decorations.

@@ -569,9 +575,12 @@ impl Window {
}

pub fn set_maximized(&self, maximized: bool) {
let resizable = self.is_resizable();
Copy link
Member

Choose a reason for hiding this comment

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

instead of just ignoring the maximized request, should we instead make the window resizable, maximize it, then make it non-resizable again?

@@ -594,7 +603,7 @@ impl Window {
}

pub fn is_maximizable(&self) -> bool {
true
self.window.is_resizable() && self.maximized.load(Ordering::Acquire)
Copy link
Member

Choose a reason for hiding this comment

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

is_maximizable is not the same as is_maximized

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.

[bug][linux][WSL] Wayland protocol error 71 when restoring maximized window state
2 participants