Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Add support for xwayland_minimize actions #2264

Closed
wants to merge 1 commit into from
Closed

Add support for xwayland_minimize actions #2264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions include/wlr/xwayland.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ struct wlr_xwayland_surface {
bool modal;
bool fullscreen;
bool maximized_vert, maximized_horz;
bool minimized;

bool has_alpha;

Expand All @@ -181,6 +182,7 @@ struct wlr_xwayland_surface {
struct wl_signal request_configure;
struct wl_signal request_move;
struct wl_signal request_resize;
struct wl_signal request_minimize;
struct wl_signal request_maximize;
struct wl_signal request_fullscreen;
struct wl_signal request_activate;
Expand Down Expand Up @@ -250,6 +252,9 @@ void wlr_xwayland_surface_configure(struct wlr_xwayland_surface *surface,

void wlr_xwayland_surface_close(struct wlr_xwayland_surface *surface);

void wlr_xwayland_surface_set_minimized(struct wlr_xwayland_surface *surface,
bool minimized);

void wlr_xwayland_surface_set_maximized(struct wlr_xwayland_surface *surface,
bool maximized);

Expand Down
2 changes: 2 additions & 0 deletions include/xwayland/xwm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ enum atom_name {
NET_WM_STATE_FULLSCREEN,
NET_WM_STATE_MAXIMIZED_VERT,
NET_WM_STATE_MAXIMIZED_HORZ,
NET_WM_STATE_HIDDEN,
NET_WM_PING,
WM_CHANGE_STATE,
WM_STATE,
CLIPBOARD,
PRIMARY,
Expand Down
55 changes: 54 additions & 1 deletion xwayland/xwm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ const char *atom_map[ATOM_LAST] = {
[NET_WM_STATE_FULLSCREEN] = "_NET_WM_STATE_FULLSCREEN",
[NET_WM_STATE_MAXIMIZED_VERT] = "_NET_WM_STATE_MAXIMIZED_VERT",
[NET_WM_STATE_MAXIMIZED_HORZ] = "_NET_WM_STATE_MAXIMIZED_HORZ",
[NET_WM_STATE_HIDDEN] = "_NET_WM_STATE_HIDDEN",
[NET_WM_PING] = "_NET_WM_PING",
[WM_CHANGE_STATE] = "WM_CHANGE_STATE",
[WM_STATE] = "WM_STATE",
[CLIPBOARD] = "CLIPBOARD",
[PRIMARY] = "PRIMARY",
Expand Down Expand Up @@ -146,6 +148,7 @@ static struct wlr_xwayland_surface *xwayland_surface_create(
wl_signal_init(&surface->events.request_configure);
wl_signal_init(&surface->events.request_move);
wl_signal_init(&surface->events.request_resize);
wl_signal_init(&surface->events.request_minimize);
wl_signal_init(&surface->events.request_maximize);
wl_signal_init(&surface->events.request_fullscreen);
wl_signal_init(&surface->events.request_activate);
Expand Down Expand Up @@ -290,7 +293,7 @@ static void xwm_surface_activate(struct wlr_xwm *xwm,

static void xsurface_set_net_wm_state(struct wlr_xwayland_surface *xsurface) {
struct wlr_xwm *xwm = xsurface->xwm;
uint32_t property[4];
uint32_t property[5];
int i;

i = 0;
Expand All @@ -306,6 +309,9 @@ static void xsurface_set_net_wm_state(struct wlr_xwayland_surface *xsurface) {
if (xsurface->maximized_horz) {
property[i++] = xwm->atoms[NET_WM_STATE_MAXIMIZED_HORZ];
}
if (xsurface->minimized) {
property[i++] = xwm->atoms[NET_WM_STATE_HIDDEN];
emersion marked this conversation as resolved.
Show resolved Hide resolved
}

xcb_change_property(xwm->xcb_conn,
XCB_PROP_MODE_REPLACE,
Expand Down Expand Up @@ -664,6 +670,8 @@ static void read_surface_net_wm_state(struct wlr_xwm *xwm,
xsurface->maximized_vert = true;
} else if (atom[i] == xwm->atoms[NET_WM_STATE_MAXIMIZED_HORZ]) {
xsurface->maximized_horz = true;
} else if (atom[i] == xwm->atoms[NET_WM_STATE_HIDDEN]) {
xsurface->minimized = true;
}
}
}
Expand Down Expand Up @@ -1125,6 +1133,7 @@ static void xwm_handle_net_wm_state_message(struct wlr_xwm *xwm,

bool fullscreen = xsurface->fullscreen;
bool maximized = xsurface_is_maximized(xsurface);
bool minimized = xsurface->minimized;

uint32_t action = client_message->data.data32[0];
for (size_t i = 0; i < 2; ++i) {
Expand All @@ -1142,6 +1151,9 @@ static void xwm_handle_net_wm_state_message(struct wlr_xwm *xwm,
} else if (property == xwm->atoms[NET_WM_STATE_MAXIMIZED_HORZ] &&
update_state(action, &xsurface->maximized_horz)) {
xsurface_set_net_wm_state(xsurface);
} else if (property == xwm->atoms[NET_WM_STATE_HIDDEN] &&
update_state(action, &xsurface->minimized)) {
xsurface_set_net_wm_state(xsurface);
}
}
// client_message->data.data32[3] is the source indication
Expand All @@ -1164,6 +1176,12 @@ static void xwm_handle_net_wm_state_message(struct wlr_xwm *xwm,

wlr_signal_emit_safe(&xsurface->events.request_maximize, xsurface);
}

if (minimized != xsurface->minimized) {
xsurface->saved_width = xsurface->width;
Copy link
Member

Choose a reason for hiding this comment

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

We should only do this if the surface is minimized.

Copy link
Author

@damianatorrpm damianatorrpm Jun 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please be more specific in your comments.
Do what emit signals? Safe size?
The handling is exactly the same as for maximized in this function.

I suppose the below is what you think is right so I changed it to that:
if (minimized && minimized != xsurface->minimized) {

Copy link
Member

Choose a reason for hiding this comment

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

No, we want to emit the signal even if the client requests to not be minimized.

Please have a look at the similar block of code above for maximized. We only set saved_width and saved_height if the surface becomes maximized. We should do the same here, only update the saved size if the surface wants to become minimized.

Copy link
Member

Choose a reason for hiding this comment

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

Can you look into this?

Copy link
Author

@damianatorrpm damianatorrpm Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see how this is wrong, maybe you suggest a proper way?
Github has the ability to suggest changes.

It is handled the way fullscreen is.

xsurface->saved_height = xsurface->height;
wlr_signal_emit_safe(&xsurface->events.request_minimize, xsurface);
}
}

static void xwm_handle_wm_protocols_message(struct wlr_xwm *xwm,
Expand Down Expand Up @@ -1215,6 +1233,25 @@ static void xwm_handle_client_message(struct wlr_xwm *xwm,
xwm_handle_wm_protocols_message(xwm, ev);
} else if (ev->type == xwm->atoms[NET_ACTIVE_WINDOW]) {
xwm_handle_net_active_window_message(xwm, ev);
} else if (ev->type == xwm->atoms[WM_CHANGE_STATE]) {
struct wlr_xwayland_surface *xsurface = lookup_surface(xwm, ev->window);
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract all of this into a xwm_handle_wm_change_state_message function?

Copy link
Member

Choose a reason for hiding this comment

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

Can you look into this?

uint32_t detail = ev->data.data32[0];

if (xsurface == NULL) {
return;
} else if (detail == ICCCM_ICONIC_STATE) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the relationship between ICCCM_ICONIC_STATE and NET_WM_STATE_HIDDEN?

Copy link
Author

Choose a reason for hiding this comment

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

if a window is minimized it's both the NET_WM_STATE is hidden the WM_STATE is iconic

Copy link
Member

Choose a reason for hiding this comment

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

Hm, so why do we need to handle ICCCM_ICONIC_STATE changes?

Copy link
Member

Choose a reason for hiding this comment

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

Bump

Copy link
Author

@damianatorrpm damianatorrpm Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is the way minimize is being set - through WM_STATE which is iconic state - i3 does the same, net_wm_state_hidden mean window is not visible, iconic state means minimized, see here: https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what your above statement means code-wise,
or does it just mean 'resolved'.

Copy link
Member

Choose a reason for hiding this comment

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

There are two code-paths in this PR:

  • Client wants to become minimized. It sends NET_WM_STATE_HIDDEN, and if I understand correctly, it can alternatively send ICCCM_ICONIC_STATE. In this case, wlroots should (1) not minimize the window and (2) emit a request_minimize event. (1) is important because we need to let the compositor handle the request. Maybe the compositor can't minimize windows. Maybe the compositor wants to do something else when a client requests to be minimized. The compositor must stay in control. wlroots shouldn't do anything behind the compositor's back.
  • A compositor decides to minimize a window. This can be in response to a request_minimize event, or this can be because of something else (e.g. the user clicked "minimize" in a compositor menubar). The compositor will call wlr_xwayland_surface_set_minimized.

Copy link
Contributor

Choose a reason for hiding this comment

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

@damianatorrpm this discussion needs to be resolved - this is the main blocker for this patch.

wlroots isn't a window manager in its own right, its a toolkit for window managers. We just help information flow smoothly from client to compositor implementation, and the compositor implementation calls the shots. So, instead of having wlroots' Xwayland layer handle the minimize request, we need to add a new event which the compositor can register for, which is emitted when the window requests to be minimized. Then, the compositor can decide what to do - for example, calling wlr_xwayland_surface_set_minimized themselves.

Copy link
Author

Choose a reason for hiding this comment

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

This has been changed, see the updated PR.

Copy link
Author

Choose a reason for hiding this comment

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

This has been resolved, see updated PR

xsurface_set_wm_state(xsurface, ICCCM_ICONIC_STATE);
xsurface_set_net_wm_state(xsurface);
xsurface->minimized = true;
wlr_signal_emit_safe(&xsurface->events.request_minimize, xsurface);
} else if (detail == ICCCM_NORMAL_STATE) {
xsurface_set_wm_state(xsurface, ICCCM_NORMAL_STATE);
xsurface_set_net_wm_state(xsurface);
xsurface->minimized = false;
wlr_signal_emit_safe(&xsurface->events.request_minimize, xsurface);
} else {
wlr_log(WLR_DEBUG, "unhandled wm_change_state event %u", detail);
}
} else if (!xwm_handle_selection_client_message(xwm, ev)) {
char *type_name = xwm_get_atom_name(xwm, ev->type);
wlr_log(WLR_DEBUG, "unhandled x11 client message %u (%s)", ev->type,
Expand Down Expand Up @@ -1761,6 +1798,7 @@ struct wlr_xwm *xwm_create(struct wlr_xwayland *xwayland, int wm_fd) {
xwm->atoms[NET_WM_STATE_FULLSCREEN],
xwm->atoms[NET_WM_STATE_MAXIMIZED_VERT],
xwm->atoms[NET_WM_STATE_MAXIMIZED_HORZ],
xwm->atoms[NET_WM_STATE_HIDDEN],
xwm->atoms[NET_CLIENT_LIST],
};
xcb_change_property(xwm->xcb_conn,
Expand Down Expand Up @@ -1792,6 +1830,20 @@ struct wlr_xwm *xwm_create(struct wlr_xwayland *xwayland, int wm_fd) {
return xwm;
}

void wlr_xwayland_surface_set_minimized(struct wlr_xwayland_surface *surface,
bool minimized) {
surface->minimized = minimized;

if (minimized) {
xsurface_set_wm_state(surface, ICCCM_ICONIC_STATE);
} else {
xsurface_set_wm_state(surface, ICCCM_NORMAL_STATE);
}

xsurface_set_net_wm_state(surface);
xcb_flush(surface->xwm->xcb_conn);
}

void wlr_xwayland_surface_set_maximized(struct wlr_xwayland_surface *surface,
bool maximized) {
surface->maximized_horz = maximized;
Expand Down Expand Up @@ -1851,6 +1903,7 @@ bool wlr_xwayland_or_surface_wants_focus(
if (xwm_atoms_contains(surface->xwm, surface->window_type,
surface->window_type_len, needles[i])) {
ret = false;
break;
}
}

Expand Down