implement-unsplit into main fixes #19
still needs a bunch of cleanup, maybe tests.
@edmonds does this look like it'd need any special handling of placeholder containers? I didn't completely follow the expected invariants there.
b7b155e68e
to cee35c7c92
cee35c7c92
to ab009f50ae
ab009f50ae
to b6e38bd4e9
@ -39,0 +46,4 @@
for (int i = 0; i < siblings->length; i++) {
if (siblings->items[i] == current) {
if (i == 0) {
return siblings->items[1];
Hm, can siblings->length be 1? If so this looks like it accesses one past the end of the siblings list.
Good eye, indeed the case where you'd 'unsplit' a workspace that wasn't split wasn't handled yet. Added a check.
@ -21,2 +21,4 @@
'-Wundef',
'-Wvla',
'-g',
Meson has a dedicated flag to enable debugging symbols, but it should be enabled by default already, typically you wouldn't set -g by hand.
Oops, didn't mean to commit that :)
b6e38bd4e9
to db1f473fce
@edmonds does this look like it'd need any special handling of placeholder containers? I didn't completely follow the expected invariants there.
Looks like you're using container_insert_child() which already takes care of dealing with the placeholder (aac3fdad17). I'll take a more in-depth look later.
db1f473fce
to d6b283d27b
d6b283d27b
to c335e3b94c
c335e3b94c
to 865506706d
Looks like sway uses tabs for indentation but the lines you added to this file are indented with four spaces, can you convert to tabs to keep the code base consistent?
@ -47,0 +92,4 @@
struct sway_container *parent = con->pending.parent;
struct sway_container *child = parent->pending.children->items[0];
while (parent->pending.children->length) {
container_insert_child(destination, child, destination->pending.children->length);
Here you're inserting at the end of the list, which is the same thing as adding (appending), so this can just be:
container_add_child(destination, child);
nice
@ -47,0 +91,4 @@
}
struct sway_container *parent = con->pending.parent;
struct sway_container *child = parent->pending.children->items[0];
while (parent->pending.children->length) {
If you're unsplitting an empty container you might pick up a placeholder container and put it into the destination container which isn't correct if the destination container already has (non-placeholder) containers in it.
I was able to unsplit this:
Workspace 1
Output: DP-1
Layout: splith
Representation: H[H[V[T[Alacritty] T[<empty>]] T[<empty>]]]
Into this:
Workspace 1
Output: DP-1
Layout: splith
Representation: H[H[T[Alacritty <empty>] T[<empty>]]]
It looks like V[T[Alacritty] T[<empty>]] became T[Alacritty <empty>] after unsplitting.
If a container is an empty tabbed container (no application windows inside it), it must have a placeholder <empty> container as the first and only container in the L_TABBED container. Conversely a non-empty tabbed container (has application windows inside it) must not have any placeholder containers in it.
So e.g. if you're joining (unsplitting) T[Alacritty] and T[<empty>] that should result in T[Alacritty], not T[Alacritty <empty>].
So this loop should probably be something like this:
while (parent->pending.children->length) {
if (child->is_placeholder) {
container_begin_destroy(child);
} else {
container_add_child(destination, child);
child->pending.width = child->pending.height = 0;
child->width_fraction = child->height_fraction = 0;
}
child = parent->pending.children->items[0];
}
Or maybe:
if (child->is_placeholder) {
container_begin_destroy(child);
} else {
while (parent->pending.children->length) {
container_add_child(destination, child);
child->pending.width = child->pending.height = 0;
child->width_fraction = child->height_fraction = 0;
child = parent->pending.children->items[0];
}
}
I gave it some light testing and found/fixed a bug (see in-line comment) and it seems to be working. I was never a big user of the Notion unsplit command, but it seems to be doing what it should be, intuitively.
The TODO that you have marked ("create a target frame if we can find none") doesn't quite make sense to me. When I was able to trigger the "No destination frame found" message it seemed to be in situations where the unsplit command shouldn't do anything, like if you already have a single tabbed container taking up the entire workspace and there's nothing obvious to unsplit it with, or trying to unsplit a floating window. (Maybe unsplitting a floating window should be treated the same as toggling floating mode on that window.) But maybe it also triggers in other situations?
Unsplitting a fullscreen window seems to behave as if the window wasn't fullscreened, i.e. when i exited fullscreen mode the layout had been rearranged as if I hadn't been in fullscreen mode in the first place. Maybe that makes sense, but maybe there's an argument that unsplitting shouldn't occur in fullscreen mode since you can't immediately see the result of the action.
Thanks for the review!
Looks like sway uses tabs for indentation but the lines you added to this file are indented with four spaces, can you convert to tabs to keep the code base consistent?
Yeah, we should add this to PR validation, I bet this won't be the last time I forget :)
found/fixed a bug
Thanks!
maybe there's an argument that unsplitting shouldn't occur in fullscreen mode since you can't immediately see the result of the action
Good idea
maybe there's an argument that unsplitting shouldn't occur in fullscreen mode since you can't immediately see the result of the action
Hmm, you can currently 'split' in fullscreen mode, so perhaps it makes sense that you can also 'unsplit'. Maybe let's leave it as is for now and revisit in the future.
865506706d
to 2e5ba55fdb
@ -47,0 +86,4 @@
struct sway_container *destination = find_unsplit_destination(current, ws, ancestor);
if (!destination) {
return cmd_results_new(CMD_FAILURE, "No destination frame found");
Not clear to me that this should be a hard failure. Also, should probably use sway terminology here, "container" rather than "frame". Maybe something like:
sway_log(SWAY_DEBUG, "No destination container found for unsplit");
return cmd_results_new(CMD_SUCCESS, NULL);
@ -47,0 +94,4 @@
if (child->is_placeholder) {
container_begin_destroy(child);
} else {
container_insert_child(destination, child, destination->pending.children->length);
Use container_add_child() here
2e5ba55fdb
c843ff6f36
@ -47,0 +87,4 @@
struct sway_container *destination = find_unsplit_destination(current, ws, ancestor);
if (!destination) {
sway_log(SWAY_DEBUG, "No destination container found for unsplit");
return cmd_results_new(CMD_SUCCESS, NULL);
Whitespace :-)
c843ff6f36
7d0b24756f
No due date set.
No dependencies set.
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?