Motif hints: Respect maximum border style configuration set by user

Context:
Motif hints [1] allow applications to request specific window manager
frame decorations. Most applications like alacritty, chromium, and
godot, use the hints as a binary flag, setting or un-setting
`MWM_DECOR_ALL`.
Previously [2], we had disallowed applications to set the "normal"
border style through motif hints. This effectively meant that users that
had set `default_border pixel` would not see applications spawning with
normal decorations [3].
However, that meant that applications like godot [4] could not toggle
their border between none and normal so the behaviour changed with
v4.21 [5].
That change however also allowed applications to override the default
none/pixel border style the user set. For example, alacritty can be
configured to either have all or no decorations [6] and they always set
the motif hint on startup, completely overriding i3 user's preference:
1. If decorations are disabled with alacritty's config then they will
   override `default_border normal` and no title will be used.
2. If decorations are enabled (also the default behavior) with
   alacritty's config then they will override `default_border pixel` and
   a title will be used.

This patch redefines how we interpret motif hints. When a client sets
`MWM_DECOR_ALL`, we interpret it as "the maximum decoration the user has
allowed for this window". I.e., if a client was all decorations and the
user expects the window to not have a title, we don't include the title
in "all" decorations.

The user's preference is determined by these:
1. For new tiling windows, as set by `default_border`
2. For new floating windows, as set by `default_floating_border`
3. For all windows that the user runs the `border` command, whatever is
   the result of that command for that window.
Example:
- User opens new tiling window with `default_border pixel` => maximum
  decoration = PIXEL
- Window requests all/title decorations => i3 enforces the user maximum
  decoration, PIXEL (no change)
- Window requests no decorations => i3 accepts it and sets border to
  NONE, maximum decoration remains PIXEL
- User toggles the border, next style is NORMAL => maximum decoration is
  now NORMAL
- Window requests no decorations => i3 accepts it and sets border to
  NONE
- Window requests all/title decorations => i3 accepts it and sets the
  maximum border, NORMAL
- User toggles the border, next style is NONE => maximum decoration is
  now NONE
- Window requests all/title decorations => i3 enforces the user maximum
  decoration, NONE (no change)

With this, we will still allow behaviour where windows can toggle their
border style with motif hints [4][7].

Reference/footnotes:
[1]: https://linux.die.net/man/3/vendorshell
[2]: https://github.com/i3/i3/pull/2386
[3]: Notice how there is apparently a gap because `default border none`
settings would not be respected if an application wanted just "border"
decorations but this was never reported, probably because of the rare
conjunction of applications requesting that and users defaulting to none
borders.
[4]: https://github.com/godotengine/godot/issues/40037
[5]: https://github.com/i3/i3/pull/5135
[6]: Set by an underlying library here:
fafdedfb7d/src/platform_impl/linux/x11/util/hint.rs (L113-L142)
called by alactitty here:
4ddb608563/alacritty/src/display/window.rs (L341)
[7]: https://github.com/i3/i3/issues/3678

Closes #3678
Fixes #5149

include/con.h

@@ -451,7 +451,7 @@ int con_border_style(Con *con);
  * floating window.
  *
  */
-void con_set_border_style(Con *con, int border_style, int border_width);
+void con_set_border_style(Con *con, border_style_t border_style, int border_width);
 
 /**
  * This function changes the layout of a given container. Use it to handle

include/data.h

@@ -723,7 +723,16 @@ struct Con {
      * layout in workspace_layout and creates a new split container with that
      * layout whenever a new container is attached to the workspace. */
     layout_t layout, last_split_layout, workspace_layout;
+
     border_style_t border_style;
+    /* When the border style of a con changes because of motif hints, we don't
+     * want to set more decoration that the user wants. The user's preference is determined by these:
+     * 1. For new tiling windows, as set by `default_border`
+     * 2. For new floating windows, as set by `default_floating_border`
+     * 3. For all windows that the user runs the `border` command, whatever is
+     * the result of that command for that window. */
+    border_style_t max_user_border_style;
+
     /** floating? (= not in tiling layout) This cannot be simply a bool
      * because we want to keep track of whether the status was set by the
      * application (by setting _NET_WM_WINDOW_TYPE appropriately) or by the

release-notes/bugfixes/1-motifs

@@ -0,0 +1 @@
+motif hints: respect maximum border style configuration set by user 

src/commands.c

@@ -747,6 +747,8 @@ void cmd_border(I3_CMD, const char *border_style_str, long border_width) {
             return;
         }
 
+        /* User changed the border */
+        current->con->max_user_border_style = border_style;
         const int con_border_width = border_width_from_style(border_style, border_width, current->con);
         con_set_border_style(current->con, border_style, con_border_width);
     }

src/con.c

@@ -41,7 +41,7 @@ Con *con_new_skeleton(Con *parent, i3Window *window) {
     TAILQ_INSERT_TAIL(&all_cons, new, all_cons);
     new->type = CT_CON;
     new->window = window;
-    new->border_style = config.default_border;
+    new->border_style = new->max_user_border_style = config.default_border;
     new->current_border_width = -1;
     new->window_icon_padding = -1;
     if (window) {
@@ -1794,7 +1794,11 @@ int con_border_style(Con *con) {
  * floating window.
  *
  */
-void con_set_border_style(Con *con, int border_style, int border_width) {
+void con_set_border_style(Con *con, border_style_t border_style, int border_width) {
+    if (border_style > con->max_user_border_style) {
+        border_style = con->max_user_border_style;
+    }
+
     /* Handle the simple case: non-floating containerns */
     if (!con_is_floating(con)) {
         con->border_style = border_style;
@@ -1807,8 +1811,6 @@ void con_set_border_style(Con *con, int border_style, int border_width) {
      * con->rect represent the absolute position of the window (same for
      * parent). Then, we change the border style and subtract the new border
      * pixels. For the parent, we do the same also for the decoration. */
-    DLOG("This is a floating container\n");
-
     Con *parent = con->parent;
     Rect bsr = con_border_style_rect(con);
     int deco_height = (con->border_style == BS_NORMAL ? render_deco_height() : 0);

src/floating.c

@@ -347,8 +347,9 @@ bool floating_enable(Con *con, bool automatic) {
     con->floating = FLOATING_USER_ON;
 
     /* 4: set the border style as specified with new_float */
-    if (automatic)
-        con->border_style = config.default_floating_border;
+    if (automatic) {
+        con->border_style = con->max_user_border_style = config.default_floating_border;
+    }
 
     /* Add pixels for the decoration. */
     Rect border_style_rect = con_border_style_rect(con);

testcases/t/548-motif-hints.t

@@ -0,0 +1,172 @@
+#!perl
+# vim:ts=4:sw=4:expandtab
+#
+# Please read the following documents before working on tests:
+# • https://build.i3wm.org/docs/testsuite.html
+#   (or docs/testsuite)
+#
+# • https://build.i3wm.org/docs/lib-i3test.html
+#   (alternatively: perldoc ./testcases/lib/i3test.pm)
+#
+# • https://build.i3wm.org/docs/ipc.html
+#   (or docs/ipc)
+#
+# • http://onyxneon.com/books/modern_perl/modern_perl_a4.pdf
+#   (unless you are already familiar with Perl)
+#
+# Test that setting and unsetting motif hints updates window decorations
+# accordingly, respecting user configuration.
+# Ticket: #3678
+# Ticket: #5149
+# Bug still in: 4.21
+use List::Util qw(first);
+use i3test i3_autostart => 0;
+use X11::XCB qw(:all);
+
+sub subtest_with_config {
+    my ($style, $cb) = @_;
+    my $config = <<EOT;
+# i3 config file (v4)
+font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1
+
+default_border $style
+EOT
+    my $pid = launch_with_config($config);
+    $cb->();
+    kill_all_windows;
+    exit_gracefully($pid);
+}
+
+sub _change_motif_property {
+    my ($window, $value) = @_;
+    $x->change_property(
+        PROP_MODE_REPLACE,
+        $window->id,
+        $x->atom(name => '_MOTIF_WM_HINTS')->id,
+        $x->atom(name => 'CARDINAL')->id,
+        32, 5,
+        pack('L5', 2, 0, $value, 0, 0),
+    );
+}
+
+sub open_window_with_motifs {
+    my $value = shift;
+    my $window = open_window(
+        before_map => sub {
+            my ($window) = @_;
+            _change_motif_property($window, $value);
+        },
+    );
+
+    sync_with_i3;
+
+    return $window;
+}
+
+my $window;
+sub change_motif_property {
+    my $value = shift;
+    _change_motif_property($window, $value);
+    sync_with_i3;
+}
+
+sub get_border_style {
+    my @content = @{get_ws_content(focused_ws)};
+    my $wininfo = first { $_->{window} == $window->id } @content;
+    return $wininfo->{border};
+}
+
+sub is_border_style {
+    my ($expected, $extra_msg) = @_;
+    my $msg = "border style $expected";
+    if (defined $extra_msg) {
+        $msg = "$msg: $extra_msg";
+    }
+
+    local $Test::Builder::Level = $Test::Builder::Level + 1;
+    is(get_border_style($window), $expected, $msg);
+}
+
+###############################################################################
+subtest 'with default_border normal', \&subtest_with_config, 'normal',
+sub {
+$window = open_window_with_motifs(0);
+is_border_style('none');
+
+$window = open_window_with_motifs(1 << 0);
+is_border_style('normal');
+
+$window = open_window_with_motifs(1 << 1);
+is_border_style('pixel');
+
+$window = open_window_with_motifs(1 << 3);
+is_border_style('normal');
+
+cmd 'border pixel';
+is_border_style('pixel', 'set by user');
+
+change_motif_property(0);
+is_border_style('none');
+
+change_motif_property(1);
+is_border_style('pixel', 'because of user maximum=pixel');
+
+cmd 'border none';
+is_border_style('none', 'set by user');
+
+change_motif_property(0);
+is_border_style('none');
+
+change_motif_property(1);
+is_border_style('none', 'because of user maximum=none');
+};
+
+subtest 'with default_border pixel', \&subtest_with_config, 'pixel',
+sub {
+$window = open_window_with_motifs(0);
+is_border_style('none');
+
+$window = open_window_with_motifs(1 << 0);
+is_border_style('pixel');
+
+$window = open_window_with_motifs(1 << 1);
+is_border_style('pixel');
+
+$window = open_window_with_motifs(1 << 3);
+is_border_style('pixel');
+
+cmd 'border normal';
+is_border_style('normal', 'set by user');
+
+change_motif_property(0);
+is_border_style('none');
+
+change_motif_property(1);
+is_border_style('normal', 'because of user maximum=normal');
+};
+
+subtest 'with default_border none', \&subtest_with_config, 'none',
+sub {
+$window = open_window_with_motifs(0);
+is_border_style('none');
+
+$window = open_window_with_motifs(1 << 0);
+is_border_style('none');
+
+$window = open_window_with_motifs(1 << 1);
+is_border_style('none');
+
+$window = open_window_with_motifs(1 << 3);
+is_border_style('none');
+
+cmd 'border pixel';
+is_border_style('pixel', 'set by user');
+
+change_motif_property(0);
+is_border_style('none');
+
+change_motif_property(1);
+is_border_style('pixel', 'because of user maximum=pixel');
+};
+
+done_testing;