Minor refactoring

- Slightly more efficient processing of Options
- Do not return reference type arguments that are mutated inside the
  function
- Use util.Constrain function when appropriate

3 files changed, 88 insertions, 48 deletions

diff --git a/src/options.go b/src/options.go
index 962e516e..723c7fc6 100644
--- a/src/options.go
+++ b/src/options.go
@@ -163,7 +163,7 @@ func defaultOptions() *Options {
 		Filter:      nil,
 		ToggleSort:  false,
 		Expect:      make(map[int]string),
-		Keymap:      defaultKeymap(),
+		Keymap:      make(map[int]actionType),
 		Execmap:     make(map[int]string),
 		PrintQuery:  false,
 		ReadZero:    false,
@@ -484,7 +484,7 @@ const (
 	escapedComma = 1
 )
 
-func parseKeymap(keymap map[int]actionType, execmap map[int]string, toggleSort bool, str string) (map[int]actionType, map[int]string, bool) {
+func parseKeymap(keymap map[int]actionType, execmap map[int]string, str string) {
 	if executeRegexp == nil {
 		// Backreferences are not supported.
 		// "~!@#$%^&*;/|".each_char.map { |c| Regexp.escape(c) }.map { |c| "#{c}[^#{c}]*#{c}" }.join('|')
@@ -592,7 +592,6 @@ func parseKeymap(keymap map[int]actionType, execmap map[int]string, toggleSort b
 			keymap[key] = actNextHistory
 		case "toggle-sort":
 			keymap[key] = actToggleSort
-			toggleSort = true
 		default:
 			if isExecuteAction(actLower) {
 				var offset int
@@ -613,7 +612,6 @@ func parseKeymap(keymap map[int]actionType, execmap map[int]string, toggleSort b
 			}
 		}
 	}
-	return keymap, execmap, toggleSort
 }
 
 func isExecuteAction(str string) bool {
@@ -635,13 +633,12 @@ func isExecuteAction(str string) bool {
 	return false
 }
 
-func checkToggleSort(keymap map[int]actionType, str string) map[int]actionType {
+func parseToggleSort(keymap map[int]actionType, str string) {
 	keys := parseKeyChords(str, "key name required")
 	if len(keys) != 1 {
 		errorExit("multiple keys specified")
 	}
 	keymap[firstKey(keys)] = actToggleSort
-	return keymap
 }
 
 func strLines(str string) []string {
@@ -691,7 +688,6 @@ func parseMargin(margin string) [4]string {
 }
 
 func parseOptions(opts *Options, allArgs []string) {
-	keymap := make(map[int]actionType)
 	var historyMax int
 	if opts.History == nil {
 		historyMax = defaultHistoryMax
@@ -741,8 +737,7 @@ func parseOptions(opts *Options, allArgs []string) {
 		case "--tiebreak":
 			opts.Criteria = parseTiebreak(nextString(allArgs, &i, "sort criterion required"))
 		case "--bind":
-			keymap, opts.Execmap, opts.ToggleSort =
-				parseKeymap(keymap, opts.Execmap, opts.ToggleSort, nextString(allArgs, &i, "bind expression required"))
+			parseKeymap(opts.Keymap, opts.Execmap, nextString(allArgs, &i, "bind expression required"))
 		case "--color":
 			spec := optionalNextString(allArgs, &i)
 			if len(spec) == 0 {
@@ -751,8 +746,7 @@ func parseOptions(opts *Options, allArgs []string) {
 				opts.Theme = parseTheme(opts.Theme, spec)
 			}
 		case "--toggle-sort":
-			keymap = checkToggleSort(keymap, nextString(allArgs, &i, "key name required"))
-			opts.ToggleSort = true
+			parseToggleSort(opts.Keymap, nextString(allArgs, &i, "key name required"))
 		case "-d", "--delimiter":
 			opts.Delimiter = delimiterRegexp(nextString(allArgs, &i, "delimiter required"))
 		case "-n", "--nth":
@@ -869,8 +863,7 @@ func parseOptions(opts *Options, allArgs []string) {
 			} else if match, _ := optString(arg, "-s", "--sort="); match {
 				opts.Sort = 1 // Don't care
 			} else if match, value := optString(arg, "--toggle-sort="); match {
-				keymap = checkToggleSort(keymap, value)
-				opts.ToggleSort = true
+				parseToggleSort(opts.Keymap, value)
 			} else if match, value := optString(arg, "--expect="); match {
 				opts.Expect = parseKeyChords(value, "key names required")
 			} else if match, value := optString(arg, "--tiebreak="); match {
@@ -878,8 +871,7 @@ func parseOptions(opts *Options, allArgs []string) {
 			} else if match, value := optString(arg, "--color="); match {
 				opts.Theme = parseTheme(opts.Theme, value)
 			} else if match, value := optString(arg, "--bind="); match {
-				keymap, opts.Execmap, opts.ToggleSort =
-					parseKeymap(keymap, opts.Execmap, opts.ToggleSort, value)
+				parseKeymap(opts.Keymap, opts.Execmap, value)
 			} else if match, value := optString(arg, "--history="); match {
 				setHistory(value)
 			} else if match, value := optString(arg, "--history-size="); match {
@@ -905,21 +897,28 @@ func parseOptions(opts *Options, allArgs []string) {
 	if opts.Tabstop < 1 {
 		errorExit("tab stop must be a positive integer")
 	}
+}
 
-	// Change default actions for CTRL-N / CTRL-P when --history is used
+func postProcessOptions(opts *Options) {
+	// Default actions for CTRL-N / CTRL-P when --history is set
 	if opts.History != nil {
-		if _, prs := keymap[curses.CtrlP]; !prs {
-			keymap[curses.CtrlP] = actPreviousHistory
+		if _, prs := opts.Keymap[curses.CtrlP]; !prs {
+			opts.Keymap[curses.CtrlP] = actPreviousHistory
 		}
-		if _, prs := keymap[curses.CtrlN]; !prs {
-			keymap[curses.CtrlN] = actNextHistory
+		if _, prs := opts.Keymap[curses.CtrlN]; !prs {
+			opts.Keymap[curses.CtrlN] = actNextHistory
 		}
 	}
 
-	// Override default key bindings
-	for key, act := range keymap {
-		opts.Keymap[key] = act
+	// Extend the default key map
+	keymap := defaultKeymap()
+	for key, act := range opts.Keymap {
+		if act == actToggleSort {
+			opts.ToggleSort = true
+		}
+		keymap[key] = act
 	}
+	opts.Keymap = keymap
 
 	// If we're not using extended search mode, --nth option becomes irrelevant
 	// if it contains the whole range
@@ -939,9 +938,13 @@ func ParseOptions() *Options {
 
 	// Options from Env var
 	words, _ := shellwords.Parse(os.Getenv("FZF_DEFAULT_OPTS"))
-	parseOptions(opts, words)
+	if len(words) > 0 {
+		parseOptions(opts, words)
+	}
 
 	// Options from command-line arguments
 	parseOptions(opts, os.Args[1:])
+
+	postProcessOptions(opts)
 	return opts
 }
diff --git a/src/options_test.go b/src/options_test.go
index ef86abec..81845d29 100644
--- a/src/options_test.go
+++ b/src/options_test.go
@@ -96,6 +96,7 @@ func TestIrrelevantNth(t *testing.T) {
 		opts := defaultOptions()
 		words := []string{"--nth", "..", "-x"}
 		parseOptions(opts, words)
+		postProcessOptions(opts)
 		if len(opts.Nth) != 0 {
 			t.Errorf("nth should be empty: %s", opts.Nth)
 		}
@@ -104,6 +105,7 @@ func TestIrrelevantNth(t *testing.T) {
 		{
 			opts := defaultOptions()
 			parseOptions(opts, words)
+			postProcessOptions(opts)
 			if len(opts.Nth) != 0 {
 				t.Errorf("nth should be empty: %s", opts.Nth)
 			}
@@ -112,6 +114,7 @@ func TestIrrelevantNth(t *testing.T) {
 			opts := defaultOptions()
 			words = append(words, "-x")
 			parseOptions(opts, words)
+			postProcessOptions(opts)
 			if len(opts.Nth) != 2 {
 				t.Errorf("nth should not be empty: %s", opts.Nth)
 			}
@@ -231,15 +234,11 @@ func TestBind(t *testing.T) {
 	keymap := defaultKeymap()
 	execmap := make(map[int]string)
 	check(actBeginningOfLine, keymap[curses.CtrlA])
-	keymap, execmap, toggleSort :=
-		parseKeymap(keymap, execmap, false,
-			"ctrl-a:kill-line,ctrl-b:toggle-sort,c:page-up,alt-z:page-down,"+
-				"f1:execute(ls {}),f2:execute/echo {}, {}, {}/,f3:execute[echo '({})'],f4:execute;less {};,"+
-				"alt-a:execute@echo (,),[,],/,:,;,%,{}@,alt-b:execute;echo (,),[,],/,:,@,%,{};"+
-				",,:abort,::accept,X:execute:\nfoobar,Y:execute(baz)")
-	if !toggleSort {
-		t.Errorf("toggleSort not set")
-	}
+	parseKeymap(keymap, execmap,
+		"ctrl-a:kill-line,ctrl-b:toggle-sort,c:page-up,alt-z:page-down,"+
+			"f1:execute(ls {}),f2:execute/echo {}, {}, {}/,f3:execute[echo '({})'],f4:execute;less {};,"+
+			"alt-a:execute@echo (,),[,],/,:,;,%,{}@,alt-b:execute;echo (,),[,],/,:,@,%,{};"+
+			",,:abort,::accept,X:execute:\nfoobar,Y:execute(baz)")
 	check(actKillLine, keymap[curses.CtrlA])
 	check(actToggleSort, keymap[curses.CtrlB])
 	check(actPageUp, keymap[curses.AltZ+'c'])
@@ -259,15 +258,11 @@ func TestBind(t *testing.T) {
 	checkString("\nfoobar,Y:execute(baz)", execmap[curses.AltZ+'X'])
 
 	for idx, char := range []rune{'~', '!', '@', '#', '$', '%', '^', '&', '*', '|', ';', '/'} {
-		keymap, execmap, toggleSort =
-			parseKeymap(keymap, execmap, false, fmt.Sprintf("%d:execute%cfoobar%c", idx%10, char, char))
+		parseKeymap(keymap, execmap, fmt.Sprintf("%d:execute%cfoobar%c", idx%10, char, char))
 		checkString("foobar", execmap[curses.AltZ+int([]rune(fmt.Sprintf("%d", idx%10))[0])])
 	}
 
-	keymap, execmap, toggleSort = parseKeymap(keymap, execmap, false, "f1:abort")
-	if toggleSort {
-		t.Errorf("toggleSort set")
-	}
+	parseKeymap(keymap, execmap, "f1:abort")
 	check(actAbort, keymap[curses.F1])
 }
 
@@ -328,3 +323,53 @@ func TestParseNilTheme(t *testing.T) {
 		t.Errorf("color should now be enabled and customized")
 	}
 }
+
+func TestDefaultCtrlNP(t *testing.T) {
+	check := func(words []string, key int, expected actionType) {
+		opts := defaultOptions()
+		parseOptions(opts, words)
+		postProcessOptions(opts)
+		if opts.Keymap[key] != expected {
+			t.Error()
+		}
+	}
+	check([]string{}, curses.CtrlN, actDown)
+	check([]string{}, curses.CtrlP, actUp)
+
+	check([]string{"--bind=ctrl-n:accept"}, curses.CtrlN, actAccept)
+	check([]string{"--bind=ctrl-p:accept"}, curses.CtrlP, actAccept)
+
+	hist := "--history=/tmp/foo"
+	check([]string{hist}, curses.CtrlN, actNextHistory)
+	check([]string{hist}, curses.CtrlP, actPreviousHistory)
+
+	check([]string{hist, "--bind=ctrl-n:accept"}, curses.CtrlN, actAccept)
+	check([]string{hist, "--bind=ctrl-n:accept"}, curses.CtrlP, actPreviousHistory)
+
+	check([]string{hist, "--bind=ctrl-p:accept"}, curses.CtrlN, actNextHistory)
+	check([]string{hist, "--bind=ctrl-p:accept"}, curses.CtrlP, actAccept)
+}
+
+func TestToggle(t *testing.T) {
+	optsFor := func(words ...string) *Options {
+		opts := defaultOptions()
+		parseOptions(opts, words)
+		postProcessOptions(opts)
+		return opts
+	}
+
+	opts := optsFor()
+	if opts.ToggleSort {
+		t.Error()
+	}
+
+	opts = optsFor("--bind=a:toggle-sort")
+	if !opts.ToggleSort {
+		t.Error()
+	}
+
+	opts = optsFor("--bind=a:toggle-sort", "--bind=a:up")
+	if opts.ToggleSort {
+		t.Error()
+	}
+}
diff --git a/src/terminal.go b/src/terminal.go
index f1ddc48e..3c6f47c6 100644
--- a/src/terminal.go
+++ b/src/terminal.go
@@ -1122,15 +1122,7 @@ func (t *Terminal) constrain() {
 	diffpos := t.cy - t.offset
 
 	t.cy = util.Constrain(t.cy, 0, count-1)
-
-	if t.cy > t.offset+(height-1) {
-		// Ceil
-		t.offset = t.cy - (height - 1)
-	} else if t.offset > t.cy {
-		// Floor
-		t.offset = t.cy
-	}
-
+	t.offset = util.Constrain(t.offset, t.cy-height+1, t.cy)
 	// Adjustment
 	if count-t.offset < height {
 		t.offset = util.Max(0, count-height)