Add relative outputs to wlr_output_layout #1002

VincentVanlaer · 2018-05-26T11:44:40Z

Introduces a way for compositors to create layouts with outputs that are relative to other outputs.
These relative outputs will update when the position or size of the reference ouput changes.
A rootston implementation is provided which

allows relative outputs to be configured,
makes sure that gaps do not occur when not all configured outputs are connected,
makes sure that overlaps do not occur when outputs without configuration are connected.

See #550.

API changes:

~~removed wlr_output_layout_move~~
~~wlr_output_layout_add_auto: changed behavior, ouputs added through this function do not adjust dynamically anymore.~~

cc @Ongy @Timidger

Ongy · 2018-05-26T12:20:32Z

wlr_output_layout_add_auto: changed behavior, ouputs added through this function do not adjust dynamically anymore.

I'm really not a fan of this.
This may lead to unconnected outputs at some point, and with the current cursor logic, there's no way to move it from one such output to the other.

BUT I started to implement something in this general fashion for waymonad before, and dropped it because I couldn't find a statisfactory solution. There's a lot of things that can go wrong, and some tradeoffs have to be made in the end.

I'd prefer if the _auto added outputs didn't get a fixed x/y like this, IMO it's better to default their internal relative location, so they are right of the output that currently provides the right most edge.
This also requires a bit of defaulting/collision detection if it's supposed to work at all times, but won't split the desktop.

VincentVanlaer · 2018-05-26T12:49:07Z

@Ongy The idea was that using add_auto is fine for examples and just getting things on screen, but that it isn't really suited for compositors in general. See the discussion in #550 for the discussion on this. With that in mind I opted for the implementation that would require the least work. I prefer to see wlr_output_layout as something that helps the compositor with the calculations and integrates well with the rest of the library. Any "policies" surrounding what to do when outputs disappear, when gaps occur, ... should in my opinion live in the compositor.

Ongy · 2018-05-26T13:44:34Z

The latest response you got there is more on my line than yours here.
At the very least there should be code to warp the cursor if it reaches the edge (rootston/cursor.c I guess) if you want to follow acrisci's advice there.

But the _auto was good as it was for me, add and forget.
While I generally like the idea of having relative output layouts, but your changes make the wlr_output_layout automation useless for my needs.
It's not add-and-forget for _auto. I still need to manage my outputs by hand (as library consumer, not necessarily enduser). This is a regression which (IMO) outweighs the current benefit.

As is, I'm against these changes.

VincentVanlaer · 2018-05-26T14:28:48Z

I'm not necessarily against having add_auto attach to right of the rightmost output. The only reason I implemented it in this way was because I saw not much use for it in a full-fledged compositor, due to its shortcomings (order dependent hard to deal with if one wants to attach outputs to those auto outputs). But I can understand that it can have its use, and I will try to create a decent implementation for this.

Regarding deadzones, while I do think it is useful to deal with them, I don't think cursor warping is something that should be in this PR, especially with the behavior of the previous paragraph.

Ongy · 2018-05-26T15:02:05Z

I always considerd the _auto limited. And I do agree, that it won't be the end-all be-all for most compositors.
But I think that the add-and-forget property of it is its strong suite.

And yes, if they layout prevents detached outputs, the warping over deadzones won't be necessary at all.

VincentVanlaer · 2018-05-28T13:42:59Z

Reverted add_auto behavior.
While improving on the rootston behavior in order to prevent gaps, I found that there were a few things missing in wlr_output_layoutin order to make it reasonably easy and efficient to extend.

All functions for interfacing with the wlr_output_layout use wlr_output and do a lookup in the list of wlr_output_layout_output.
The implementation in rootston in this PR requires an explicit ordering of wlr_output_layout_output. Make this part of the API.
User data in wlr_output_layout_output, like in wlr_output_layout.
Instead of re-configuring the layout every time an output is added/removed/changed, which can be inefficient (see roots_layout_reflow), we could require the user to explicitly reconfigure the layout., throug something like wlr_output_layout_reconfigure. This also makes sure that intermediate states do not get transmitted to xdg-outputclients.
If the layout fires a signal when wlr_output_layout_reconfigure is called, it makes custom reflows after mode changes or transform easier to deal with. In the case of rootston, reflowing after modeset currently only works because the handler in wlr_output_layout_output gets called before the handler in roots_output.

Unless there's heavy opposition to this, I will make this into a separate issue.

acrisci · 2018-05-28T15:21:27Z

I'd like to keep the auto layout function as it is as I said in that issue because it's really useful for creating default layouts.

VincentVanlaer · 2018-05-28T15:24:46Z

It should behave the same in the last version of this PR

emersion · 2018-05-28T16:16:27Z

types/wlr_output_layout.c

@@ -190,6 +229,45 @@ static struct wlr_output_layout_output *output_layout_output_create(
 	return l_output;
 }

+// Adapted from wl_list_for_each
+// Starts with wlr_ to avoid name conflicts
+#define wlr_wl_list_for_each_offset(pos, head, member, offset)			\


I'd rather write a little less efficient but more readable code

Rather, I'm not quite sure why you define this here. It's only used once until the #undef, so it can just be inline.

That's a remnant from when I used it in two functions

acrisci · 2018-05-28T18:56:46Z

types/wlr_output_layout.c

@@ -270,20 +348,6 @@ struct wlr_output *wlr_output_layout_output_at(struct wlr_output_layout *layout,
 	return NULL;
 }

-void wlr_output_layout_move(struct wlr_output_layout *layout,
-		struct wlr_output *output, int lx, int ly) {


don't remove this.

wlr_output_layout_add can do the same thing (and does so in master), so what would be the reason for keeping this around?

It might make sense. Make an issue for it. It's an unrelated breaking public API change so it needs some discussion and visibility.

I guess this might also depend on what is decided in #1013.

acrisci · 2018-05-28T19:42:36Z

User data in wlr_output_layout_output, like in wlr_output_layout.

I'd rather just make the wlr_output_layout_output private. #1013

emersion · 2018-05-31T08:23:36Z

include/wlr/types/wlr_output_layout.h

+
+/**
+ * Add an auto configured output to the layout relative to another output.
+ * This will place the relative output next to the border of the absolute


s/absolute/reference/

emersion · 2018-05-31T08:24:38Z

include/wlr/types/wlr_output_layout.h

+ * This will place the relative output next to the border of the absolute
+ * output. The output layout will adjust dynamically to keep the relative
+ * output in this position when the absolute output changes coordinates.
+ * If either output is not in the output layout, it will be added. If the


it will be added

Hmm?

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

rootston/config.c

+			free(lrc);
+		}
+		free(lc->name);
+		free(kc);


emersion · 2018-05-31T08:34:08Z

rootston/desktop.c

-	desktop->layout = wlr_output_layout_create();
-	wlr_xdg_output_manager_create(server->wl_display, desktop->layout);
+	struct roots_layout_config *default_layout;
+


Could add a little assert(!wl_list_empty(&config->layouts)) here

emersion · 2018-05-31T08:36:48Z

rootston/keyboard.c

+		struct roots_layout_config *config =
+			wl_container_of(next, desktop->layout->current_config, link);
+
+		wlr_log(L_DEBUG, "Switching to layout %s", config->name);


I'm not a fan of different output layouts for rootston.

It's more of a development feature. It makes it easy to test multiple layouts without having to restart rootston and helps to test situations where the layout is changing.

I'm not a big fan of it but also not opposed to it, but either way it belongs in a separate PR imo

Just this specific part of the rootston implementation or the whole rootston implementation?

The multiple layouts and switching at runtime part

An output that is made relative to another output will appear on one of the borders of the parent output. Which border the outputs attaches to can be configured. An exception to this is the SAME_AS configuration. In this case child output is positioned at the same location as the parent output. When the parent output changes position, the child oututs will follow in order to maintain teh same relative position. Relative outputs may overlap depending on the configuration. While the original issue had detection of overlapping outputs as a feature, this better fits in the compositor, since the library would have to make assumptions about the wanted behavior. An implementation for rootston can be found in later commits. When an ouput is removed and is a parent of another output, the child output is fixed at its current position. If the compositor requires different behaviour it can do this in response to the destroy signal. Fixes swaywm#550

In order to properly manage relative layouts in rootston, the 'wlr_output_layout' has been encapsulated in 'roots_layout'. This is primarily to deal with outputs not being available to reference. This lives in rootston since compositors might want to deal differently with missing outputs. The behavior of rootston in the case of missing outputs, is to mark rules whose relative output is not available as unconfigured. When an output is unconfigured it will be placed to the right of any other outputs. This way, no gaps will occur in the layout, unless the user specifically configures them with fixed output.

VincentVanlaer · 2018-06-02T21:07:06Z

Removed multiple layouts in rootston.

@acrisci seems like you have missed my comment: #1002 (comment)

emersion · 2018-06-04T20:54:39Z

rootston/rootston.ini.example

+# Output position configuration
+[layout]
+# Put the output with name VGA-1 at (0,0)
+VGA-1 = fixed 0 0


Possible to change this to 0,0 (one argument)?

emersion · 2018-06-04T20:56:27Z

types/wlr_output_layout.c

+		case WLR_OUTPUT_LAYOUT_OUTPUT_CONFIGURATION_FIXED:
+			break;
+		case WLR_OUTPUT_LAYOUT_OUTPUT_CONFIGURATION_RELATIVE_LEFT_OF: {
+				struct wlr_box *ref_box =


The indentation is a bit weird here

Ah, that's because of brackets? If you prefer, use case THING:;. But indenting with two levels is just weird.

I'm not a fan of that indentation either, but I'll just move the ref_box declaration above the switch statement.

types/wlr_output_layout.c

+			break;
+		case WLR_OUTPUT_LAYOUT_OUTPUT_CONFIGURATION_RELATIVE_LEFT_OF: {
+				struct wlr_box *ref_box =
+					output_layout_output_get_box(l_output->reference);


VincentVanlaer · 2018-06-04T21:59:18Z

types/wlr_output_layout.c

+				wl_container_of((child_output)->link.next, output, link)) {
+		while (child_output->reference != parent_output) {
+			if (parent_output == output) {
+				goto move_outputs;


I need to break out of two loops, hence the goto

emersion · 2018-06-04T21:09:51Z

types/wlr_output_layout.c

+		return;
+	}
+
+	for (child_output = wl_container_of((output)->link.next, output, link);


Not a fan of this complicated loop

Not sure how to do it with wl_list_for_each, I need to skip to the output that's being moved. It could be possible, but I don't feel it is all that cleaner.

emersion · 2018-06-04T21:20:09Z

types/wlr_output_layout.c

+	}
+
+move_outputs:
+	ref->next->prev = child_output->link.prev; // A->prev = B


Ah, so that's the code ordering outputs in topological ordering. I bet you can rewrite those manual pointer manipulations with wl_list_remove and wl_list_insert.

Yes I can, I was just being too perfectionistic w.r.t. the number of required operations.

types/wlr_output_layout.c

+	} else {
+		struct wlr_output_layout_output *l_output_rel;
+
+		wl_list_for_each(l_output_rel, &layout->outputs, link) {


emersion

This looks pretty good.

X=fixed 0,0 instead of X=fixed 0 0

VincentVanlaer · 2018-06-06T21:51:58Z

All issues are addressed. I added error handling to the config parsing of the layout to prevent rootston from segfaulting when a user created an invalid configuration.

emersion · 2018-06-08T07:07:10Z

rootston/layout.c

+	}
+}
+
+void roots_layout_reflow(struct roots_layout *layout) {


Sorry if this is obvious, but why is this required? I don't even understand what this functions does.

It finds a position for the outputs that rootston can't configure (no configuration is available or the reference output is not connected) such that nog gaps accur in the layout. The first loop obtains the extents of the outputs that rootston was able to configure, the second loop moves the outputs to a proper position.

You probably have already discussed about this, but why can't this happen in wlr_output_layout?

The idea was to make the auto configuration not too complex and have it as a set it and forget type of configuration for examples and prototypes. Having a very specific behavior for these kind of outputs in wlr_output_layout might be to prescriptive for how compositors should handle outputs when they cannot be fully configured. Moving this into the wlr_output_layout would cater this specific case, but might not be sufficient for other compositors. As a minor point, it also shows how one can extend the wlr_output_layout and allowed me to see where it could be improved. #1002 (comment)

Can you add a comment explaining what this function does, since it's not obvious from its name?

emersion · 2018-06-08T07:08:08Z

rootston/rootston.ini.example

+VGA-1 = fixed 0,0
+# Put the output with name VGA-2 to the left of the output with name VGA-1
+VGA-2 = left-of VGA-1
+# Other possibilities include : right-of, above, below, mirror


Typo: no space between "include" and the colon (:fr:)

emersion · 2018-06-08T07:09:42Z

types/wlr_output_layout.c

@@ -48,6 +47,17 @@ struct wlr_output_layout *wlr_output_layout_create(void) {
 static void output_layout_output_destroy(
 		struct wlr_output_layout_output *l_output) {
 	wlr_signal_emit_safe(&l_output->events.destroy, l_output);
+
+	struct wlr_output_layout_output *l_output_rel;


This code is duplicated in two places. Maybe it can be split in a function?

types/wlr_output_layout.c


-	if (max_x == INT_MIN) {


emersion · 2018-06-08T09:26:28Z

rootston/layout.c

+void roots_layout_reflow(struct roots_layout *layout) {
+	int max_x = INT_MIN;
+	int max_x_y = 0;
+	bool configured;


This can be accessed when uninitialized. Also, this variable doesn't seem to be used outside the loop, so it can be moved inside.

emersion

Nice work!

VincentVanlaer · 2018-06-20T09:55:41Z

Closed because it is too complex for wlroots. This should live completely in the compositor or in a separate client.

VincentVanlaer force-pushed the relative-layout branch from e4f8dc4 to 3b6da42 Compare May 28, 2018 11:23

emersion reviewed May 28, 2018

View reviewed changes

acrisci reviewed May 28, 2018

View reviewed changes

VincentVanlaer mentioned this pull request May 28, 2018

Make wlr_output_layout_output private #1013

Open

emersion reviewed May 31, 2018

View reviewed changes

rootston/config.c Outdated

free(lrc);

}

free(lc->name);

free(kc);

Copy link

Member

emersion May 31, 2018

Choose a reason for hiding this comment

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

kc?

emersion reviewed May 31, 2018

View reviewed changes

VincentVanlaer added 5 commits June 2, 2018 15:46

Remove layout_move as layout_add can do the same

1934793

Damage outputs on reflow

3837b92

Remove wlr_wl_list_for_each_offset macro

77ad9ab

VincentVanlaer force-pushed the relative-layout branch from 620dcd7 to 77ad9ab Compare June 2, 2018 14:04

Add wlr_output_layout_move

decfe45

emersion reviewed Jun 4, 2018

View reviewed changes

types/wlr_output_layout.c Outdated

break;

case WLR_OUTPUT_LAYOUT_OUTPUT_CONFIGURATION_RELATIVE_LEFT_OF: {

struct wlr_box *ref_box =

output_layout_output_get_box(l_output->reference);

This comment was marked as resolved.

Sign in to view

emersion reviewed Jun 4, 2018

View reviewed changes

types/wlr_output_layout.c Outdated

} else {

struct wlr_output_layout_output *l_output_rel;

wl_list_for_each(l_output_rel, &layout->outputs, link) {

This comment was marked as resolved.

Sign in to view

emersion reviewed Jun 4, 2018

View reviewed changes

VincentVanlaer added 3 commits June 6, 2018 20:38

Fix style

0f2e2d4

Change layout configuration format

c917384

X=fixed 0,0 instead of X=fixed 0 0

Improve handling invalid layout config in rootston

d09434f

emersion reviewed Jun 8, 2018

View reviewed changes

types/wlr_output_layout.c Outdated

if (max_x == INT_MIN) {

This comment was marked as outdated.

Sign in to view

emersion reviewed Jun 8, 2018

View reviewed changes

VincentVanlaer added 3 commits June 8, 2018 15:01

Fix auto outputs not behaving as auto outputs

2a33e03

Fix style

44e2785

Comment non intuitive behavior

7a18d51

emersion approved these changes Jun 9, 2018

View reviewed changes

emersion requested a review from ddevault June 9, 2018 20:43

VincentVanlaer closed this Jun 20, 2018

Add relative outputs to wlr_output_layout #1002

Add relative outputs to wlr_output_layout #1002

Conversation

VincentVanlaer commented May 26, 2018 • edited Loading

Ongy commented May 26, 2018

VincentVanlaer commented May 26, 2018

Ongy commented May 26, 2018

VincentVanlaer commented May 26, 2018

Ongy commented May 26, 2018

VincentVanlaer commented May 28, 2018

acrisci commented May 28, 2018

VincentVanlaer commented May 28, 2018

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

acrisci commented May 28, 2018

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

VincentVanlaer commented Jun 2, 2018

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.

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

This comment was marked as resolved.

emersion left a comment

Choose a reason for hiding this comment

VincentVanlaer commented Jun 6, 2018

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

emersion Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

emersion left a comment

Choose a reason for hiding this comment

VincentVanlaer commented Jun 20, 2018

VincentVanlaer commented May 26, 2018 •

edited

Loading

emersion Jun 8, 2018 •

edited

Loading