Add reverse layouts

[wlhlm]

This is an implementation of #1767 (allow splitting to the left/top) and addresses the underlying desire to have layouts that are filled in from right to left and bottom to top.

I have dubbed this the "layout fill order" which has two values "default", and "reverse", where the former denotes the old behavior, where new windows get added on the right in horizontal layouts and on the bottom in vertical layouts. The latter reverses this order: windows get added to the left in horizontal layouts and to the top in vertical layouts.

I have had a tough time coming up with an actual name for this functionality, not sure if "layout fill order" is intuitive for everybody, so I'm open to suggestions.

Closes #1767.

Implementation

The discussion in #1767 suggested adding additional layouts that are populated in reverse (by extending layout_t), but I think that begs a larger refactoring as in many places, the normal layout is equivalent with its reverse counterpart (let's say L_SPLITH and L_SPLITH_REVERSE), requiring layout checks to be doubled from layout == L_SPLITH to layout == L_SPLITH || layout == L_SPLITH_REVERSE.

To me, the way how a layout is populated with new windows is orthogonal to the layout itself. On a lower level, all the fill order does, is affect whether a new child node gets prepended or appended to the list of existing child nodes of a parent container, regardless of the actual layout itself. My solution is to store this in an additional property side-by-side with the layout. This let's me keep existing layout checks intact and only requires updating the few places that care about the fill order (mainly con_attach()).

User-facing changes

• command split was extended to accept the arguments left, right, up, and down
  • split right and split down are aliases for split h and split v respectively
  • split left creates a horizontal split with reverse fill order so that the next window gets attached to the left
  • split up is similar, but creates a vertical reverse split
• command workspace_layout <layout> now accepts an additional argument reverse to set the default fill order for workspaces to reverse
• command layout <default|stacked|stacking|tabbed|splitv|splith> received an optional argument reverse
  • for example layout splith reverse changes the layout to splith with a reversed fill order (new window will get inserted before focused window instead of after).
• command layout fill_order [default|reverse|toggle] allows changing the fill order independent of the layout
• split indicators are drawn close to where a new window will appear
  • for a horizontal split with default fill order, the indicator will be drawn on the right border (as before)
  • for a horizontal split with reverse fill order, the indicator will be drawn on the left border
  • vice-versa for vertical splits
• the tree representation string format (mostly seen in container titles and the testsuite) uses the 'r' suffix to denote splits with reverse fill order, for example Vr[] for a vertical split with reverse fill order.

TODO

Currently, documentation is missing completely as that makes only sense to add when the maintainers are happy with the implementation.

[orestisfl]

Thank you for your PR.

On first impression, this seems to be well thought-out and of high quality. Of course we will need to go through some iterations and I will probably try to run this on my machine as well.

Some first thoughts:

• How does this affect floating disable. I.e. when a floating window has to be moved in a "reverse" split, does it get positioned on the correct place?
• How does this affect moving? I.e. with H [ Vr [ A B ] C ] what happens if you move C left?

[wlhlm]

• How does this affect floating disable. I.e. when a floating window has to be moved in a "reverse" split, does it get positioned on the correct place?

Good question! I haven't done any testing with floats. I'll take a look.

• How does this affect moving? I.e. with H [ Vr [ A B ] C ] what happens if you move C left?

This should be unaffected as moving (in move.c) directly manipulates the child node SLIST and doesn't call _con_attach() which is where I've added the new logic. Though adding a test case won't hurt :).

EDIT: On a closer look, there are cases where tree_move() is biased towards (default) left-to-right/top-to-bottom fill ordering. While my changes don't affect this negatively, the logic could be fleshed out to reverse this bias for reverse fill order.

[wlhlm]

• How does this affect floating disable. I.e. when a floating window has to be moved in a "reverse" split, does it get positioned on the correct place?

I've gone over the floating disable code, done some minor tweaking, and added a few testcases. Should work as expected now.

• How does this affect moving? [...]

I've extended the move code a bit to better accommodate reverse layouts. A couple of testcases where added for good measure.

PTAL!

[orestisfl]

To reduce the number of ternaries in multiple parts of the code, WDYT about adding some helper functions in src/util.c?

position_t position_maybe_reverse(position_t position, layout_fill_t order) {
    if (order == LF_DEFAULT) {
        return position;
    } else {
        return position == BEFORE ? AFTER : BEFORE;
    }
}

direction_t direction_maybe_reverse(direction_t direction, layout_fill_t order) {
    if (order == LF_DEFAULT) {
        return direction;
    }
    switch (direction) {
        case D_LEFT:
            return D_RIGHT;
        case D_RIGHT:
            return D_LEFT;
        case D_UP:
            return D_DOWN;
        case D_DOWN:
            return D_UP;
    }
    assert(false);
}

[wlhlm]

Thanks for taking a look.

To be honest, I'm having some trouble figuring out how your suggestions are supposed to improve the code.

Let me start with position_maybe_reverse():

The way i3 windowing works is, it builds a tree of containers where the containers on the leaves of the tree are windows. i3 then takes this tree and arranges the windows on the screen based on their position within the tree. With this model in mind, there are two ways I can implement reverse layouts here. I can either 1) change how containers get placed into the tree, or 2) change how windows get arranged on the screen. Option 2 is also called layout.

I have implemented option number 1 in this PR, since changing window layouting sounded harder to implement and more difficult to test.

Now, remember that despite having implemented option 1, the way reverse layouts are exposed to the users is as "layout fill order" and as the word "layout" alludes to, these config options and commands describe implementation option 2 above. This means that every time a reverse layout is relevant for tree manipulation, I need to translate the layout fill order option to the appropriate tree operation. This seems to be the idea behind position_maybe_reverse(). For example, I want to place a con after another con in a reverse horizontal layout. In this kind of layout "after" means placing it on the left of the target. Considering the implementation in this PR, the appropriate tree operation to achieve this layout is to place the con before the target con in the tree. This translation of after to before is handled by position_maybe_reverse().

All of this is a long-winded way of saying that the suggested signature of position_maybe_reverse() is incorrect since its position_t input and position_t output have the same types, but they are different from a semantic standpoint. A better version of this function would be:

tree_position_t position_maybe_reverse(layout_position_t position, layout_fill_t order)

but that would require me creating new types and I don't think that's worth it for the few times this function is relevant.

Sorry, I also don't see where I could use direction_maybe_reverse(). Maybe you could comment on specific lines/areas of the code where you think it could be improved.

[orestisfl]

Hi, I think you overestimated the applicability of my comment, it's just a low level abstraction to remove some of the complexity from conditionals in the new code. I've highlighted some lines where it seems that you can use it.

By all means, if you think that it does not make the code more readable, you don't need to integrate them in your implementation.

[wlhlm]

I've factored out the complicated assignment, though I wasn't able to find a way to further simplify it, the suggested functions didn't really help. I think the other assignments are fine as they are.

[wlhlm]

I just rebased and would like to request that the maintainer take a look again. Is there anything major that needs changing? I've put quite a bit of effort into coming up with sensible test cases, so at this point I'm pretty confident that this PR does what it says on the tin. Of course, we can discuss if it is the right tin in the first place ;)

[orestisfl]

Thanks!

I will try to use it a bit tonight/tomorrow and hopefully provide more helpful feedback ;)

[Airblader]

@orestisfl Did you have time to try it out? @wlhlm just pinged me about this on reddit regarding the new feature policy, and I think given the work put into this already we should continue here unchanged.

[orestisfl]

@Airblader I agree. I just installed the patch. Sorry for the delays.

[orestisfl]

Thank you for your hard work!

I think it's time to write the user documentation.

[orestisfl]

Please use curly braces for new code (here and everywhere)

[orestisfl]

Not a huge fan of this function being in con.c since it takes a char * as an argument. I would make it a static inside commands.c.

[orestisfl]

Maybe also make the function accept NULL?

Suggested change

	if (strcasecmp(fill_order, "default") == 0) {
	if (fill_order == NULL || strcasecmp(fill_order, "default") == 0) {

[dmolin]

Any further update on this PR? @wlhlm are you still with us 🙏 ? Tbh this glaring missing feature of I3 is the ONLY thing that keeps me (and I'm very sure many others) from switching from Bspwm over to I3. Can some good soul make the extra effort to push this through the finish line?

[jtaala]

I would feel absolutely happy (privileged even!) to help get this PR across the line.

Given it seems @wlhlm may not be available/around the project as of late, I've merged the great work in his right-to-left-layouts branch into my fork and have brought it up to date with the latest i3:next.

@orestisfl, I've also implemented your requested changes in #4246 (review).

Please see next...jtaala:right-to-left-layouts for @wlhlm's work (and my small changes implementing @orestisfl's requested changes) diff'd with latest i3:next.

I've been testing @wlhlm's work in my fork and have found no issues thus far, and I've also discovered that I really like right-to-left/bottom-to-top behaviour much better (just my preference).

Given the hard work of @wlhlm and the quality of his work, plus the evident usefulness of the feature - I would be happy to help write the documentation now to get it into i3:next.

@orestisfl, @Airblader, (and @wlhlm if you're around) - would you be happy with me picking up and progressing this? Would likely mean once done, I would need to create a new PR (fully referencing this one of course).

Please see new PR with my (small) changes and documentation added here #4794.