Rework default config binding replacement

This reworks the criteria necessary for default bindings to be
overwritten by custom user bindings. It should provide a better
heuristic which accounts for the possibility that notmodes are added
which prevent a conflict between the bindings, so the default isn't
unnecessarily removed.

It is still possible to define a new binding that intersects a default
binding without the default getting removed, if the user explicitly
specifies a mode that doesn't include the default binding's mode. This
is based on the assumption that users explicitly specifying a new mode
are trying to make a mode-specific addition and are capable of removing
the default when desired.

This helps with old modes still being present in user's config files,
since a lot of new config options just have the additional ~Vi mode set.

Fixes #3476.

2 files changed, 44 insertions, 20 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index bf0c80c6..54c5ebbe 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Default color scheme is now `Tomorrow Night` with the bright colors of `Tomorrow Night Bright`
 - Set IUTF8 termios flag for improved UTF8 input support
 - Dragging files into terminal now adds a space after each path
+- Default binding replacement conditions
 
 ### Fixed
 
diff --git a/alacritty/src/config/bindings.rs b/alacritty/src/config/bindings.rs
index c187b04e..547e168c 100644
--- a/alacritty/src/config/bindings.rs
+++ b/alacritty/src/config/bindings.rs
@@ -49,9 +49,9 @@ impl<T: Eq> Binding<T> {
         // the most likely item to fail so prioritizing it here allows more
         // checks to be short circuited.
         self.trigger == *input
+            && self.mods == mods
             && mode.contains(self.mode)
             && !mode.intersects(self.notmode)
-            && (self.mods == mods)
     }
 
     #[inline]
@@ -61,16 +61,20 @@ impl<T: Eq> Binding<T> {
             return false;
         }
 
-        // Completely empty modes match all modes.
-        if (self.mode.is_empty() && self.notmode.is_empty())
-            || (binding.mode.is_empty() && binding.notmode.is_empty())
-        {
-            return true;
+        let selfmode = if self.mode.is_empty() { TermMode::ANY } else { self.mode };
+        let bindingmode = if binding.mode.is_empty() { TermMode::ANY } else { binding.mode };
+
+        if !selfmode.intersects(bindingmode) {
+            return false;
+        }
+
+        // The bindings are never active at the same time when the required modes of one binding
+        // are part of the forbidden bindings of the other.
+        if self.mode.intersects(binding.notmode) || binding.mode.intersects(self.notmode) {
+            return false;
         }
 
-        // Check for intersection (equality is required since empty does not intersect itself).
-        (self.mode == binding.mode || self.mode.intersects(binding.mode))
-            && (self.notmode == binding.notmode || self.notmode.intersects(binding.notmode))
+        true
     }
 }
 
@@ -1023,6 +1027,7 @@ mod tests {
         b2.mode = TermMode::ALT_SCREEN;
 
         assert!(b1.triggers_match(&b2));
+        assert!(b2.triggers_match(&b1));
     }
 
     #[test]
@@ -1043,26 +1048,18 @@ mod tests {
         let b2 = MockBinding::default();
 
         assert!(b1.triggers_match(&b2));
+        assert!(b2.triggers_match(&b1));
     }
 
     #[test]
-    fn binding_matches_superset_mode() {
-        let mut b1 = MockBinding::default();
-        b1.mode = TermMode::APP_KEYPAD;
-        let mut b2 = MockBinding::default();
-        b2.mode = TermMode::ALT_SCREEN | TermMode::APP_KEYPAD;
-
-        assert!(b1.triggers_match(&b2));
-    }
-
-    #[test]
-    fn binding_matches_subset_mode() {
+    fn binding_matches_modes() {
         let mut b1 = MockBinding::default();
         b1.mode = TermMode::ALT_SCREEN | TermMode::APP_KEYPAD;
         let mut b2 = MockBinding::default();
         b2.mode = TermMode::APP_KEYPAD;
 
         assert!(b1.triggers_match(&b2));
+        assert!(b2.triggers_match(&b1));
     }
 
     #[test]
@@ -1073,6 +1070,7 @@ mod tests {
         b2.mode = TermMode::APP_KEYPAD | TermMode::APP_CURSOR;
 
         assert!(b1.triggers_match(&b2));
+        assert!(b2.triggers_match(&b1));
     }
 
     #[test]
@@ -1083,6 +1081,7 @@ mod tests {
         b2.notmode = TermMode::ALT_SCREEN;
 
         assert!(!b1.triggers_match(&b2));
+        assert!(!b2.triggers_match(&b1));
     }
 
     #[test]
@@ -1093,6 +1092,30 @@ mod tests {
         b2.mode = TermMode::APP_KEYPAD;
 
         assert!(!b1.triggers_match(&b2));
+        assert!(!b2.triggers_match(&b1));
+    }
+
+    #[test]
+    fn binding_matches_notmodes() {
+        let mut subset_notmodes = MockBinding::default();
+        let mut superset_notmodes = MockBinding::default();
+        subset_notmodes.notmode = TermMode::VI | TermMode::APP_CURSOR;
+        superset_notmodes.notmode = TermMode::APP_CURSOR;
+
+        assert!(subset_notmodes.triggers_match(&superset_notmodes));
+        assert!(superset_notmodes.triggers_match(&subset_notmodes));
+    }
+
+    #[test]
+    fn binding_matches_mode_notmode() {
+        let mut b1 = MockBinding::default();
+        let mut b2 = MockBinding::default();
+        b1.mode = TermMode::VI;
+        b1.notmode = TermMode::APP_CURSOR;
+        b2.notmode = TermMode::APP_CURSOR;
+
+        assert!(b1.triggers_match(&b2));
+        assert!(b2.triggers_match(&b1));
     }
 
     #[test]