Implement floating #2027

RyanDwyer · 2018-05-24T12:50:21Z

This implements basic floating features. You can float views, unfloat views, make them sticky and unsticky, and click to focus/unfocus them. You cannot move or resize them.

I was going to make it so you can float C_CONTAINERs in this PR, but there were some issues and I wanted to get this in front of everyone for review, so I blocked off that functionality. There's still bits in the code where it handles C_CONTAINERs being floated though.

This PR has a known issue with floating xdg_shell views when using multiple outputs. If floating the view on the left output, I'm experiencing a flickering copy of the container at the same output-local position on the right output. This does not happen for xwayland views. I can't see what I'm doing wrong, so if someone can assist me with that then that would be appreciated.

I went with the approach of having a sway_workspace->floating property, but I made it a struct sway_container * rather than a list_t. Using a container makes it easier to pass to functions that expect to be given a container. This floating container is created at workspace creation, destroyed at workspace destruction, and is immune to reaping. The type of this container is C_CONTAINER and the layout is irrelevant and never used.

Each view implementation has a new wants_floating function, which allows the implementation to inspect the view and determine if it should be floating or not. There's a basic/terrible implementation for xwayland already done, but there needs to be some discussion on how to do this properly. Perhaps wlroots should expose its own list of window types so we don't have to use the xcb library. At the moment I'm using magic numbers, but this is a temporary solution.

Test plan:

Use as daily driver
Float and unfloat things
Make things sticky and unsticky
Use multiple outputs

My preference is to implement the move/resize commands and floating C_CONTAINERs as separate PRs.

RedSoxFan · 2018-05-24T16:46:20Z

sway/desktop/output.c

+			title_texture = con->title_unfocused;
+			marks_texture = view->marks_unfocused;
+		}
+		render_titlebar(soutput, damage, con, con->x, con->y, con->width,


There should be a check against the border style here. If you have a floating view with opacity set below 1.0 and border pixel or border none, the title bar is visible since it is being rendered underneath the view.

emersion · 2018-05-24T20:39:04Z

include/sway/tree/container.h

@@ -76,6 +75,14 @@ struct sway_container {
 	enum sway_container_layout layout;
 	enum sway_container_layout prev_layout;

+	// Allow the container to be automatically removed if it's empty. True by
+	// default, false for the magic floating container that each workspace has.
+	bool reapable;


In this case, why don't you keep the L_FLOATING for the floating container?

emersion · 2018-05-24T20:39:46Z

include/sway/tree/container.h

+	bool reapable;
+
+	// Saves us from searching the list of children/floating in the parent
+	bool is_floating;


is_floating is trivially implemented with a function

RedSoxFan · 2018-05-24T23:58:29Z

I found a couple more issues:

If you fullscreen enable a floating view and then fullscreen disable it, the position and geometry are not restored and the view remains to cover the entire output.
When you focus a sticky view, whatever workspace the view was made sticky on is switched to

emersion · 2018-05-25T20:03:14Z

include/sway/tree/view.h

@@ -32,7 +32,9 @@ struct sway_view_impl {
 	void (*configure)(struct sway_view *view, double ox, double oy, int width,
 		int height);
 	void (*set_activated)(struct sway_view *view, bool activated);
+	void (*set_maximized)(struct sway_view *view, bool maximized);


This should be renamed set_tiled (see #1988).

emersion · 2018-05-25T20:04:46Z

sway/debug-tree.c

@@ -22,8 +22,6 @@ static const char *layout_to_str(enum sway_container_layout layout) {
 		return "L_STACKED";
 	case L_TABBED:
 		return "L_TABBED";
-	case L_FLOATING:


Should be added back?

emersion · 2018-05-25T20:06:32Z

sway/desktop/output.c

+	// coordinates into output-local coordinates. This needs to happen for all
+	// children of the floating container too.
+	struct sway_container *output = container_parent(con, C_OUTPUT);
+	container_translate(con, -output->x, -output->y);


Hmm, I'm really not a fan of this. Find another way.

Yeah, I'm not a fan either. The options I can think of for this are:

Adjust the container's position temporarily as it currently does.

Have file-scoped translate_x and translate_y variables, which are 0 if the container being rendered is tiled, and the inverse of the output's position if floating. The rendering functions would add the translation to the thing being rendered, so it's basically forced into output-level scope.

Same as above, but using a wlr_matrix instead of the translate variables.

Use layout-local coordinates for all swayc positions, then subtract the output's position when adding damage and rendering anything.

Options 2 and 3 are pretty complicated. To damage or render any view, you have to figure out if the view is itself floating or a child of a floating container (this isn't supported yet but will be later). If it is, its coordinates are layout-local and need to be translated to output-local coordinates.

I think the best option is option 4 - use layout-local coordinates for all swaycs. This would also resolve the "these coords might be layout-local or output-local" issue that you pointed out in #2027 (review).

I went and implemented option 4. That particular commit was +71, -130. It has simplified other areas of the codebase and appears to have fixed my damage problems too.

emersion · 2018-05-25T20:07:40Z

sway/desktop/output.c

+	}
+}
+
+static bool floater_intersects_output(struct sway_container *floater,


This isn't enough, because popups/subsurfaces can be outside of this box and still need to get rendered.

emersion · 2018-05-25T20:08:01Z

sway/desktop/output.c

@@ -839,6 +923,8 @@ static void render_output(struct sway_output *output, struct timespec *when,
 			&root_container.sway_root->xwayland_unmanaged);
 		render_layer(output, damage,
 			&output->layers[ZWLR_LAYER_SHELL_V1_LAYER_TOP]);
+
+		render_floating(output, damage);


Floating is under the top layer and under xwayland unmanaged surfaces

RyanDwyer · 2018-05-26T06:48:01Z

sway/desktop/output.c

@@ -915,6 +1001,7 @@ static void send_frame_done(struct sway_output *output, struct timespec *when) {

 	struct sway_container *workspace = output_get_active_workspace(output);
 	send_frame_done_container(&data, workspace);
+	send_frame_done_container(&data, workspace->sway_workspace->floating);


I don't quite understand this comment. Is this still relevant after my most recent commits, and if so can you please clarify what you mean?

emersion · 2018-05-25T20:11:15Z

sway/desktop/output.c

@@ -1036,7 +1123,15 @@ static void output_damage_view(struct sway_output *output,

 void output_damage_from_view(struct sway_output *output,
 		struct sway_view *view) {
-	output_damage_view(output, view, false);
+	if (container_self_or_parent_floating(view->swayc)) {
+		view->x -= output->swayc->x;


Meh, I'm sure there's a nicer way to do this

emersion · 2018-05-25T20:12:50Z

sway/desktop/xdg_shell.c

@@ -117,6 +126,11 @@ static void set_fullscreen(struct sway_view *view, bool fullscreen) {
 	wlr_xdg_toplevel_set_fullscreen(surface, fullscreen);
 }

+static bool wants_floating(struct sway_view *view) {
+	// TODO


This can return true if the view isn't resizable, ie. if the min size is equal to the max size.

Can you please clarify this? The wants_floating function is called when a view is mapped so that the implementation can decide whether it's floating by default (eg. for dialogs).

Yes. I'm just suggesting a way to implement this for xdg-shell.

emersion · 2018-05-25T20:14:17Z

sway/desktop/xdg_shell.c

-	// TODO: Let floating views do whatever
-	view_update_size(view, xdg_shell_view->pending_width,
-		xdg_shell_view->pending_height);
+	struct wlr_box *geometry = &view->wlr_xdg_surface->geometry;


The xdg-surface geometry can be unset, in which case the surface's size should be used.

Are you sure about that? wlr_xdg_shell contains these:

bool has_next_geometry; struct wlr_box next_geometry; struct wlr_box geometry;

...which suggests to me that the geometry struct is always present.

The client doesn't always set the geometry, so this box can be empty. We should probably document this, it's written in the protocol itself.

emersion · 2018-05-25T20:15:25Z

sway/desktop/xdg_shell.c

@@ -97,6 +97,7 @@ static void configure(struct sway_view *view, double ox, double oy, int width,
 	xdg_shell_view->pending_width = width;
 	xdg_shell_view->pending_height = height;
 	wlr_xdg_toplevel_set_size(view->wlr_xdg_surface, width, height);
+	view_update_position(view, ox, oy);


This is documented to take layout coords

emersion · 2018-05-25T20:15:52Z

sway/desktop/xdg_shell_v6.c

@@ -97,6 +97,7 @@ static void configure(struct sway_view *view, double ox, double oy, int width,
 	xdg_shell_v6_view->pending_width = width;
 	xdg_shell_v6_view->pending_height = height;
 	wlr_xdg_toplevel_v6_set_size(view->wlr_xdg_surface_v6, width, height);
+	view_update_position(view, ox, oy);


All my comments on xdg-shell also apply here

emersion · 2018-05-25T20:17:07Z

sway/desktop/xwayland.c

@@ -152,33 +152,43 @@ static uint32_t get_int_prop(struct sway_view *view, enum sway_view_prop prop) {
 	}
 }

-static void configure(struct sway_view *view, double ox, double oy, int width,
+// The x and y arguments are output-local for tiled views, and layout
+// coordinates for floating views.


emersion · 2018-05-25T20:17:41Z

sway/desktop/xwayland.c

+		}
+		struct wlr_output_layout *layout = root->sway_root->output_layout;
+		struct wlr_output_layout_output *loutput =
+			wlr_output_layout_get(layout, output->sway_output->wlr_output);


No need to do all of this. Just take the struct sway_container *output's coordinates.

emersion · 2018-05-25T20:18:58Z

sway/desktop/xwayland.c

+		case 44: // NET_WM_WINDOW_TYPE_SPLASH
+		case 276: // ? PGP passphrase dialog
+		case 337: // ? Firefox open file dialog
+		case 338: // ? Firefox open file dialog


Let's implement this instead of having these magic values :)

wlroots needs to expose its atom names first. And I'm a bit confused with how this all works - wlroots lacks atom names DIALOG and TOOLBAR which are used by i3. And according to wlr_xwayland_surface_is_unmanaged, surfaces with atoms SPLASH and UTILITY will be unmanaged, but to match i3 these would need to be managed and floated.

BTW, according to Ongy the atom values are defined at runtime, so I've removed the hard coded values from my PR. Keeping them there may cause false positives.

I can do the wlroots PR tonight if that helps

RyanDwyer · 2018-05-26T14:10:06Z

sway/desktop/xwayland.c

+		view->natural_height = xsurface->height;
+	}
+	if (view->swayc && container_is_floating(view->swayc)) {
+		view_update_size(view, xsurface->width, xsurface->height);


This is resolved in commit "Don't let xwayland views set position unless unmanaged".

emersion · 2018-05-25T20:24:23Z

sway/tree/container.c

+					.width = floater->width,
+					.height = floater->height,
+				};
+				if (wlr_box_contains_point(&box, lx, ly)) {


Won't work with subsurfaces/popups outside the box

I floated Firefox, then right clicked near the right side so the context menu popped up outside of the float. I can interact with the popup outside the floating area just fine. That's because layer surfaces are checked before floating_container_at and container_at are called.

This is completely unrelated to layer surfaces. Firefox is xwayland, which uses unmanaged surfaces that are global to the whole layout and don't have a parent. I'm talking about xdg-popups and wayland subsurfaces.

I can replicate this with FileZilla. However, I can also replicate it when tiled, and I can replicate in master, and I can replicate it in master from a week ago which was prior to when I split container_at into separate functions. So this problem has been around for a while.

I guess the solution will be to iterate the containers, check their subsurfaces first, then iterate them all again and check the toplevel surface. The same has to happen for rendering too - currently a tiled view can render over the top of its left sibling's context menu.

My preference is to make a separate PR for this because it's not specific to floating.

Filezilla is not using xdg-shell. Pick any GTK+3 app, right click, and you should get a popup.

Hmm, can you just remove this wlr_box_contains_point check and only use container_at?

emersion · 2018-05-25T20:27:31Z

sway/tree/container.c

+	container_damage_whole(container);
+}
+
+void container_set_geometry_from_view(struct sway_container *container) {


The name should make it more obvious that it expects a floating view - maybe container_set_geometry_from_floating_view?

emersion · 2018-05-25T20:29:26Z

sway/tree/container.c

+	container->height = top + view->height + border_width;
+}
+
+bool container_self_or_parent_floating(struct sway_container *container) {


This name is weird, and doesn't match what the function really does?

With the change of swayc coordinates from output-local to layout-local, this function was no longer needed so I've deleted it.

emersion · 2018-05-25T20:32:50Z

sway/tree/view.c

-void view_update_position(struct sway_view *view, double ox, double oy) {
-	if (view->swayc->x == ox && view->swayc->y == oy) {
+void view_update_position(struct sway_view *view, double lx, double ly) {
+	if (!container_is_floating(view->swayc)) {


Should still do nothing if the position hasn't changed

emersion · 2018-05-26T07:00:27Z

sway/desktop/output.c

@@ -450,8 +454,10 @@ static void render_titlebar(struct sway_output *output,
 		wlr_texture_get_size(marks_texture,
 			&texture_box.width, &texture_box.height);
 		texture_box.x =
-			(x + width - TITLEBAR_H_PADDING) * output_scale - texture_box.width;
-		texture_box.y = (y + TITLEBAR_V_PADDING) * output_scale;
+			(x - output->wlr_output->lx + width - TITLEBAR_H_PADDING)


Don't use output->wlr_output->lx, use output->swayc->x instead

sam-lord · 2018-06-01T14:51:56Z

Tried using this, top borders not rendered on windows which start as floating (gnome-terminal preferences window and gnome-keyring when opening Geary).

ddevault · 2018-06-01T22:41:43Z

I don't want to hold this up forever so I'm going to file a separate ticket for the border issue.

RyanDwyer requested review from emersion and ddevault May 24, 2018 12:50

RyanDwyer force-pushed the implement-floating branch from ea644f7 to 73a373a Compare May 24, 2018 13:20

RedSoxFan reviewed May 24, 2018

View reviewed changes

emersion reviewed May 24, 2018

View reviewed changes

emersion reviewed May 25, 2018

View reviewed changes

RyanDwyer force-pushed the implement-floating branch from c3212a5 to fe9dd3e Compare May 26, 2018 06:42

emersion reviewed May 26, 2018

View reviewed changes

emersion mentioned this pull request May 26, 2018

[WIP] Floating #1911

Closed

13 tasks

RyanDwyer force-pushed the implement-floating branch from fe9dd3e to f9d3ef0 Compare May 26, 2018 13:46

RyanDwyer and others added 25 commits June 1, 2018 23:14

Prevent splitting a floating view

5d69a56

Rename set_maximized functions to set_tiled

02de2a6

Add L_FLOATING back to debug tree

00f6e17

Store swayc coordinates as layout-local

e4e912e

Render floating views before top layer and unmanaged

7d2b33a

Remove check for if floating view intersects output

3281574

Rename container_set_geometry_from_view

70f5d6f

Remove container_self_or_parent_floating

d4ed204

Implement wants_floating for xdg_shell and xdg_shell_v6

4371c74

Use surface size if xdg shell's geometry isn't set

3b1db30

Use swayc rather than wlr_output when rendering

02d385e

Don't let xwayland views set position unless unmanaged

5b1601c

Remove unfinished wants_floating implementation for xwayland

becceaf

Fix getting adjacent output

40af5d8

Don't unmaximize floating views

9767229

Fix fullscreen position

f24087d

Adjust move command to account for changed coordinate system

f7cadf2

Fix floating position when view is floated when mapped

9119f87

Consider floating views when calculating title height

c9f8d35

Fix mouse warping interaction with layout coords

e605dc4

Fix crash when using multiple outputs

00cac22

Fix fullscreen position of xwayland views

c9e3a31

Fix mpv damage issue when unfullscreening into floating

a2c1cb9

Don't auto float xdg views if their dimensions are not set

d466b8f

Fix changing borders on floating views

70c2c50

RyanDwyer force-pushed the implement-floating branch from b05f47b to 70c2c50 Compare June 1, 2018 13:52

ddevault merged commit 96446fd into swaywm:master Jun 1, 2018

RyanDwyer deleted the implement-floating branch June 2, 2018 00:26

Implement floating #2027

Implement floating #2027

Conversation

RyanDwyer commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedSoxFan commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanDwyer May 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion May 25, 2018 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-lord commented Jun 1, 2018

ddevault commented Jun 1, 2018

RyanDwyer May 26, 2018 •

edited

emersion May 25, 2018 •

edited