[WIP] gaps: fixes for edge cases & better alignment with i3-gaps syntax #2131
Conversation
sway/tree/arrange.c
Outdated
| @@ -268,6 +284,10 @@ void arrange_children_of(struct sway_container *parent) { | |||
| if (config->reloading) { | |||
| return; | |||
| } | |||
| if (parent->type == C_OUTPUT) { | |||
| arrange_output(parent); | |||
There was a problem hiding this comment.
The indentation is a bit weird here.
Why do we need that?
sway/tree/arrange.c
Outdated
| @@ -284,9 +304,27 @@ void arrange_children_of(struct sway_container *parent) { | |||
| return; | |||
| } | |||
|
|
|||
| if (config->smart_gaps && workspace->current_gaps != (workspace->children->length <= 1 ? 0 : 1)) { | |||
| if (workspace->current_gaps == 0) { | |||
| container_damage_whole(workspace); | |||
There was a problem hiding this comment.
If smart gaps are on and a second window is added to a workspace, damage gets left in the outer borders. This clears it.
There was a problem hiding this comment.
TBH I don't think this is the right fix. We should just damage before and after arranging. We only damage after right now.
There was a problem hiding this comment.
The problem is that adding a container (by opening a terminal for example) to a workspace with a single container already in it only arranges the new container. If smart_gaps were on in this situation, the workspace would need to apply outer gaps.
There was a problem hiding this comment.
Keep in mind there's going to be some significant changes once the atomic layout updates PR is completed and merged. The calls to add damage in response to layout changes will all be removed and done in a single place when the layout is applied. For that reason you might not want to try too hard to get it perfect here.
sway/tree/arrange.c
Outdated
| printf("Child %p, %d\n", c->children->items[i], ((struct sway_container *)c->children->items[i])->type); | ||
| } | ||
| if (!config->edge_gaps || (config->smart_gaps && c->children->length == 1)) { | ||
| printf("SKIPPING OUTER GAPS %s\n", c->name ? c->name : "UNKNOWN NAME"); |
There was a problem hiding this comment.
If you want to log something, use wlr_log (and don't use caps lock!)
sway/commands/gaps.c
Outdated
| } else if (strcmp(argv[amount_idx], "current") == 0) { | ||
| amount_idx++; | ||
| scope = GAPS_SCOPE_CURRENT; | ||
| } else { | ||
| return cmd_results_new(CMD_INVALID, "gaps", "gaps inner|outer all|current|current_container set|plus|minus <amount>"); |
There was a problem hiding this comment.
Seems like there's a typo in here
sway/commands/gaps.c
Outdated
| @@ -104,6 +102,8 @@ struct cmd_results *cmd_gaps(int argc, char **argv) { | |||
| } else if (strcmp(argv[amount_idx], "minus") == 0) { | |||
| amount_idx++; | |||
| op = GAPS_OP_SUBTRACT; | |||
| } else { | |||
| return cmd_results_new(CMD_INVALID, "gaps", "gaps inner|outer all|current|current_container set|plus|minus <amount>"); | |||
sway/commands/gaps.c
Outdated
| GAPS_SCOPE_WORKSPACE, | ||
| GAPS_SCOPE_CURRENT | ||
| GAPS_SCOPE_ALL, // GLOBAL | ||
| GAPS_SCOPE_CURRENT // CURRENT WORKSPACE |
There was a problem hiding this comment.
I don't think these comments are needed
sway/commands/gaps.c
Outdated
| @@ -120,7 +120,7 @@ struct cmd_results *cmd_gaps(int argc, char **argv) { | |||
| "gaps inner|outer <amount>"); | |||
| } | |||
| return cmd_results_new(CMD_INVALID, "gaps", | |||
| "gaps inner|outer all|workspace|current set|plus|minus <amount>"); | |||
| "gaps inner|outer all|current|current_container set|plus|minus <amount>"); | |||
sway/commands/gaps.c
Outdated
| if (c->parent) { | ||
| arrange_children_of(c->parent); | ||
| } else { | ||
| arrange_root(); |
There was a problem hiding this comment.
If c doesn't have a parent, maybe we should do nothing (arranging root won't do anything interesting since c is out of the tree)
natesymer
force-pushed
the
gaps_improvements
branch
3 times, most recently
from
e2d6ac9
to
e094429
Compare
sway/commands/gaps.c
Outdated
| arrange_children_of(c->parent); | ||
| } else { | ||
| switch (scope) { | ||
| case GAPS_SCOPE_ALL: { |
There was a problem hiding this comment.
Style: don't intend switch labels
switch (scope) {
case GAPS_SCOPE_ALL:
…
sway/commands/gaps.c
Outdated
| break; | ||
| } | ||
| default: { | ||
| return cmd_results_new(CMD_INVALID, "gaps", "gaps inner|outer all|current set|plus|minus <amount>"); |
There was a problem hiding this comment.
This could probably use one of the #defines?
|
|
||
| for (int i = 0; i < ws->children->length; i++) { | ||
| struct sway_container *sibling = cont->parent->children->items[i]; | ||
| if (sibling == cont) continue; |
| } | ||
| gaps = c->has_gaps ? c->gaps_outer : config->gaps_outer; | ||
| } else { | ||
| if (config->smart_gaps && container_parent(c, C_WORKSPACE)->children->length < 2) { |
There was a problem hiding this comment.
Does config->edge_gaps need to be checked here?
There was a problem hiding this comment.
Edge gaps must be enabled to have outer gaps. If disabled, workspaces have no gaps because workspaces only have outer gaps.
There was a problem hiding this comment.
Ergo edge_gaps must be checked. It's cheap though.
|
There's currently a bug where a single C_CONTAINER container with multiple children would not get gapped under smart gaps. I've seen your comments, and I'm working on both. |
emersion
changed the title
natesymer
force-pushed
the
gaps_improvements
branch
from
25e00e6
to
8fdf85f
Compare
My previous PR (#2047) had a few bugs with edge cases. This PR is designed to fix those bugs, as well as provide more compatibility with the i3-gaps syntax (specifically the longest gaps command's syntax & semantics).