diff --git a/README.md b/README.md index 8099d14..4c40308 100644 --- a/README.md +++ b/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.]` 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 diff --git a/internal/action/action.go b/internal/action/action.go index f52fce3..7377c31 100644 --- a/internal/action/action.go +++ b/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 diff --git a/internal/action/action_test.go b/internal/action/action_test.go index ccc7c8c..a90c2e1 100644 --- a/internal/action/action_test.go +++ b/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) }