Add support for xwayland_minimize actions #2264

damianatorrpm · 2020-06-07T18:11:57Z

as required by chrome/steam csd or games:
Fixes #2251
In the PR there is also a fix for a forgotten break statement
Minimize functionality tested with wayfire

emersion · 2020-06-07T19:08:50Z

xwayland/xwm.c

+
+		if (xsurface == NULL) {
+			return;
+		} else if (detail == ICCCM_ICONIC_STATE) {


What is the relationship between ICCCM_ICONIC_STATE and NET_WM_STATE_HIDDEN?

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

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

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

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

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.

@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.

This has been changed, see the updated PR.

This has been resolved, see updated PR

xwayland/xwm.c

emersion · 2020-06-07T19:10:36Z

Thanks for your PR!

Please make sure tabs are used for indentation. Some lines use spaces in this patch.

damianatorrpm · 2020-06-07T19:20:50Z

I think all is resolved

xwayland/xwm.c

damianatorrpm · 2020-06-08T10:40:14Z

Strange how do I remove the whitespace than, I edit using gedit and it's set to tabs and show the "trailing whitespace" as tabs

fluix-dev · 2020-06-08T13:19:13Z

It is a tab, it's just extra after the end of the line. For example, you have code:

...minimized != minimized;

After the ; is a (Tab - Github doesn't seem to like rendering this) so probably just move your cursor after it and hit backspace.

emersion · 2020-06-08T13:25:02Z

Yup. To be more explicit:

const bool change = surface->minimized != minimized;	← this is a tab

damianatorrpm · 2020-06-08T16:34:29Z

@TheAvidDev @emersion ah, now I see it, of course trailing like in a trailer...
thanks

damianatorrpm · 2020-06-09T08:25:55Z

i3/i3@136b3e3
see also i3 handling

xwayland/xwm.c

emersion · 2020-06-12T14:10:53Z

xwayland/xwm.c

@@ -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;


We should only do this if the surface is minimized.

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) {

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.

Can you look into this?

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.

emersion · 2020-06-12T14:11:34Z

xwayland/xwm.c

@@ -1215,6 +1233,19 @@ 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);


Can we extract all of this into a xwm_handle_wm_change_state_message function?

Can you look into this?

xwayland/xwm.c

emersion

See comments

damianatorrpm · 2020-06-15T08:21:42Z

Bump.

emersion · 2020-06-15T08:23:13Z

Please bump every week. We don't have enough time to reply to 2-day bumps for patches that aren't critical bugfixes.

damianatorrpm · 2020-06-15T08:27:31Z

It's a bug fix see description.

emersion · 2020-06-15T08:28:23Z

No, it's a new feature.

damianatorrpm · 2020-06-15T08:33:58Z

Cool stuff. Once you replied to the in-conversations let me know.

xwayland/xwm.c

ddevault · 2020-07-04T13:34:31Z

@damianatorrpm you need to be more polite to the people kind enough to review your patch. There are multiple open problems in the discussion that you need to address; bumping the thread before then is not helpful.

Also, general review comment: when you have everything squared away, please rebase these commits into a single commit.

as required by chrome/steam csd or games PR style fixes Trailinig whitespaces and a space 2 more excess tabs suggeste fixes rev suggestions typo type refact typo

damianatorrpm · 2020-07-08T14:19:04Z

@damianatorrpm you need to be more polite to the people kind enough to review your patch. There are multiple open problems in the discussion that you need to address; bumping the thread before then is not helpful.

Also, general review comment: when you have everything squared away, please rebase these commits into a single commit.

@ddevault
These bumps included the request to answer a follow up question which had not been answered,
I was asked via IRC by to bump every week.

The commits are in a single commit.

When contributors send pull request, reviewers should have the tactfulness to read related and linked documentation, to have an understanding technically what it is about, instead of arguing on documented properties, which shows a 'less than interested' attitude. I suggest and hope we move forward any misunderstanding.

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

All things mentioned have been addressed to the best of my knowledge and the discussions 'resolved'.

ddevault · 2020-07-08T14:50:26Z

You have a bad attitude, and it's not worth further damaging our reviewer's mental health to deal with you. It is not us that lack tact in this exchange.

ddevault · 2020-07-08T14:51:30Z

If someone else wants to adopt this work and bring it to us with a better attitude, we'd be interested in reviewing it again later.

emersion reviewed Jun 7, 2020

View reviewed changes