Bugfix: multiple criteria should use a logical AND (+test) (Thanks f8l)

src/match.c

@@ -69,40 +69,66 @@ void match_copy(Match *dest, Match *src) {
  *
  */
 bool match_matches_window(Match *match, i3Window *window) {
+    LOG("checking window %d (%s)\n", window->id, window->class_class);
+
     /* TODO: pcre, full matching, … */
-    if (match->class != NULL && window->class_class != NULL && strcasecmp(match->class, window->class_class) == 0) {
+    if (match->class != NULL) {
-        LOG("match made by window class (%s)\n", window->class_class);
+        if (window->class_class != NULL && strcasecmp(match->class, window->class_class) == 0) {
-        return true;
+            LOG("window class matches (%s)\n", window->class_class);
+        } else {
+            LOG("window class does not match\n");
+            return false;
+        }
     }
 
-    if (match->instance != NULL && window->class_instance != NULL && strcasecmp(match->instance, window->class_instance) == 0) {
+    if (match->instance != NULL) {
-        LOG("match made by window instance (%s)\n", window->class_instance);
+        if (window->class_instance != NULL && strcasecmp(match->instance, window->class_instance) == 0) {
-        return true;
+            LOG("window instance matches (%s)\n", window->class_instance);
+        } else {
+            LOG("window instance does not match\n");
+            return false;
+        }
     }
 
-    if (match->id != XCB_NONE && window->id == match->id) {
+    if (match->id != XCB_NONE) {
-        LOG("match made by window id (%d)\n", window->id);
+        if (window->id == match->id) {
-        return true;
+            LOG("match made by window id (%d)\n", window->id);
+        } else {
+            LOG("window id does not match\n");
+            return false;
+        }
     }
 
     /* TODO: pcre match */
-    if (match->title != NULL && window->name_json != NULL && strcasecmp(match->title, window->name_json) == 0) {
+    if (match->title != NULL) {
-        LOG("match made by title (%s)\n", window->name_json);
+        if (window->name_json != NULL && strcasecmp(match->title, window->name_json) == 0) {
-        return true;
+            LOG("title matches (%s)\n", window->name_json);
+        } else {
+            LOG("title does not match\n");
+            return false;
+        }
     }
 
-    LOG("match->dock = %d, window->dock = %d\n", match->dock, window->dock);
+    if (match->dock != -1) {
-    if (match->dock != -1 &&
+        LOG("match->dock = %d, window->dock = %d\n", match->dock, window->dock);
-        ((window->dock == W_DOCK_TOP && match->dock == M_DOCK_TOP) ||
+        if ((window->dock == W_DOCK_TOP && match->dock == M_DOCK_TOP) ||
          (window->dock == W_DOCK_BOTTOM && match->dock == M_DOCK_BOTTOM) ||
          ((window->dock == W_DOCK_TOP || window->dock == W_DOCK_BOTTOM) &&
           match->dock == M_DOCK_ANY) ||
-         (window->dock == W_NODOCK && match->dock == M_NODOCK))) {
+         (window->dock == W_NODOCK && match->dock == M_NODOCK)) {
-        LOG("match made by dock\n");
+            LOG("dock status matches\n");
-        return true;
+        } else {
+            LOG("dock status does not match\n");
+            return false;
+        }
     }
 
-    LOG("window %d (%s) could not be matched\n", window->id, window->class_class);
+    /* We don’t check the mark because this function is not even called when
+     * the mark would have matched - it is checked in cmdparse.y itself */
+    if (match->mark != NULL) {
+        LOG("mark does not match\n");
+        return false;
+    }
 
-    return false;
+    return true;
 }

testcases/t/19-match.t

@@ -56,4 +56,64 @@ ok(@{$content} == 0, 'window killed');
 
 # TODO: same test, but with pcre expressions
 
+######################################################################
+# check that multiple criteria work are checked with a logical AND,
+# not a logical OR (that is, matching is not cancelled after the first
+# criterion matches).
+######################################################################
+
+$tmp = fresh_workspace;
+
+# TODO: move to X11::XCB
+sub set_wm_class {
+    my ($id, $class, $instance) = @_;
+
+    # Add a _NET_WM_STRUT_PARTIAL hint
+    my $atomname = $x->atom(name => 'WM_CLASS');
+    my $atomtype = $x->atom(name => 'STRING');
+
+    $x->change_property(
+        PROP_MODE_REPLACE,
+        $id,
+        $atomname->id,
+        $atomtype->id,
+        8,
+        length($class) + length($instance) + 2,
+        "$instance\x00$class\x00"
+    );
+}
+
+my $left = $x->root->create_child(
+    class => WINDOW_CLASS_INPUT_OUTPUT,
+    rect => [ 0, 0, 30, 30 ],
+    background_color => '#0000ff',
+);
+
+$left->_create;
+set_wm_class($left->id, 'special', 'special');
+$left->name('left');
+$left->map;
+sleep 0.25;
+
+my $right = $x->root->create_child(
+    class => WINDOW_CLASS_INPUT_OUTPUT,
+    rect => [ 0, 0, 30, 30 ],
+    background_color => '#0000ff',
+);
+
+$right->_create;
+set_wm_class($right->id, 'special', 'special');
+$right->name('right');
+$right->map;
+sleep 0.25;
+
+# two windows should be here
+$content = get_ws_content($tmp);
+ok(@{$content} == 2, 'two windows opened');
+
+cmd '[class="special" title="left"] kill';
+
+$content = get_ws_content($tmp);
+is(@{$content}, 1, 'one window still there');
+
 done_testing;