Rework the gaps code so sizes are correct and to prevent broken layouts #4298

pedrocr · 2019-06-30T23:40:08Z

Fixes for issues #4296 and #4294. They're built on top of each other because they're touching the same code and the reworking of the calculation to make the gaps even makes it easier to then write the code to cap the gaps.

The weird addition of #include "pango.h" in sway/tree/workspace.c was just to get MIN() and MAX(). This is probably not ok and sway should instead include its own definitions of these macros.

--
Fixes #4294
Fixes #4296
Fixes #4304

To make sure inner gaps don't generate unequally sized windows always apply a total of one inner gap to all children. Doing just this would result in half-gaps around the workspace border so compensate for that by adding gaps to the workspace. Fixes swaywm#4296

RedSoxFan

The weird addition of #include "pango.h" in sway/tree/workspace.c was just to get MIN() and MAX().

Considering that these are not defined in pango.h, but one of the headers included in that file, I'm not a fan of this approach.

Another thing to consider is making it so the gap clamping considers both width and height so that they are equal in both directions. I'm not sure which method is better. Thoughts?

It should also be noted in sway.5 that the inner gaps may be smaller or omitted entirely when there isn't enough space available.

sway/tree/container.c

pedrocr · 2019-07-02T11:30:40Z

Considering that these are not defined in pango.h, but one of the headers included in that file, I'm not a fan of this approach.

Definitely. My next sentence was exactly that sway should include its own macros. pango.h was just a placeholder, that reused what container.c already did. Please let me know where in the sway tree it makes sense to include those macros.

Another thing to consider is making it so the gap clamping considers both width and height so that they are equal in both directions. I'm not sure which method is better. Thoughts?

Whenever the gaps get clamped the result is always going to be a bit odd and this should only really happen in extreme cases. But I think this way should work better. Right now if the window doesn't have enough space horizontally but does vertically the vertical gaps to the other containers are all correct. The way you suggest would result in unequal vertical gaps even though the window fits fine vertically.

It should also be noted in sway.5 that the inner gaps may be smaller or omitted entirely when there isn't enough space available.

Makes sense, I'll add that. I reused the sane width/height values from the resize command. I don't know if those values should be placed somewhere global in the code and possibly also referred to in the documentation somewhere.

pedrocr · 2019-07-02T13:27:46Z

I've moved the macros and the static values to include/util.h and added a note to the manpage.

sway/tree/container.c

include/util.h

sway/tree/workspace.c

Large inner gaps can lead to broken rendering with width/height going negative. Limit the gaps so that they never make the windows smaller than a minimum sensible size (the same used for the resize command). Fixes swaywm#4294

Just like with inner gaps, outer gaps must be capped so that the workspace doesn't run out of space. Since here we have different gaps in all four directions clamp them in a way that at least maintains the relative sizing of the gaps vertically and horizontally. Fixes swaywm#4300

pedrocr · 2019-07-04T18:28:48Z

I've fixed all the issues and reimplemented the workspace gaps handling. The code is now correct as it clamps gaps after both outer and inner gaps are applied. Only when we have a total gap (and outer gaps can supposedly be negative) can we then clamp with the correct logic. The code also becomes simpler that way.

pedrocr · 2019-07-04T18:54:58Z

I've found another bug with negative outer gaps filed as #4304 and added a fix for that as well.

RedSoxFan

A few small tweaks and this will be good to merge

RedSoxFan · 2019-07-05T02:18:22Z

sway/tree/container.c

-	c->current_gaps.left = c->x == ws->x ? ws->gaps_inner : 0;
+	int gaps_horizontal = MAX(0, MIN(ws->gaps_inner, c->width - MIN_SANE_W));
+	int gaps_horizontal1 = gaps_horizontal / 2;
+	int gaps_horizontal2 = gaps_horizontal - gaps_horizontal1;


I understand the idea of making this mirror the workspace gaps section, but I think the cleanness of the following outweighs it.

int gaps_horizontal = MAX(0, MIN(ws->gaps_inner, c->width - MIN_SANE_W)); c->current_gaps.left = gaps_horizontal / 2; c->current_gaps.right = gaps_horizontal - c->current_gaps.left;

I had that like that once upon a time :)

RedSoxFan · 2019-07-05T02:20:31Z

sway/tree/workspace.c

+		// to match the half gaps that the children have all already added around
+		// themselves. We use the opposite order for the two halves so that the
+		// sum adds up to the correct total gap size in all circumstances.
+		int gaps_horizontal1 = ws->gaps_inner / 2;


I think the following would be cleaner:

int inner_gaps_left = ws->gaps_inner / 2; ws->current_gaps.left += inner_gaps_left; ws->current_gaps.right = ws->gaps_inner - inner_gaps_left;

RedSoxFan · 2019-07-05T02:35:53Z

sway/tree/workspace.c

+	// inner gaps.
+	int mingaps = ws->layout == L_TABBED ||
+	              ws->layout == L_STACKED ? 0 : -ws->gaps_inner;
+	ws->current_gaps.left = MAX(mingaps, ws->current_gaps.left);


I don't think this is right. The outer gaps should be able to negate the inner gaps for that side, but this would allow for it to negate double the inner gaps. I think these should all be like ws->current_gaps.left = MAX(0, ws->current_gaps.left).

It looks like this function was just missing a call to prevent_invalid_outer_gaps in between setting the outer gaps and inner gaps.

How do you get a double negation? I've gotten rid of prevent_invalid_outer_gaps completely, it doesn't seem workable to cover all cases with that. It just does that calculation on apply instead. So it allows the outer gaps to at most be -ws->gaps_inner for tiled containers to then allow those to be negated by the child gaps. It also seems to make more sense to not manipulate the values the user actually sets himself as the workspace may change sizes by moving from one output to another.

This reminds me of another corner case. If the gaps are large enough that the gap clamping will apply in the children the negative gap will not be properly negated... That doesn't seem trivial to fix.

How do you get a double negation?

Any values where outer == -2 * inner should work, but the following should make it easily visible

swaymsg -- gaps outer all set -400, gaps inner all set 200

So it allows the outer gaps to at most be -ws->gaps_inner

Currently, it is being checked after inner gaps are calculated so it actually makes sure that all gaps are at most -ws->gaps_inner, which is incorrect.

This reminds me of another corner case. If the gaps are large enough that the gap clamping will apply in the children the negative gap will not be properly negated

Can you try explaining that further? When would that occur?

Currently, it is being checked after inner gaps are calculated so it actually makes sure that all gaps are at most -ws->gaps_inner, which is incorrect.

It's correct because the children will then apply ws->gaps_inner themselves and thus negate that. That's why I only do it for tiled children, as for stacked/tabbed we apply the inner gaps right here so clamping to 0 is correct.

Can you try explaining that further? When would that occur?

Set outer gaps to -1000 and inner gaps to +1000. Now start opening windows. At first it's correct with no outer gaps but gaps within windows. As you open more windows and the inner gaps get clamped the windows start falling outside the screen. This is going to be annoying to fix...

It's still wrong though, because clamping needs to be to half size only per edge...

But this doesn't work generally because of the other bug. I'll have to think of a better solution.

I've pushed a fix for the half-sized per edge, but this is not mergeable like this because of the other bug. It also doesn't even work always like this because since after the gap clamping it can be negative, the code that adjusts outer gaps can return broken results.

Since outer gaps can be negative we need to robustly prevent the resulting gaps from ending up negative. The previous code only did this on workspace creation but the gaps can change after that. Fixes swaywm#4304

pedrocr · 2019-07-05T12:36:07Z

I think I have a plan to make everything work but it's a completely different solution from this. It would go something like:

The workspace gaps code would add the full gap around the screen (outer and inner) and do the clamping calculations for those. That fixes Negative outer gaps break workspaces #4304 and this new issue I pointed out.
The container gaps code would be integrated with the tiling layout code and would only add the gaps between containers. The layout code has all the information needed to do proper clamping and adjusting of sizes between containers to fix Very large inner gaps break layout #4294 and Gaps create unequally sized windows #4296.

I haven't written the code yet but it only makes sense to do that on top of #4293.

pedrocr · 2019-07-06T12:25:59Z

I've implemented the plan on top of #4293 and it worked really well:

pedrocr/sway@layout_improvement...pedrocr:gaps_rework

The code ends up simpler and easier to reason about. And even after the clamping code it's a net reduction in code:

$ git diff layout_improvement gaps_rework | diffstat
 include/sway/tree/container.h |   12 -------
 include/sway/tree/node.h      |    3 +
 include/util.h                |    5 +++
 sway/commands/move.c          |    1 
 sway/commands/resize.c        |    2 -
 sway/tree/arrange.c           |   38 +++++++++++++++--------
 sway/tree/container.c         |   67 ------------------------------------------
 sway/tree/view.c              |   16 ++--------
 sway/tree/workspace.c         |   52 ++++++++++++++------------------
 9 files changed, 60 insertions(+), 136 deletions(-)

pedrocr · 2019-07-06T12:34:51Z

Closed in favor of #4310

pedrocr changed the title ~~Rework the inner gaps code so it's more correct and prevents broken layouts~~ Rework the inner gaps code so sizes are correct and to prevent broken layouts Jul 1, 2019

pedrocr force-pushed the gaps_improvement branch from f9caba9 to ca44661 Compare July 1, 2019 00:30

RedSoxFan reviewed Jul 2, 2019

View reviewed changes

sway/tree/container.c Outdated Show resolved Hide resolved

sway/tree/container.c Outdated Show resolved Hide resolved

pedrocr force-pushed the gaps_improvement branch 2 times, most recently from 4fd592a to 3b738b7 Compare July 2, 2019 13:24

pedrocr changed the title ~~Rework the inner gaps code so sizes are correct and to prevent broken layouts~~ Rework the gaps code so sizes are correct and to prevent broken layouts Jul 2, 2019

pedrocr force-pushed the gaps_improvement branch from bd27823 to 765b50f Compare July 2, 2019 14:09

RedSoxFan requested changes Jul 4, 2019

View reviewed changes

sway/tree/container.c Outdated Show resolved Hide resolved

include/util.h Outdated Show resolved Hide resolved

sway/tree/workspace.c Outdated Show resolved Hide resolved

sway/tree/workspace.c Outdated Show resolved Hide resolved

pedrocr added 2 commits July 4, 2019 19:22

Limit inner gaps to sensible values

b589767

Large inner gaps can lead to broken rendering with width/height going negative. Limit the gaps so that they never make the windows smaller than a minimum sensible size (the same used for the resize command). Fixes swaywm#4294

pedrocr force-pushed the gaps_improvement branch from 765b50f to 9cbf3be Compare July 4, 2019 18:24

pedrocr force-pushed the gaps_improvement branch from e43469d to c316355 Compare July 4, 2019 18:52

pedrocr force-pushed the gaps_improvement branch from c316355 to 6028829 Compare July 4, 2019 19:23

RedSoxFan reviewed Jul 5, 2019

View reviewed changes

pedrocr force-pushed the gaps_improvement branch 2 times, most recently from a0ed94f to 7df7c37 Compare July 5, 2019 09:49

Prevent workspace gaps from ending up negative

f66eb11

Since outer gaps can be negative we need to robustly prevent the resulting gaps from ending up negative. The previous code only did this on workspace creation but the gaps can change after that. Fixes swaywm#4304

pedrocr force-pushed the gaps_improvement branch from 7df7c37 to f66eb11 Compare July 5, 2019 09:56

pedrocr closed this Jul 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the gaps code so sizes are correct and to prevent broken layouts #4298

Rework the gaps code so sizes are correct and to prevent broken layouts #4298

pedrocr commented Jun 30, 2019 •

edited by RedSoxFan

Loading

RedSoxFan left a comment

pedrocr commented Jul 2, 2019

pedrocr commented Jul 2, 2019

pedrocr commented Jul 4, 2019

pedrocr commented Jul 4, 2019

RedSoxFan left a comment

RedSoxFan Jul 5, 2019

pedrocr Jul 5, 2019

RedSoxFan Jul 5, 2019

RedSoxFan Jul 5, 2019

pedrocr Jul 5, 2019

RedSoxFan Jul 5, 2019 •

edited

Loading

pedrocr Jul 5, 2019 •

edited

Loading

pedrocr Jul 5, 2019

pedrocr Jul 5, 2019

pedrocr commented Jul 5, 2019 •

edited

Loading

pedrocr commented Jul 6, 2019

pedrocr commented Jul 6, 2019

Rework the gaps code so sizes are correct and to prevent broken layouts #4298

Rework the gaps code so sizes are correct and to prevent broken layouts #4298

Conversation

pedrocr commented Jun 30, 2019 • edited by RedSoxFan Loading

RedSoxFan left a comment

Choose a reason for hiding this comment

pedrocr commented Jul 2, 2019

pedrocr commented Jul 2, 2019

pedrocr commented Jul 4, 2019

pedrocr commented Jul 4, 2019

RedSoxFan left a comment

Choose a reason for hiding this comment

RedSoxFan Jul 5, 2019

Choose a reason for hiding this comment

pedrocr Jul 5, 2019

Choose a reason for hiding this comment

RedSoxFan Jul 5, 2019

Choose a reason for hiding this comment

RedSoxFan Jul 5, 2019

Choose a reason for hiding this comment

pedrocr Jul 5, 2019

Choose a reason for hiding this comment

RedSoxFan Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

pedrocr Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

pedrocr Jul 5, 2019

Choose a reason for hiding this comment

pedrocr Jul 5, 2019

Choose a reason for hiding this comment

pedrocr commented Jul 5, 2019 • edited Loading

pedrocr commented Jul 6, 2019

pedrocr commented Jul 6, 2019

pedrocr commented Jun 30, 2019 •

edited by RedSoxFan

Loading

RedSoxFan Jul 5, 2019 •

edited

Loading

pedrocr Jul 5, 2019 •

edited

Loading

pedrocr commented Jul 5, 2019 •

edited

Loading