Fix layout when fullscreen #4293

pedrocr · 2019-06-28T22:38:50Z

This is an alternative to #4283 that's implemented using width/height fractions of the parent container. It seems to make the code cleaner, although it also required changing the resize command to match. As far as I can tell everything works fine.

Fixes #3547
Fixes #4297

If there is more than one new window layout correctly by calculating the default size of the new windows using the information of how many of them there are in total. This helps with issue swaywm#3547 but doesn't fix it in all situations. Things now work correctly if the first layout of new windows happens after leaving fullscreen. But if for some reason an arrange_container() gets called while we are fullscreen the windows will still be incorrectly sized after saved_width/saved_height get used to restore the first window's size before going fullscreen.

RyanDwyer · 2019-06-29T12:15:29Z

This isn't quite what I had in mind. I meant setting each weighting value to 1 when the container is created then not changing them until the user resizes them. They would not sum to 1. In all places where we zero the width and height, we would make it reset the weighting to 1 instead. There would then be no reason to have the if (child->width <= 0) branch when arranging, and therefore no reason to count the children to see how many are new.

I consider the above approach to be more of a refactor than a bug fix. A fix should be merged first, then we can look into how well weighting would work and whether it's worth it (that alliteration was not intentional).

pedrocr · 2019-06-29T12:43:24Z

Merging a fix first would be merging #4283. That one is a fix without changing the way the code works. But that's just uglier and it has to manipulate saved_width and saved_height. I see what you mean about simplifying the weights calculation by not doing normalization. That may work with less special cases. I'll give it a try.

pedrocr · 2019-06-29T14:13:04Z

I think this is what you meant. It's indeed much simpler. I started along these lines last night and then for some reason didn't think it was going to work. In my defense it was late and I was tired :)

The diff right now is probably good but the commit sequence arrives at it in a roundabout way. I'll squash/rework stuff into a cleaner history.

pedrocr · 2019-06-30T11:24:05Z

I've condensed it into two commits. The first does the layout changes, the second implements the resize command based on the new layout method. So hopefully this is mergeable now.

pedrocr · 2019-07-05T18:15:31Z

I've just come across a behavior that probably means normalization to the sum being 1.0 is actually needed:

Open two windows
Resize one window to be larger than the other
Close one of the windows
Open a new window

The expected result would be two equally sized windows but because the existing window has a factor that's different than 1.0 the new window will not be half size and can be larger or smaller depending on the remaining factor.

So the previous code is actually the one that retains current behavior while fixing bugs.

pedrocr · 2019-07-05T22:06:35Z

I've pushed the normalizing solution again, as it seems needed to have correct behavior. It may still be possible to make the layout functions simpler but probably not by too much without altering behavior.

RedSoxFan

Style wise, there are several lines that:

are well over 80 columns and could be split
have indentation issues
are not cleanly split across lines

Functionality wise:

floating containers do not work. the views just render on top of each other
if you float and re-tile a floating view a few times, it will continuously get smaller when tiled

sway/commands/resize.c

sway/tree/arrange.c

pedrocr · 2019-07-11T07:39:58Z

Thanks for the review. Do you have an example of what you mean with the floating containers rendering on top of each other? I just tested with GIMP and its floating windows and everything seems to work as expected.

I've pushed a fix for the issue of reducing size on going from floating to tiled.

RedSoxFan · 2019-07-11T07:53:47Z

Do you have an example of what you mean with the floating containers rendering on top of each other?

Float a view, split it (horizontally or vertically, doesn't matter), open another view. Instead of being split, the two views take up the full size of the floating container. Using the opacity command may help visualize it.

pedrocr · 2019-07-11T10:01:14Z

Thanks. I've pushed a fix.

RedSoxFan · 2019-07-11T20:27:07Z

A couple more issues:

If you create H[viewA V[viewB viewC] and move viewA right and left multiple times, it'll continuously shrink.

If you have one output with H[viewA viewB viewC] that are all equal size and move viewC to the output to the right and then move it back, the sizing is viewA 25%, viewB 25%, viewC 50%. Moving viewC further to the left retains the 50% width.

pedrocr · 2019-07-11T22:37:30Z

I'll have a look at these but I don't know what the correct behavior is. This PR sizes things by a fraction of the total container and when a view is coming from somewhere else it's treated as if it's new. This will naturally be different behavior than using the width/height of existing views as the starting point to rearrange the container. So the behavior will never match current behavior in all situations. If that's wanted I did another PR that adjusted saved_width and saved_height as containers got their layout arranged while something is fullscreen. It didn't fix things like #4297 but maybe that's enough of an edge case to ignore and we should be going back to that. It all depends on what the wanted behavior is.

pedrocr · 2019-07-11T22:52:27Z

Both cases are fixed now, thanks again for looking at this. They were just missing fixes in the move command which already setts width/height to 0. So it may very well be that this approach actually does deliver the same behavior as right now.

RedSoxFan

A few line length issues. Other than that, looks good. Can you please edit the original post and add Fixes #xxxx for the issue(s) that this resolves?

sway/commands/resize.c

sway/tree/arrange.c

pedrocr · 2019-07-12T07:56:34Z

I've fixed those now. The commits already have the Fixes #xxxx lines but I've added them to the PR as well.

RedSoxFan

Code LGTM. Can you squash this into a single commit?

pedrocr · 2019-07-13T09:16:32Z

Should it really be a single commit? At least the first commit is independent and the others help explain the changes.

RedSoxFan · 2019-07-14T01:58:56Z

Should it really be a single commit? At least the first commit is independent and the others help explain the changes.

I guess the first commit can remain separate, but the rest of the commits are a logical grouping. Without commits 3-8, commit 2 is not in a usable state and commits 3-8 don't make sense on there own as they are essentially just intermediary fixes for the second commit. Feel free to include as much information as you want in the commit message to explain the changes, but the commit history should be clean and logically grouped together.

pedrocr · 2019-07-14T10:10:27Z

It depends on the project. Some would prefer a sequence of commits that does the work building upon each other.

Instead of using container->width/height as both the input and output of the layout calculation have container->width_fraction/height_fraction as the share of the parent this container occupies and calculate the layout based on that. That way the container arrangement can always be recalculated even if width/height have been altered by things like fullscreen. To do this several parts are reworked: - The vertical and horizontal arrangement code is ajusted to work with fractions instead of directly with width/height - The resize code is then changed to manipulate the fractions when working on tiled containers. - Finally the places that manipulated width/height are adjusted to match. The adjusted parts are container split, swap, and the input seat code. It's possible that some parts of the code are now adjusting width and height only for those to be immediately recalculated. That's harmless and since non-tiled containers are still sized with width/height directly it may avoid breaking other corner cases. Fixes swaywm#3547 Fixes swaywm#4297

RedSoxFan · 2019-07-14T15:13:49Z

It depends on the project. Some would prefer a sequence of commits that does the work building upon each other.

A general rule for sway's commit history is a change log should be able to be generated from the short log. Also, I appreciate the work you are putting into this and not trying to come off rude, but please don't argue sway's conventions by saying what other project use. They are set and are not going to change.

RedSoxFan · 2019-07-14T15:14:03Z

Thanks!

pedrocr · 2019-07-14T15:15:52Z

Thanks for the patience to review it! I'll get the gaps PR ready based on this.

pedrocr mentioned this pull request Jun 28, 2019

Layout correctly with several new windows #4283

Closed

pedrocr force-pushed the layout_improvement branch 2 times, most recently from 06e5343 to f36e925 Compare June 30, 2019 11:22

pedrocr force-pushed the layout_improvement branch from f36e925 to 64fb008 Compare June 30, 2019 23:34

pedrocr mentioned this pull request Jul 5, 2019

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

Closed

pedrocr force-pushed the layout_improvement branch from 64fb008 to 11648aa Compare July 5, 2019 22:03

pedrocr force-pushed the layout_improvement branch from 11648aa to 6c817ea Compare July 6, 2019 12:11

This was referenced Jul 6, 2019

Don't manipulate size/pos of fullscreen containers #4284

Closed

Rework the gaps code to make it simpler and correct #4310

Merged

RedSoxFan requested changes Jul 11, 2019

View reviewed changes

sway/commands/resize.c Outdated Show resolved Hide resolved

sway/commands/resize.c Outdated Show resolved Hide resolved

sway/commands/resize.c Outdated Show resolved Hide resolved

sway/tree/arrange.c Show resolved Hide resolved

sway/tree/arrange.c Show resolved Hide resolved

pedrocr force-pushed the layout_improvement branch 2 times, most recently from d4902f4 to 885c017 Compare July 11, 2019 07:52

pedrocr force-pushed the layout_improvement branch from 885c017 to e973ca7 Compare July 11, 2019 10:00

pedrocr force-pushed the layout_improvement branch from e973ca7 to 8b04f73 Compare July 11, 2019 22:51

RedSoxFan requested changes Jul 11, 2019

View reviewed changes

sway/commands/resize.c Outdated Show resolved Hide resolved

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

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

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

pedrocr force-pushed the layout_improvement branch from 8b04f73 to c10e9d6 Compare July 12, 2019 07:52

pedrocr force-pushed the layout_improvement branch 2 times, most recently from b4f15c1 to 13b0188 Compare July 12, 2019 08:03

RedSoxFan reviewed Jul 13, 2019

View reviewed changes

pedrocr force-pushed the layout_improvement branch from 13b0188 to a4ca232 Compare July 14, 2019 10:18

RedSoxFan approved these changes Jul 14, 2019

View reviewed changes

RedSoxFan merged commit e3a3917 into swaywm:master Jul 14, 2019

RedSoxFan mentioned this pull request Jul 15, 2019

Using split in fullscreen causes odd behavior #4332

Closed

norcalli mentioned this pull request Jul 27, 2019

Resize broken for tiling windows #4386

Closed

sebastiancarlos mentioned this pull request Oct 23, 2023

Allow extremely small tiling windows. #7791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix layout when fullscreen #4293

Fix layout when fullscreen #4293

pedrocr commented Jun 28, 2019 •

edited

Loading

RyanDwyer commented Jun 29, 2019

pedrocr commented Jun 29, 2019

pedrocr commented Jun 29, 2019

pedrocr commented Jun 30, 2019

pedrocr commented Jul 5, 2019

pedrocr commented Jul 5, 2019

RedSoxFan left a comment

pedrocr commented Jul 11, 2019

RedSoxFan commented Jul 11, 2019

pedrocr commented Jul 11, 2019

RedSoxFan commented Jul 11, 2019

pedrocr commented Jul 11, 2019

pedrocr commented Jul 11, 2019 •

edited

Loading

RedSoxFan left a comment

pedrocr commented Jul 12, 2019

RedSoxFan left a comment

pedrocr commented Jul 13, 2019

RedSoxFan commented Jul 14, 2019

pedrocr commented Jul 14, 2019

RedSoxFan commented Jul 14, 2019

RedSoxFan commented Jul 14, 2019

pedrocr commented Jul 14, 2019

Fix layout when fullscreen #4293

Fix layout when fullscreen #4293

Conversation

pedrocr commented Jun 28, 2019 • edited Loading

RyanDwyer commented Jun 29, 2019

pedrocr commented Jun 29, 2019

pedrocr commented Jun 29, 2019

pedrocr commented Jun 30, 2019

pedrocr commented Jul 5, 2019

pedrocr commented Jul 5, 2019

RedSoxFan left a comment

Choose a reason for hiding this comment

pedrocr commented Jul 11, 2019

RedSoxFan commented Jul 11, 2019

pedrocr commented Jul 11, 2019

RedSoxFan commented Jul 11, 2019

pedrocr commented Jul 11, 2019

pedrocr commented Jul 11, 2019 • edited Loading

RedSoxFan left a comment

Choose a reason for hiding this comment

pedrocr commented Jul 12, 2019

RedSoxFan left a comment

Choose a reason for hiding this comment

pedrocr commented Jul 13, 2019

RedSoxFan commented Jul 14, 2019

pedrocr commented Jul 14, 2019

RedSoxFan commented Jul 14, 2019

RedSoxFan commented Jul 14, 2019

pedrocr commented Jul 14, 2019

pedrocr commented Jun 28, 2019 •

edited

Loading

pedrocr commented Jul 11, 2019 •

edited

Loading