Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a wayland protocol for communicating layouts #112

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

Leon-Plickat
Copy link
Member

@Leon-Plickat Leon-Plickat commented Oct 4, 2020

TODO:

  • Protocol
  • River implementation
    • "float-mode" (when no layout is in use, like when an output has no namespace yet or there is no layout with a matching namespace)
      • Keep track of if view dimensions on an output are set by a layout client active (meaning last used or pending) layout per output.
      • Do not automatically set views to floating when moved/resize manually (with mouse or keyboard or command) when in "float-mode". This would not be expected by users and is confusing since the change is only visible once the views are layed out again. So instead let's just act like dwm, where all moving and resizing of non-floating windows in "float-mode" is forgotten once the next layout is active. This will also prevent all potential bugs and unexpected behaviour which could happen when a layout is demanded and one of the views is moved by the user before the layout is committed.
    • man page
      • Document default options of outputs
    • Block output transaction when layout demand is in progress
  • Rivertile implementation
    • river-layout
    • river-options
    • Gaps
      • use river-options
    • man page
  • Simple reference client implementation in contrib
    • Demonstrate river-layout
    • Demonstrate using river-option
  • New keybinds for default config
  • transaction

With the ack_layout_request event, multiple layout clients can run side by side. River uses the one which send the ack first and ignores the others. Idea: maybe people want to implement different layouts as different clients.

The serial in every request / event is needed because a layout client may be tasked with generating layout for multiple outputs simultaneously.

@Leon-Plickat Leon-Plickat marked this pull request as draft October 4, 2020 02:16
@Leon-Plickat
Copy link
Member Author

We also need to think about what happens when no layout client is running.

My current idea is to get rid of the built-in "full" layout and instead place new windows at 0,0 (or maybe center), let them have their requested size, and don't touch existing windows. That probably simplifies things and has the interesting side-effect that river could be used as a floating compositor.

@ifreund
Copy link
Member

ifreund commented Oct 18, 2020

We also need to think about what happens when no layout client is running.

My current idea is to get rid of the built-in "full" layout and instead place new windows at 0,0 (or maybe center), let them have their requested size, and don't touch existing windows. That probably simplifies things and has the interesting side-effect that river could be used as a floating compositor.

That sounds reasonable to me. I'm fine with river basically being a minimal floating compositor by default with no layout client running.

@ifreund
Copy link
Member

ifreund commented Oct 30, 2020

Commenting so we don't let it slide: we need to address the problem of switching between multiple layouts before committing to this protocol fully. A common use case would be using the default rivertile layout generator alongside a more specialized custom layout generator.

@Leon-Plickat
Copy link
Member Author

Actually there are two problems to solve here: How to switch between different layout generator clients and how to get arbitrary information from the user to a running client. The importance of the latter one can be demonstrated by an example: A user may want to switch the master position of rivertile (and running four instances of rivertile at the same time is a bit stupid IMO).

A simple solution to both problems is just letting the user kill the current layout generator and exec a new one (or the same one but with different args). That however is not a very clean solution and should be avoided if possible IMO.

I believe the first problem must be solved entirely in the compositor alone without any interaction with the client.

The first problem may be solved with namespaces: When creating a river_layout object, a client would need to register an arbitrary namespace string. When requesting a layout, river listens to the client which first acks the request which also has the correct namespace. A user could then switch namespaces with riverctl layout <namespace>. This approach allows multiple layout clients to run simultaneously, which I think is important to allows different layout across different outputs. (Note that I think the namespace should be different from the layout name, as the latter is pretty-printed data explicitly provided for the user and may change depending on different circumstances: A client may put things like the amount of views in its master area into its name string, or use somethin abstract, like the dwm-typical []= .)

For the second problem I see two approaches: 1) We add additional parameters to the layout requests, like direction (which are obviously also only optional just like the current ones), or 2) we add another arbitrary string, which is set by the user at runtime and send to the client upon a layout request. I personally prefer the first option.

@ifreund
Copy link
Member

ifreund commented Oct 31, 2020

When creating a river_layout object, a client would need to register an arbitrary namespace string. When requesting a layout, river listens to the client which first acks the request which also has the correct namespace. A user could then switch namespaces with riverctl layout

Why don't we just send the request to the first client to register in the target namespace? then we don't waste the user's cpu time waking up all the layout clients that will be ignored anyways.

@Leon-Plickat
Copy link
Member Author

Yeah that sounds reasonable.

@Leon-Plickat
Copy link
Member Author

I have been over this again and have a few thoughts:

  • Instead of the advertise_* events, wouldn't it be better to send the view states in a wl_array of a special struct (like river_layout_view_state) we define in the protocol? That would make layout client depending on the additional information considerably easier to write, as all logic would happen in response to a single event (the layout request).
  • The additional information should not include the current dimensions and position of the view. Layouts could otherwise depend on the current layout, which would be circular. Also those values are basically arbitrary, so layout logic could become opaque to the user, which we should avoid.
  • We now have namespaces and intend to only send the layout request to a single client. If we explicitly note this in the protocol, we can drop ack_layout_request as well as the "layout authority" concept, which would seriously simplify the protocol.

@ifreund
Copy link
Member

ifreund commented Dec 3, 2020

Instead of the advertise_* events, wouldn't it be better to send the view states in a wl_array of a special struct (like river_layout_view_state) we define in the protocol? That would make layout client depending on the additional information considerably easier to write, as all logic would happen in response to a single event (the layout request).

Hard nack, this would make the protocol dependent on the C ABI and there is no way for the scanner to scan arbitrary struct definitions out of the protocol code (for good reason). What we could and probably should do is provide example/skeleton code in a few popular languages to get people started, maybe even make it into a small library if it makes sense.

The additional information should not include the current dimensions and position of the view. Layouts could otherwise depend on the current layout, which would be circular. Also those values are basically arbitrary, so layout logic could become opaque to the user, which we should avoid.

Totally agree there.

We now have namespaces and intend to only send the layout request to a single client. If we explicitly note this in the protocol, we can drop ack_layout_request as well as the "layout authority" concept, which would seriously simplify the protocol.

This seems like a reasonable approach to take. How should we handle the case where the client requests a namespace that is already in use then? I suppose we could make that a protocol error but that seems harsh as clients have no way of knowing what namespaces are in use.

@Leon-Plickat
Copy link
Member Author

Hard nack, this would make the protocol dependent on the C ABI and there is no way for the scanner to scan arbitrary struct definitions out of the protocol code (for good reason). What we could and probably should do is provide example/skeleton code in a few popular languages to get people started, maybe even make it into a small library if it makes sense.

Yeah, I wasn't sure if it is possible, which is why I came up with the events in the first place.

I prefer the skeleton approach. A library would be a bit more involved to support.

This seems like a reasonable approach to take. How should we handle the case where the client requests a namespace that is already in use then? I suppose we could make that a protocol error but that seems harsh as clients have no way of knowing what namespaces are in use.

A protocol error is definitely too harsh. Also client could get different results (protocol error vs works as expected) when doing the exact same thing, which isn't a good thing IMO.

Maybe it should be possible for a client to change its namespace whenever it wants (instead of just allowing it to be set at connection) and river would then send an "invalid namespace" (or a more generic "connection refused") event if the namespace is already used or unsuitable for other reasons. I don't think we can get around this communication step if we want to handle this cleanly.

@Leon-Plickat
Copy link
Member Author

Leon-Plickat commented Dec 4, 2020

You are right, allowing clients to change their namespace at any time is probably racy. So maybe we don't allow that and when the namespace is invalid, the client can't do anything other than destroying the river_layout object. If it wants to try again, it has to bind a new one, or maybe we don't allow that, so it has to be explicitly restarted, forcing the user to know what is going on.

@Leon-Plickat Leon-Plickat force-pushed the river-layout-unstable-v1 branch 2 times, most recently from b2cb34d to 5b0d18b Compare December 7, 2020 01:29
@Leon-Plickat
Copy link
Member Author

Leon-Plickat commented Dec 7, 2020

  • Improved wording.
  • Rebased and squashed.
  • Removed old broken contrib layouts and added a new one.
    • Todo: More contrib layouts in different languages, like rust (zig probably not needed since rivertile exists).
  • Started porting rivertile.
    • Todo: gaps.

@Leon-Plickat Leon-Plickat force-pushed the river-layout-unstable-v1 branch 5 times, most recently from 246c654 to 11829d0 Compare December 13, 2020 23:49
Copy link
Member

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

This is looking very close to ready, there's still a bit of cleanup that can be done though.

protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
@Leon-Plickat
Copy link
Member Author

I think we have to rename "layout request" to something else, because technically it is not a request but rather an event. The current name is a bit confusing.

@Leon-Plickat Leon-Plickat force-pushed the river-layout-unstable-v1 branch 2 times, most recently from d38bd13 to 31aee90 Compare December 26, 2020 17:08
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
protocol/river-layout-unstable-v1.xml Outdated Show resolved Hide resolved
river/LayoutDemand.zig Outdated Show resolved Hide resolved
Copy link
Contributor

@YerinAlexey YerinAlexey left a comment

Choose a reason for hiding this comment

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

Looked at this patch again

@Leon-Plickat
Copy link
Member Author

Leon-Plickat commented Apr 18, 2021

Ah I see, I missed that pre_fullscreen_box was updated in every arrange while the view was in fullscreen. Given that behavior, I find the name a little misleading.

true

As for whether this is a good idea or not, I'm not entirely convinced. Generally, the less state we store the better as out-of-sync/stale state is a large source of bugs. I don't think a single extra arrange when transitioning a view out of fullscreen (a relatively rare event) is a significant issue. As for #243, we could restore views to their float_box on exiting fullscreen if there is no active layout.

That would mean that a views position when initially set to floating while a layout is active depends on whether it has been fullscreened earlier while no layout was active. Floating and fullscreen are from a UX perspective totally unrelated, so they should not form any causality chain in my opinion. Layout state being involved makes this even more awkward.

@ifreund
Copy link
Member

ifreund commented Apr 18, 2021

That would mean that a views position when initially set to floating while a layout is active depends on whether it has been fullscreened earlier while no layout was active. Floating and fullscreen are from a UX perspective totally unrelated, so they should not form any causality chain in my opinion. Layout state being involved makes this even more awkward.

I wasn't suggesting that we should set the float_box when making a view fullscreen, only that we could use whatever the float_box happens to be as a reasonable size/position to give a view on making it not fullscreen if there is no layout.

@Leon-Plickat
Copy link
Member Author

Oh, yeah that's better. Though it would mean that after leaving fullscreen all views would snap the their float_box, no matter what their previous box actually was, if no layout is active.

@ifreund
Copy link
Member

ifreund commented Apr 18, 2021

Oh, yeah that's better. Though it would mean that after leaving fullscreen all views would snap the their float_box, no matter what their previous box actually was, if no layout is active.

Hmm, we can leave views that weren't fullscreen at their previous dimensions, but then it might be strange that only the fullscreened view ends up in its float_box dimensions.

Maybe the approach you implemented is the best way to go, though we should s/pre_fullscreen_box/post_fullscreen_box/.

@Leon-Plickat
Copy link
Member Author

Changed the name.

@Leon-Plickat
Copy link
Member Author

Anyway, I would like to "feature freeze" this PR now and only include your upcoming options change and strictly layout related bug fixes.

What do you think about the git history? I think some commits maybe should be squashed.

river/View.zig Outdated Show resolved Hide resolved
@ifreund
Copy link
Member

ifreund commented Apr 18, 2021

Anyway, I would like to "feature freeze" this PR now and only include your upcoming options change and strictly layout related bug fixes.

What do you think about the git history? I think some commits maybe should be squashed.

I'll do a pass over the commits just before landing this in master. I've got a few things to debug with the options changes before I push them. Would you mind updating the example C layout for the new options protocol? If you want to get started before I push you can find the xml here: https://0x0.st/-A9l.xml

@Leon-Plickat
Copy link
Member Author

Great, I'll try to find some time tomorrow for the contrib layout.

@ifreund
Copy link
Member

ifreund commented Apr 18, 2021

Alright, the initial implementation of river-options-v2 is done and seems to work well. One minor pain point is that users wanting to configure the options of e.g. rivertile before starting river tile must declare said options themselves. I'm not quite sure what, if anything, we should do about this though, so I don't think it needs to block the merge.

Barring any bugs I'm unaware of, the current merge blockers are updating the contrib layout in C for river-options-v2 and reviewing the documentation/example init script.

@snakedye
Copy link

@ifreund Why should I destroy an undeclared option?

image

To give context, I use this option to send commands to my client. It is cleared (I set it's value to null) after the command is interpreted so I'm notified of following one.

@ifreund
Copy link
Member

ifreund commented Apr 19, 2021

@ifreund Why should I destroy an undeclared option?

image

To give context, I use this option to send commands to my client. It is cleared (I set it's value to null) after the command is interpreted so I'm notified of following one.

The protocol has changed, options must be declared explicitly with one of the declare_foo_option requests and may no longer be implicitly declared through a handle to an undeclared option. Please give the new protocol a read and let me know on IRC if you have any more questions.

Leon-Plickat and others added 4 commits April 20, 2021 18:10
Replace the current layout mechanism based on passing args to a child
process and parsing it's stdout with a new wayland protocol. This much
more robust and allows for more featureful layout generators.

Co-authored-by: Isaac Freund <ifreund@ifreund.xyz>
Run the init command in a new process group and send SIGTERM to the
entire group on exit. Without doing this, only the sh invocation used
for the `sh -c` would receive SIGTERM.

This is particularly useful when starting a per-session server manager
as the init command.
Options are now all global but may be overridden per-output. If an
output local value is requested but none has been set, the global value
is provided instead. This makes for much better ergonomics when
configuring layout related options in particular.
- Remove old layouts which no longer work.
- Add new C layout.
Copy link
Member

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Very nice work on this @Leon-Plickat!

@ifreund ifreund merged commit 924a470 into riverwm:master Apr 20, 2021
@Leon-Plickat Leon-Plickat deleted the river-layout-unstable-v1 branch April 20, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants