fix(action): a user binding displaces the built-in on its key (#63)

README.md

@@ -161,8 +161,9 @@ Key names are those the Picker recognises: `ctrl+x`, `alt+x`, `enter`,
 (`ctrl+n` bound means `↓` is the only way down), but `esc` and `ctrl+c`
 can never be bound. A plain printable key (`a`, `?`) would steal typing, so
 it is an error unless `keys.vim = true`, where it applies in list focus.
-Two Actions on one key is an error, and `key = ""` leaves an Action
-unbound. Each of these is reported with the line it is on.
+Two Actions you bind to one key is an error, though a built-in's default key
+yields to yours, and `key = ""` leaves an Action unbound. Each of these is
+reported with the line it is on.
 
 An `[actions.<name>]` table whose name is a built-in Action overrides only
 the fields it sets. The only built-in Action is `jump`, which Jumps to the
@@ -179,9 +180,12 @@ run    = "code {path}"
 detach = true
 ```
 
-`jump` holds `enter` until you move it, so giving `enter` to another Action
-means moving `jump` or setting `[actions.jump] key = ""`, which leaves it
-unbound. That is allowed; `jump` just has no key.
+A key you set wins over a built-in that holds it by default: `jump` holds
+`enter` until you give `enter` to another Action, which leaves `jump`
+unbound, as if you had set `[actions.jump] key = ""`. So remapping `enter`
+alone is enough; there is no need to move `jump` first. The same goes for
+rebinding one built-in onto another built-in's key. Two Actions you bind to
+the same key are still an error.
 
 ### Layout
 

internal/action/action.go

@@ -72,6 +72,11 @@ func (e *Error) Unwrap() error { return e.Err }
 // result: the built-ins in their own order, then the user's other Actions
 // sorted by name. It fails on the first invalid Action with an *Error.
 //
+// A key a user table sets wins over an Action that only holds it by default,
+// which is left unbound as if the user had set its key to "". Two Actions
+// whose keys the user set to the same key are an error, as are two that
+// both hold it by default.
+//
 // vim says whether the vim key map is on, the only one where a plain
 // printable key may be bound.
 func Merge(user map[string]Override, vim bool) ([]Action, error) {
@@ -123,7 +128,17 @@ func Merge(user map[string]Override, vim bool) ([]Action, error) {
 		if key == "" {
 			continue
 		}
-		if prev, ok := owner[key]; ok {
+		prev, clash := owner[key]
+		switch {
+		case !clash:
+		case user[a.Name].Key != nil && user[prev].Key != nil:
+			return nil, &Error{a.Name, "key", fmt.Errorf("%q is already bound to %q", key, prev)}
+		case user[a.Name].Key != nil:
+			out[index[prev]].Key = ""
+		case user[prev].Key != nil:
+			a.Key = ""
+			continue
+		default:
 			return nil, &Error{a.Name, "key", fmt.Errorf("%q is already bound to %q", key, prev)}
 		}
 		owner[key] = a.Name

internal/action/action_test.go

@@ -105,13 +105,64 @@ func TestMerge_Errors(t *testing.T) {
 	}
 }
 
-func TestMerge_ClashWithABuiltinNamesTheUserAction(t *testing.T) {
+func TestMerge_UserKeyDisplacesABuiltinHoldingIt(t *testing.T) {
 	withBuiltins(t, Action{Name: "files", Key: "ctrl+o", Run: "xdg-open {path}"})
 
-	_, err := Merge(map[string]Override{"code": {Key: str("ctrl+o"), Run: str("code")}}, false)
+	got, err := Merge(map[string]Override{"code": {Key: str("ctrl+o"), Run: str("code")}}, false)
+	if err != nil {
+		t.Fatalf("Merge: %v", err)
+	}
+	keys := map[string]string{}
+	for _, a := range got {
+		keys[a.Name] = a.Key
+	}
+	if keys["code"] != "ctrl+o" || keys["files"] != "" {
+		t.Errorf("keys = %v, want code on ctrl+o and files unbound", keys)
+	}
+}
+
+func TestMerge_UserRebindingABuiltinDisplacesAnotherBuiltin(t *testing.T) {
+	withBuiltins(t,
+		Action{Name: "jump", Key: "enter", Jump: true},
+		Action{Name: "remote", Key: "ctrl+r", Run: "open-remote {path}"},
+	)
+
+	got, err := Merge(map[string]Override{"remote": {Key: str("enter")}}, false)
+	if err != nil {
+		t.Fatalf("Merge: %v", err)
+	}
+	keys := map[string]string{}
+	for _, a := range got {
+		keys[a.Name] = a.Key
+	}
+	if keys["remote"] != "enter" || keys["jump"] != "" {
+		t.Errorf("keys = %v, want remote on enter and jump unbound", keys)
+	}
+}
+
+func TestMerge_UserRebindingAnEarlierBuiltinDisplacesALaterOne(t *testing.T) {
+	withBuiltins(t,
+		Action{Name: "a", Key: "ctrl+a", Run: "x"},
+		Action{Name: "b", Key: "ctrl+b", Run: "y"},
+	)
+
+	got, err := Merge(map[string]Override{"a": {Key: str("ctrl+b")}}, false)
+	if err != nil {
+		t.Fatalf("Merge: %v", err)
+	}
+	if got[0].Key != "ctrl+b" || got[1].Key != "" {
+		t.Errorf("keys = %q, %q, want a on ctrl+b and b unbound", got[0].Key, got[1].Key)
+	}
+}
+
+func TestMerge_TwoUserActionsOnEnterIsAnError(t *testing.T) {
+	_, err := Merge(map[string]Override{
+		"code": {Key: str("enter"), Run: str("code {path}")},
+		"edit": {Key: str("enter"), Run: str("vi {path}")},
+	}, false)
 	var ae *Error
-	if !errors.As(err, &ae) || ae.Name != "code" {
-		t.Fatalf("err = %v, want an *Error for code", err)
+	if !errors.As(err, &ae) || ae.Name != "edit" || ae.Field != "key" {
+		t.Fatalf("err = %v, want an *Error for edit's key", err)
 	}
 }
 
@@ -160,14 +211,8 @@ func TestMerge_JumpRebindsAndEnterGoesToAnotherAction(t *testing.T) {
 	}
 }
 
-func TestMerge_EnterNeedsJumpMovedOrUnboundBeforeAnotherActionTakesIt(t *testing.T) {
-	code := Override{Key: str("enter"), Run: str("code {path}")}
-
-	if _, err := Merge(map[string]Override{"code": code}, false); err == nil {
-		t.Error("Merge succeeded with jump still on enter, want a conflict")
-	}
-
-	got, err := Merge(map[string]Override{"code": code, "jump": {Key: str("")}}, false)
+func TestMerge_EnterGoesToAnotherActionAndJumpIsLeftUnbound(t *testing.T) {
+	got, err := Merge(map[string]Override{"code": {Key: str("enter"), Run: str("code {path}")}}, false)
 	if err != nil {
 		t.Fatalf("Merge: %v", err)
 	}