Add Awesome/Monad style automatic layouts to Sway #1024

willakat · 2017-01-01T15:35:28Z

Hi all,

I've been wanting to try out a Wayland setup and as an Awesome (and ex-i3) user, I gave Sway a spin. I quickly missed the (in my opinion) easy way with which Awesome just sorts out all the nitty-gritty of laying out your windows for you with a few simple pre-defined layouts that usually cover my limited needs. Anyway, long story short, I added the feature to Sway and find it quite pleasant to use. I took the opportunity to refactor some of the layout code. I'll probably add an includable config file to provide a setup that roughly matches Awesome's.

Enjoy!

- added L_AUTO_FIRST/LAST instead of using explicit layouts. - when switching between auto layout that don't share the same major axis, invert the width/height of their child views to preserve their relative proportions.

Hummer12007 · 2017-01-01T15:54:25Z

include/sway/layout.h

@@ -75,4 +75,7 @@ void swayc_log(log_importance_t verbosity, swayc_t *cont, const char* format, ..
 */
 enum swayc_layouts default_layout(swayc_t *output);

+inline bool is_auto_layout(enum swayc_layouts layout) {


Needs to be static inline (GNU semantics).

Needs to move out of the header and into a file (no code in headers, just don't make it inline)

Hummer12007 · 2017-01-01T16:01:37Z

README.md

@@ -10,6 +11,10 @@ irc.freenode.net).

 [More screenshots](https://github.com/SirCmpwn/sway/wiki/Screenshots-of-Sway)

+This fork attempts to add


Yeah just revert all of the readme changes

ddevault · 2017-01-01T17:09:34Z

sway/commands/layout.c

+					      EXPECTED_EQUAL_TO, 2))) {
+				return error;
+			}
+			int inc = (int) strtol(argv[1], NULL, 10);


Can you add error handling for invalid ints here?

char *end; int inc = (int)strtol(argv[1], &end, 10); if (*end) { // error }

Also note no space between (int) and strtol

ddevault · 2017-01-01T17:10:38Z

sway/commands/layout.c

+			enum swayc_layouts layout;
+			if (strcasecmp(argv[1], "next") == 0) {
+				if (is_auto_layout(parent->layout) && parent->layout < L_AUTO_LAST) {
+					layout = parent->layout + 1;


Can you add another function somewhere that uses a switch case or something, given a layout, to find the next auto layout? I'd rather not rely on the order of and value of the enum values.

Nevermind, let's not

ddevault · 2017-01-01T17:11:09Z

sway/commands/layout.c

+						       "Must be one of <prev|next>.");
+			}
+			swayc_change_layout(parent, layout);
+		} else if (strcasecmp(argv[0], "promote") == 0) {


Would prefer this as move promote

renamed to "move first" (I actually had this change pending ... so I completely agree!)

ddevault · 2017-01-01T17:13:06Z

sway/commands/resize.c

+ */
+static int group_start_index(swayc_t *container, int index) {
+	if ((index < 0) || (! is_auto_layout(container->layout)) ||
+	    ((uint_fast32_t) index < container->nb_master)) {


Weird indent here, also weird extra parenthesis

if (index < 0 || !is_auto_layout(container->layout) || (size_t)index < container->nb_master)) { }

trying out "spacemacs", haven't yet looked into having it format using tabs only. For now fixed it up manually.

ddevault · 2017-01-01T17:13:27Z

sway/commands/resize.c

+		return 0;
+	} else {
+		uint_fast32_t grp_sz = slave_count(container) / container->nb_slave_groups;
+		uint_fast32_t remainder = slave_count(container) % container->nb_slave_groups;


Use size_t instead of uint_fast32_t

ddevault · 2017-01-01T17:15:09Z

sway/commands/resize.c

+		return container->children->length;
+	} else {
+		int nxt_idx;
+		if ((uint_fast32_t)index < container->nb_master) {


That goes for everywhere, s/uint_fast32_t/size_t/g

ddevault · 2017-01-01T17:15:40Z

sway/commands/resize.c

+ * return the index of the Group containing <index>th child of <container>.
+ * The index is the order of the group along the container's major axis (starting at 0).
+ */
+static uint_fast32_t group_index(swayc_t *container, int index) {


Can you prefix all of these auto related functions with auto_?

ddevault · 2017-01-01T17:16:32Z

sway/commands/resize.c

+		parent = container->parent;
+		if ((parent->children && parent->children->length > 1) &&
+		    (is_auto_layout(parent->layout) || (use_width ? parent->layout == L_HORIZ :
+							parent->layout == L_VERT))) {


More weird indentation, use tabs

ddevault · 2017-01-01T17:18:02Z

sway/commands/resize.c

-		return true;
+		// Recursive resize does not handle positions, let arrange_windows
+		// take care of that.
+		arrange_windows(swayc_active_workspace(), -1, -1);


This whole function has now become pretty complicated. Can you split it up into several?

on todo list...

ddevault · 2017-01-01T17:18:44Z

sway/container.c

+	// if layout change modifies the auto layout's major axis, swap width and height
+	// to preserve current ratios.
+	if (is_auto_layout(layout) && is_auto_layout(container->layout)) {
+		enum swayc_layouts prev_major = (container->layout == L_AUTO_LEFT ||


Unnecessary parens

ddevault · 2017-01-01T17:19:20Z

sway/layout.c

@@ -256,7 +260,7 @@ void move_container(swayc_t *container, enum movement_direction dir) {
 		layout = L_VERT;
 	} else if (dir == MOVE_LEFT || dir == MOVE_RIGHT) {
 		layout = L_HORIZ;
-	} else {
+	} else if (! (dir == MOVE_NEXT || dir == MOVE_PREV)) {


No space after !

ddevault · 2017-01-01T17:20:02Z

sway/layout.c

@@ -279,19 +283,53 @@ void move_container(swayc_t *container, enum movement_direction dir) {
 		sway_log(L_DEBUG, "container:%p, parent:%p, child %p,",
 				container,parent,child);
 		if (parent->layout == layout
+			|| layout == L_NONE /* accept any layout for next/prev direction */


This won't accept any layout, right? It'll accept no layout, which isn't a thing that happens in practice (things like views have no layout)

"layout == L_NONE" was to broad, replaced by "layout == L_NONE && parent->type == C_CONTAINER". This is intended to accept the MOVE_PREV/MOVE_NEXT directions regardless of the actual container layout.

ddevault · 2017-01-01T17:21:06Z

sway/layout.c

+static void apply_auto_layout(swayc_t *container, const double x, const double y,
+			      const double width, const double height,
+			      enum swayc_layouts group_layout,
+			      bool master_first);


Please make the indentation on these consistent

attempted to make things more consistent (used tabs exclusively)

grepped through the code for mixed tab/space indents and removed the ones I had put in.

ddevault · 2017-01-01T17:22:37Z

sway/layout.c

+
+	switch(group_layout) {
+	default:
+		sway_log(L_ERROR, "Unknown layout type (%d) used in %s()",


Make this L_DEBUG. It's programmer error, right? Not user error? And is a recoverable condition?

ddevault · 2017-01-01T17:22:49Z

sway/layout.c

+	// height and width of next group to be laid out.
+	const double *group_h, *group_w;
+
+	switch(group_layout) {


Space after switch

ddevault · 2017-01-01T17:23:46Z

sway/layout.c

 		}
+
+		/* update position for next group */
+		pos += group_dim;
 	}
 }


Could this function be split up a bit?

definitely. on todo list.

ddevault · 2017-01-01T17:27:26Z

Overall these changes look good. I like the refactored layout code. You should do a pass on this with the style guide in hand.

There's a lot of complexity I would like to see simplified if you can spare the time to go through everything and see if you can find ways to simplify it (optional).

These are some huge changes, so I'm not going to merge them right away. I'm going to start running them locally for a few days and see if they break any existing functionality. Would appreciate it if any Awesome users could try this and verify that it behaves appropriately as well.

Could you make a wiki page explaining how to configure sway to behave like Awesome?

Achieved by introducing auto_group_bounds function that produces the start/end indexes of a group inside an auto layot container.

- "layout auto_left|auto_xxx" are now "layout auto xxx" - "layout incmaster <n>" is now "layout auto master [set|inc] <n>" - "layout incncol <n>" is now "layout auto ncol [set|inc] <n>"

willakat · 2017-01-08T17:33:54Z

I've been playing around with swaygrab but haven't encountered any SEGVs. I'm stumped as to what caused the parent pointer to get mangled. I could poke around the core file (if you kept it) and it would be interesting if this is a/ reproducible and b/ what sequence of actions preceded the crash.

As for the rest of the comments on the patch, I think I'm only missing the simplification bit (a few have been made, but nothing significant).

Regarding reseting nb_master/nb_slave_groups when switching layouts, I would argue against since it's very convenient to switch back and forth to, say a tabbed layout, to use all the available space, and recover the child layout as it was prior to the switch.

I also still need to push a sample Awesome-style config.

ddevault · 2017-01-08T17:46:48Z

Alright, I'll retest and rereview this when I have some time (probably today or tomorrow). I'll try to get some more info about the swaygrab issue as well (fwiw the failure was when trying to capture video).

thejan2009 · 2017-01-08T18:17:42Z

I'm not sure how Awesome does it, but you might have to look into container movements. The following was achieved by moving a container "up" into the bigger area and then moving it back "down". When additional views are invoked, their width is different from every other container. I suppose they should be equally wide?

ddevault · 2017-01-09T21:20:20Z

sway/sway.5.txt

+
+**layout** auto <mode>::
+	Sets layout to one of the auto modes, i.e. one of _left_, right_, _top_,
+	or _bot_.


Just spell out "bottom" here

ddevault · 2017-01-09T21:21:13Z

sway/sway.5.txt

@@ -360,7 +381,7 @@ The default colors are:
 	switch to workspace 2, then invoke the "workspace 2" command again, you
 	will be returned to workspace 1. Defaults to _no_.

-**workspace_layout** <default|stacking|tabbed>::
+**workspace_layout** <default|stacking|tabbed|auto_left|auto_right|auto_top|auto_bottom>::


workspace_layout auto left, not workspace_layout auto_left

Airblader · 2017-01-10T08:01:09Z

(Random side comment: instead of making i3 jump through hoops for KDE integration KDE should just behave better to not require special casing. Specifications exist for good reasons. I'm not sure why people don't consider this and then blame i3 for it.)

Previous implementation would not preserve dimension of groups along the major axis. This should avoid weird behavior when using container motion commands.

willakat · 2017-01-14T19:09:29Z

Regarding thejan2009's comment, the most recent batch of commits should address the issue. When inserting/removing a child in a container, the auto-layout will attempt to preserve sizes along the "major" axis. Along the "minor" axis, sizes are preserved for children that aren't promoted/demoted to a different group. In the latter case their size will match the spot vacated in the arrival group.

ddevault · 2017-01-14T20:58:24Z

samples/awesome-like.config

+}
+bindsym $mod+r mode "resize"
+
+new_window pixel 1


Can you put this into a directory called contrib instead?

ddevault · 2017-01-14T21:05:32Z

I would also make your sample awesome config more awesome like instead of trying to keep sway's default keybindings intact. Your config doesn't actually even include sway's default keybindings anyway, the defaults are via your config (rather than being hardcoded into sway). Fix that in a seperate pull request if you like.

ddevault · 2017-01-14T21:12:00Z

Thanks for all the hard work!

willakat · 2017-01-15T10:15:52Z

You're welcome! Many thanks for yours on Sway and to the other contributors.

willakat added 11 commits December 29, 2016 20:31

Added Awesome/Monad type "auto" layouts

97f7d47

Handle resize in auto layouts

5425d04

Added "layout incnmaster|incncol" commands

8b0073b

[fix] handle cases where nb_master > children->length in auto layout

ed71e67

[fix] Handle auto layout resize with multiple slave groups

bc3dc97

introduce next/prev as a direction for focus/move commands.

0ff9fe9

[fix] move next/prev behavior for vert/horiz layout

2b0e3c2

cleanup in auto layouts

a0aa8d9

- added L_AUTO_FIRST/LAST instead of using explicit layouts. - when switching between auto layout that don't share the same major axis, invert the width/height of their child views to preserve their relative proportions.

Added "layout promote" command.

1b87193

Added a word in the Readme about the purpose of the fork.

4b1d9b0

[fix] cycle auto layouts backwards

15745ab

Hummer12007 suggested changes Jan 1, 2017

View reviewed changes

ddevault reviewed Jan 1, 2017

View reviewed changes

Fix inline is_auto_layout

c01b898

willakat and others added 10 commits January 7, 2017 20:26

simplification of apply_auto_layout

1f47c58

Achieved by introducing auto_group_bounds function that produces the start/end indexes of a group inside an auto layot container.

Added mouse resize for auto layouts

f24ebd7

Merge branch 'master' into master

653e96f

Merge branch 'master' of https://github.com/SirCmpwn/sway

2040c62

[fix] Keep Clang happy

d822150

Indent cleanups

063c798

reworked "layout auto*" star commands

07474a4

- "layout auto_left|auto_xxx" are now "layout auto xxx" - "layout incmaster <n>" is now "layout auto master [set|inc] <n>" - "layout incncol <n>" is now "layout auto ncol [set|inc] <n>"

fixed up space-after-cast style issues

52f3a8d

Merge branch 'master' of https://github.com/willakat/sway

32430d3

Merge branch 'master' into master

dc361fd

ddevault reviewed Jan 9, 2017

View reviewed changes

ddevault mentioned this pull request Jan 9, 2017

Feature request: Awesome/Xmonad-style automatic layouts i3/i3#2627

Closed

willakat and others added 5 commits January 14, 2017 19:40

Merge branch 'master' into master

4c06a10

Improved behavior of insert/remove child in auto layouts

b74870f

Previous implementation would not preserve dimension of groups along the major axis. This should avoid weird behavior when using container motion commands.

replaced "bot" with "bottom" in auto layout commands

71b3869

Added a sample config for Awesome-like behavior

5c40cc4

[fix] handle auto layout of empty container

a90ddde

ddevault reviewed Jan 14, 2017

View reviewed changes

Move awesome config to contrib/

a2cf3be

ddevault merged commit 81102e8 into swaywm:master Jan 14, 2017

ddevault mentioned this pull request Feb 21, 2017

Sway 0.12 #1027

Closed

2 tasks

		@@ -10,6 +11,10 @@ irc.freenode.net).

		[More screenshots](https://github.com/SirCmpwn/sway/wiki/Screenshots-of-Sway)

		This fork attempts to add

Add Awesome/Monad style automatic layouts to Sway #1024

Add Awesome/Monad style automatic layouts to Sway #1024

Conversation

willakat commented Jan 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddevault Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddevault commented Jan 1, 2017

willakat commented Jan 8, 2017

ddevault commented Jan 8, 2017

thejan2009 commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Airblader commented Jan 10, 2017

willakat commented Jan 14, 2017

Choose a reason for hiding this comment

ddevault commented Jan 14, 2017

ddevault commented Jan 14, 2017

willakat commented Jan 15, 2017

ddevault Jan 1, 2017 •

edited

Loading