Add wlr_output_manager support #62

Uks2 · 2022-06-20T17:00:15Z

This allows things like wlr-randr to work.

There's the standard wlroots plumbing to connect up, plus a few extra
checks here and there to make sure we're not trying to access an output
that doesn't exist anymore.

ghost

Hah, i’ve been wishing to implement this one for quite long now, thanks! ❤

As i’m not familiar with the protocol, this is more of a first-look review; hopefully i’ll manage to take the time and check also with the protocol (but by no chance should i be blocking merge).

Also, i’m only a contributor with no authority; what i say may be completely different from what buffet would say, so keep that in mind 😉

include/desktop/output.h

kiwmi/desktop/layer_shell.c

kiwmi/desktop/output.c

kiwmi/desktop/layer_shell.c

ghost

Thanks! Just one omission and reviewing my own code 😂

Also feel free to mark any of the conversations as resolved; i don’t know why GitHub limits that to the PR author and repo owner.

kiwmi/desktop/output.c

ghost · 2022-06-21T21:01:38Z

Can you split the diff unrelated to wlr_output_manager into a separate commit? It’s quite small i know, but it would help make the history cleaner.

ghost

On my way to check it with the protocol :) (But i can’t promise i’ll have done it by tomorrow.)

kiwmi/desktop/output.c

include/desktop/output.h

ghost

Enjoy my thought train jumping back and forth 😇, i hope you can understand it.

Untested so far, only reviewed.

kiwmi/desktop/output.c

include/desktop/output.h

kiwmi/desktop/output.c

ghost · 2022-06-29T10:10:18Z

kiwmi/desktop/output.c

+        if (okay && !test) {
+            struct kiwmi_output *output = head->state.output->data;
+            if (head->state.enabled) {
+                if (output->enable_changed) {


Lol, i’m dumb. Of course would i focus on writing those mostly worthless nitpicks and forget that if enable_changed, the output should be added to / removed from the output_layout to match the behaviour of other compositors. (And, because we don’t want to send intermediate state, the output_layout_change listener should be disabled / no-op while the configuration is in progress. Which also somewhat replies to #62 (comment).)

(This one’s also resolved with the 2022-07-11 force-push; the parenthesised part lives on in another comment below.)

Uks2 · 2022-06-30T21:25:13Z

Wait! No! This version isn't sending events to lua at all. Rats.

I'll see if I can fix that tomorrow...

kiwmi/desktop/output.c

Uks2 · 2022-07-01T20:24:22Z

Another small problem I've noticed: if you disable a monitor using wlr-randr or whatever, the bounds for the cursor remain unchanged; you can move it off screen, into the invisible place where that monitor used to be.

I'm not sure why that's happening. When we create a wlr_cursor, we associate the wlr_output_layout with it, and wlroots registers its own handler for when that layout changes. I'll need to take a closer look at some point.

ghost · 2022-07-02T07:22:37Z

I believe that’s related to #62 (comment): the wlr_output_layout doesn’t add/remove outputs as they are enabled/disabled, the compositor has to do it itself. (This behaviour is useful e.g. for wlr-output-power-management.)

kiwmi/desktop/output.c

ghost

I suppose i can keep nitpicking forever, oh well 🤦. The main logic seems good though!

This revision untested yet, as i’ve got a dirty tree that i’m lazy to clean atm, but your changes are okay. IIRC there was some issue before with sometimes erroneously getting a new output event, so i’ll see if it’s still there or got fixed mid-way (ignore it until i report back).

ghost · 2022-07-11T20:14:47Z

kiwmi/desktop/output.c

+void
+output_layout_change_notify(struct wl_listener *listener, void *UNUSED(data))


Note that this gets called also every time we modify the layout from within output_manager_configure(). Possible ideas how to deal with it:

have some state saying whether we’re in output_manager_configure() – if yes, return early

issue: existence of state similar to previous kiwmi_output.enable_changed

unregister the listener in o_m_c(), re-register at the end

issue: the listener will soon be used for other purposes (i use it in my PR as well)

(variation of previous point) unregister the listener, re-register between the 2nd loop and the final ifs; replace the set_configuration() call with a manual emission of the event

issue: quite complicated

keep it as it is and don’t fix what seems to work

Should also note that the config can misbehave and change the layout from within the callback, in which case it has to be updated before being sent to clients.

I suppose i can keep nitpicking forever, oh well .

It's okay, I do have a talent for producing extremely nitpickable work

I've opted to just move the contents of that listener into a normal function that's called in the couple of places that it's needed. Nothing should be changing the output_layout outside of this file so it should work fine (correct me if I'm wrong). Seemed simpler than having a listener that doesn't actually correspond to the event it's listening to.

Nothing should be changing the output_layout outside of this file so it should work fine

The config can change it. And it seems pretty hard to me to get this right without removing kiwmi_output:move().

(so perhaps leave it be and add a todo? but were i the maintainer, i wouldn’t be happy merging such a thing)

Humm, not only the config. If nested kiwmi is requested by its host compositor to resize, it also modifies the output layout without any direct action by us.

Good spot and I forgot that the config could move outputs.

I've added output_manager_update calls for both cases (changing the size of nested compositors sets off the mode change listener, so I've put that one in there). This does make things a little less neat though.

Given that your scene graph stuff also messes around with this, I'd suggest leaving it be for now, then once you've got your scene graph code finalised, you can make an informed decision on what's the neatest way of structuring it.

Given that your scene graph stuff also messes around with this, I'd suggest leaving it be for now, then once you've got your scene graph code finalised, you can make an informed decision on what's the neatest way of structuring it.

Well, scratch that. Looks like I've got a rebase to do.

ghost · 2022-07-11T20:17:04Z

kiwmi/desktop/output.c

+    struct kiwmi_output *output;
+    wl_list_for_each (output, &desktop->outputs, link) {


Self-note: should fix what’s iterated in my rebase (also if i rebase the scene PR onto this); then also the area!=NULL check below makes sense.

kiwmi/desktop/output.c

ghost · 2022-07-11T20:25:08Z

kiwmi/desktop/output.c

+                        output->wlr_output,
+                        head->state.x,
+                        head->state.y);
+                    wl_signal_emit(&output->events.resize, output);


This event is emitted even if the output isn’t resized. I think a check similar to enable_changed would fix that. (Or should it be preserved so that all outputs emit some event?)

The resize signal is now emitted if there are pending mode, scale or transform changes, or if head->state.x/y are different to the current values. Having moves trigger a resize event is a bit weird but I think the config should be told when an output moves and having seperate move and resize events doesn't seem worth the effort.

ghost · 2022-07-12T12:41:33Z

IIRC there was some issue before with sometimes erroneously getting a new output event, so i’ll see if it’s still there or got fixed mid-way

Seems like it got fixed and/or i remember wrongly.

Edit: for the record, it’s only me being dumb again and testing my rebase rather than this PR. That wrong event was apparently caused by changes in wlroots 0.16. No need to worry for now; it can be solved when the time comes.

kiwmi/desktop/output.c

ghost · 2022-07-16T11:19:28Z

The commit message also isn’t right anymore (fortunately):

It required the addition of a flag to the output structure, because,
when applying or testing a new layout, you have to go through each
output and check that all their new configurations work. Then, if they
all do, you go through again commit those changes and send off new
output events and such. But you need a way of keeping track of which
outputs have had their enabled status changed and it was simpler to just
put that in the output's struct than to create some whole new data
structure.

This is basically some pre-emptive bug fixing before adding wlr_output_manager support in the next commit.

ghost

Seems okayish, only the layout change propagation is more complicated.

ghost · 2022-07-30T20:02:05Z

kiwmi/desktop/output.c

+                bool moved = output->wlr_output->pending.committed
+                    & (WLR_OUTPUT_STATE_MODE | WLR_OUTPUT_STATE_SCALE
+                       | WLR_OUTPUT_STATE_TRANSFORM);
+                struct wlr_output_layout_output *layout_output =
+                    wlr_output_layout_get(
+                        server->desktop.output_layout, output->wlr_output);
+                moved |= head->state.x != layout_output->x
+                    || head->state.y != layout_output->y;


I wouldn’t be scared of making this two variables, moved & resized. Would help code understanding and also make it easier to add a separate move event to kiwmi_output if desired.

Fair, though I'd recommend we keep moves and resizes in the same event, seeing as the vast majority of configs will be doing exactly the same thing in each case.

ghost · 2022-07-30T20:02:28Z

kiwmi/desktop/output.c

+            if (head == bad_head) {
+                break;
+            } else {
+                continue;
+            }


My logic would be, if we’ve gotten here, continue, instead of either break, or continue. Seems very subjective though.

Suggested change

if (head == bad_head) {

break;

} else {

continue;

}

if (head == bad_head) {

break;

}

continue;

If there's jumps on both outcomes of the if, I generally like to use if/else. Makes it clearer to me that the if statement is a for in the control flow rather than a detour that will rejoin.

I can change it if you'd prefer though.

This allows things like wlr-randr to work. wlr-randr or similar can send in two new events: `output_manager_apply' and `output_manager_test'. In output.c, their handler both call an new `output_manager_configure' function which loops through the list of outputs twice. The first loop applies all the requested configuration ad checks that its not all messed up. The second loop either commits that configuration or reverts it depending on whether it worked and whether we're responding to a test event. There's also now an output_manager_update function, called whenever the output layout is changed, which copies changes from the wlr_output_layout to the wlr_output_manager.

ghost suggested changes Jun 20, 2022

View reviewed changes

Uks2 force-pushed the output-managing-branch branch from 547d120 to 53b5eac Compare June 20, 2022 20:36

ghost suggested changes Jun 21, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

Uks2 force-pushed the output-managing-branch branch 2 times, most recently from fceb9a4 to 0acd10a Compare June 21, 2022 20:25

Uks2 force-pushed the output-managing-branch branch from 0acd10a to 8efdb8d Compare June 22, 2022 06:37

ghost suggested changes Jun 26, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

include/desktop/output.h Outdated Show resolved Hide resolved

ghost suggested changes Jun 27, 2022

View reviewed changes

ghost reviewed Jun 29, 2022

View reviewed changes

Uks2 force-pushed the output-managing-branch branch 2 times, most recently from 6db8601 to d475202 Compare June 30, 2022 20:59

ghost suggested changes Jul 1, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

ghost reviewed Jul 1, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

Uks2 force-pushed the output-managing-branch branch from d475202 to d3f4100 Compare July 1, 2022 20:14

ghost suggested changes Jul 3, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

Uks2 force-pushed the output-managing-branch branch from d3f4100 to 782c945 Compare July 11, 2022 15:23

ghost reviewed Jul 11, 2022

View reviewed changes

ghost reviewed Jul 12, 2022

View reviewed changes

kiwmi/desktop/output.c Outdated Show resolved Hide resolved

ghost mentioned this pull request Jul 14, 2022

output: wlr_renderer_begin with physical size #66

Merged

Handle layers when removing outputs

cb7c633

This is basically some pre-emptive bug fixing before adding wlr_output_manager support in the next commit.

Uks2 force-pushed the output-managing-branch branch from 782c945 to 8916132 Compare July 30, 2022 18:34

ghost reviewed Jul 30, 2022

View reviewed changes

Uks2 force-pushed the output-managing-branch branch from 8916132 to 69507ee Compare July 30, 2022 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wlr_output_manager support #62

Add wlr_output_manager support #62

Uks2 commented Jun 20, 2022

ghost left a comment

ghost left a comment

ghost commented Jun 21, 2022

ghost left a comment

ghost left a comment

ghost Jun 29, 2022

ghost Jul 11, 2022

Uks2 commented Jun 30, 2022

Uks2 commented Jul 1, 2022

ghost commented Jul 2, 2022

ghost left a comment

ghost Jul 11, 2022

ghost Jul 14, 2022

Uks2 Jul 30, 2022

ghost Jul 30, 2022

ghost Jul 30, 2022

Uks2 Jul 30, 2022

Uks2 Jul 30, 2022

ghost Jul 11, 2022

ghost Jul 11, 2022

Uks2 Jul 30, 2022

ghost commented Jul 12, 2022 •

edited by ghost

Loading

ghost commented Jul 16, 2022

ghost left a comment

ghost Jul 30, 2022

Uks2 Jul 30, 2022

ghost Jul 30, 2022

Uks2 Jul 30, 2022

		void
		output_layout_change_notify(struct wl_listener listener, void UNUSED(data))

		struct kiwmi_output *output;
		wl_list_for_each (output, &desktop->outputs, link) {

Add wlr_output_manager support #62

Are you sure you want to change the base?

Add wlr_output_manager support #62

Conversation

Uks2 commented Jun 20, 2022

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost commented Jun 21, 2022

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uks2 commented Jun 30, 2022

Uks2 commented Jul 1, 2022

ghost commented Jul 2, 2022

ghost left a 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

ghost commented Jul 12, 2022 • edited by ghost Loading

ghost commented Jul 16, 2022

ghost left a 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

ghost commented Jul 12, 2022 •

edited by ghost

Loading