-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Rework split, layout, workspace_layout, and move commands to work like i3 #5756
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
Conversation
Wow, you beat me to the punch here -- I was going to make the same change tonight. The existing However, this isn't quite right (I know you haven't marked it as ready for review yet etc., but anyway). The check for the parent layout to be a [vh]split is actually necessary, because if you have a Probably a good idea to leave a comment linking to the i3 source here, because this is pretty non-obvious. |
6f541c4
to
7ee8a53
Compare
Fixes #5755, #5754, Re-fixes #5157, #4038. I ended up going through a bunch of i3-compat issues but I think this matches i3 in the above cases. I don't think 5755 is related to 5754 just my fixes ended up on the same branch. Yeah thanks @Xyene I forgot to copy that part. I added a comment as suggested. EDIT: This will probably need some testing |
For #4038 OP seems to describe i3 behavior as |
I'm not entirely sure how...
This state is not correct. Subsequent |
Yeah this is wrong in that case.
i3 doesn't always wrap workspaces, just when changinging the layout on a single container. Otherwise they change the workspace layout. The difference here is in the behavior of the layout command, which I guess I should also update to match: https://github.com/i3/i3/blob/3cd1c45eba6de073bc4300eebb4e1cc1a0c4479a/src/con.c#L1817 The case I was trying to get at (and how I interpreted #4038) is Begin: empty workspace
|
Getting better 👍 The case I described still happens with |
Wow no kidding. In i3 The sway(5) man page will probably need to be updated with a proper description of |
Yeah, it's tricky. Bit of a meta-comment, and you may already have a testing setup that works for you, but I've found the following to be very helpful in comparing i3 with Sway:
|
So far I've been running i3 in a separate session and sway embedded in gdb. It's a workable setup but could be improved. The i3 layout dump will certainly be handy, I had a basic one but it was still bit of a hassle to interpret. I need to test more i3 layouts Quick take at matching i3 workspace_layout behavior like https://github.com/i3/i3/blob/e54e88b9e58579b406c5de94b68cb9ad3211eb4e/src/workspace.c#L899, but I'm guessing code will need to be moved somewhere more appropriate than view_map to match all the cases. |
The latest commit certainly improves things. I'm generally a fan of keeping the workspace node layout intact (i3-style) over what Sway currently does; it makes debugging i3-compat issues easier. With Start state{
"name": "1",
"type": "workspace",
"layout": "splith",
"nodes": [
{
"name": null,
"type": "con",
"layout": "tabbed",
"nodes": [
{
"name": "xyene@lynwood: /code/sway-src/sway",
"type": "con",
"layout": "none",
"nodes": []
},
{
"name": "xyene@lynwood: /code/sway-src/sway",
"type": "con",
"layout": "none",
"nodes": []
}
]
}
]
} Sway result{
"name": "1",
"type": "workspace",
"layout": "splith",
"nodes": [
{
"name": null,
"type": "con",
"layout": "tabbed",
"nodes": [
{
"name": "xyene@lynwood: /code/sway-src/sway",
"type": "con",
"layout": "none",
"nodes": []
}
]
},
{
"name": "xyene@lynwood: /code/sway-src/sway",
"type": "con",
"layout": "none",
"nodes": []
}
]
} i3 result{
"name": "1",
"type": "workspace",
"layout": "splith",
"nodes": [
{
"name": null,
"type": "con",
"layout": "tabbed",
"nodes": [
{
"name": "xyene@lynwood: /code/i3",
"type": "con",
"layout": "splith",
"nodes": []
}
]
},
{
"name": null,
"type": "con",
"layout": "tabbed",
"nodes": [
{
"name": "xyene@lynwood: /code/i3",
"type": "con",
"layout": "splith",
"nodes": []
}
]
}
]
} The extracted container should "inherit" the layout of the workspace. (This is effectively what #5157 was about.) |
Hopefully improved. I Only tested it in a few cases and had #5761 applied on top. I moved the change into workspace_add_tiling where it obviously belongs, but lots of places aren't happy to have workspace_add_tiling put their con as a child of a new container instead of the workspace they requested. So, most calls of workspace_add_tiling are changed to something like |
That case works much better now. Quick testing: starting with a |
One more weird case with Log
|
Oops, pushed the corrected version of that commit.
I don't observe that with your #5761 applied. I guess it gets reaped. |
Exciting, it's getting there :)
You're right, oops. It does seem that some extraneous containers are still being created with Log
|
The commit I removed was just one that had commented out some code. Now workspace_wrap_children now doesn't double wrap, that was creating some extra containers. So here's an interesting layout with
In the last move, though it's pretty unwieldy, the sway layout is the one that matches my mental model of the i3 tree. I'd guess it's special cased somewhere, but atm I'm not sure what i3 is doing here to get the simpler layout. |
I'd guess it's this: https://github.com/i3/i3/blob/1f0c628cde40cf87371481041b7197344e0417c6/src/tree.c#L638-L651 /*
* tree_flatten() removes pairs of redundant split containers, e.g.:
* [workspace, horizontal]
* [v-split] [child3]
* [h-split]
* [child1] [child2]
* In this example, the v-split and h-split container are redundant.
* Such a situation can be created by moving containers in a direction which is
* not the orientation of their parent container. i3 needs to create a new
* split container then and if you move containers this way multiple times,
* redundant chains of split-containers can be the result.
*
*/
void tree_flatten(Con *con) { Called right at the end of We don't have a version of this in Sway AFAICT; |
Rebased on master again so the CI can build. |
So far, so good. I haven't encountered any major issues in the last 2 days, so that's encouraging. Here's a small one, though:
The resulting layout is something like @RedSoxFan, do you have any thoughts on this PR? I think it's roughly in a state where it could be merged: I think that some corner cases might still be wrong, but by and large it's less finicky than current master. |
Briefly tested this branch, it seems to fix nwg-piotr/autotiling#19 |
Hmm I'm not able to reproduce. After the focus parent the H split is focused, right? In this branch and in i3 I get T[H[H[app app]*]] after the split and then all further splith's are no-ops. Actually it seems if you focus parent again in both this branch and i3 you can get more H[ by splith'ing the topmost H. Something is fishy with workspace layouts still. Without setting |
Yes, though oddly I haven't been able to reproduce this since. What I feel (since my memory isn't that good) was happening was the new container was getting focus, as opposed to the old one keeping it. At any rate, without being able to reproduce this, it's not worth worrying about now.
But... not quite? On i3 with Edit: I just understood what you mean... yeah, that behavior from i3 feels pretty strange to me. |
I've been running this for the past week with no deal-breaking issues. In general, I'd be comfortable with merging this before we get an authoritative response regarding On the "of some concern" list, I've noticed Firefox (84.0b3) geometries are wrong when floated out of the tiling layer. I'm not sure if this is caused by this patch (the geometry resetting?) or by a regression in Firefox, but at least I wasn't able to (easily) reproduce this issue in Sway master. |
In i3 splits are ineffective on singleton H/V containers, and the command is interpreted to affect the parent layout instead.
In i3 the layout command on a workspace affects the workspace layout only on empty workspaces. Otherwise children are placed in a new container with the desired layout to preserve the workspace layout.
Some comparisons of current Sway versus i3 behavior: 1) T[T[T[app]]] + move left * Sway: T[app] * i3: T[T[app]] 2) H[V[H[V[app]]]] + move left * Sway: H[app] * i3: H[V[app]] After this commit, Sway behavior matches i3. The intermediate states are now: T[T[T[app]]] -> T[T[app T[]]] -> T[T[app]] H[V[H[V[app]]]] -> H[V[app H[V[]]]] -> H[V[app]]
This is in preparation for changing the workspace_layout command to work like it does in i3. This reverts commit b4a75a1.
In i3, the workspace_layout command does not affect the workspace layout. Instead, new workspace level containers are wrapped in the desired layout and the workspace layout always defaults to the output orientation.
This changes the move command to better match i3 behavior after the layout changes. workspace_rejigger handled the case where containers would escape their workspace in an orthogonal move by changing the layout to accomodate them, but this case is now handled within the loop.
workspace_squash is container_flatten in the reverse direction. Instead of eliminating redundant splits that are parents of the target container, it eliminates pairs of redundant H/V splits that are children of the workspace. Splits are redundant if a con and its grandchild have the same layout, and the immediate child has the opposite split. For example, layouts are transformed like: H[V[H[app1 app2]] app3] -> H[app1 app2 app3] i3 uses this operation to simplify the tree after moving heavily nested containers to a higher level in the tree via an orthogonal move.
I tried FF83 and 84 but it doesn't seem to happen for me. Is it reproducible everytime for you? Is there a chance it's fixed by #5812? I think I'm running that one too.
Done. |
Unfortunately yes, and partially. #5812 does seem to prevent the issue from being permanent (once Firefox is in a bad state, it sends another configure a bit later, which after #5812 fixes the borders). Here's a video. There are at least two issues:
I don't think this is blocking for merging this PR, since it might just be a Firefox-side issue.
This is frame 118/550 of the video. I suspect this is happening as the floated container is being split on being added back into the tiling layer, prior to being squashed again. |
Changes workspace prev|next commands to visit each numbered or named workspace first before considering workspace from the other category
Latest commit fixes #5783 but workspace {prev,next}_on_output will need the same treatment. Normally I'd make a different PR but it's just easier to have one big i3 compat branch for testing, especially when they might conflict. It should be easy enough to split later if we want. As for firefox, it turns out #5812 was pretty buggy and I think it's related. I'm not sure yet how #5848 and #5857 will be resolved but I have a hunch their closure might address the firefox issue as well, or at least make it easier to debug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of getting more eyes on this and preventing this branch getting stale, I'll go ahead and merge this now. Irrespective of any outstanding bugs this is already a great improvement over the status quo. Thanks for working on this!
EDIT: Hijacking top comment b/c this PR is barely related to what it started as anyway
Hey you. Lurker. Please test.
Fix #5755
This is intended to match i3's behavior here:
https://github.com/i3/i3/blob/3cd1c45eba6de073bc4300eebb4e1cc1a0c4479a/src/tree.c#L354