-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement split none command #4935
Conversation
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.
The workspace needs to be arranged after the flatten. Reference the do_split
function at the top of the file:
Lines 30 to 34 in 8170a23
if (root->fullscreen_global) { | |
arrange_root(); | |
} else { | |
arrange_workspace(ws); | |
} |
Since this is for i3 compatibility, this should not be merged until the feature lands in i3 master. This is especially important since there is no active PR to mirror the behavior against. The linked i3 issue also suggests rejecting the command (CMD_FAILURE) when there are siblings, which isn't currently implemented in this PR.
Just updated since this was stale and had a conflict. No change in the i3 situation AFAIK. |
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.
Code LGTM. Not sure if this should be merged yet.
This comment has been minimized.
This comment has been minimized.
Hey! i3 currently have a workaround for this: https://www.youtube.com/watch?v=AWA8Pl57UBY&t=278s |
@jhonrocha That should be fixed in master. If not, please file a new bug for it. i3 has entered a feature freeze and I'm not sure of the status of this feature for i3. |
Is there something blocking this from being merged? |
The i3 feature is not implemented, so this is blocked on that. |
The risk here is to merge something which becomes incompatible with what i3 ends up merging. |
@emersion I'm curious, why not merge this and then worst case if i3 ever ends up merging something different also add compatibility for that? sway already has features that i3 doesn't have, which risks i3 merging something incompatible with them some day in the future. |
The i3 devs have given a provisional ACK for this API. I'd be fine with merging even if there's a risk we need to add back compat for i3 later. This is a pretty small feature. |
I'm fine with merging this. I think its unlikely for i3 to merge an incompatible feature, and even if they do it's unlikely to be a disaster if we change ours to match. |
Alright, let's do this then, thanks! |
How is this supposed to work? I tried:
But it just didn't change anything. The Version info
|
That sounds right. It will only work on singleton containers though, so if you've spawned another window already it will do nothing. Maybe try running EDIT: btw, |
@rpigott : you're right. If I remove the windows from the container, it seems to work. That's what I was missing I think! |
While browsing the i3 issue tracker I saw issue i3/i3#3808 "split none" command with an accepted label.
Not sure if we want it yet, but I figured this one was pretty easy to implement for sway so here it is. i3 doesn't have an implementation yet, so I can't really compare it to theirs, but I expect this is the intended behavior.