From e72a360337548ffe6db0c97e9c79416e42521450 Mon Sep 17 00:00:00 2001 From: Junegunn Choi Date: Thu, 18 Feb 2016 01:46:18 +0900 Subject: [PATCH] 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 --- src/options.go | 51 +++++++++++++++--------------- src/options_test.go | 75 ++++++++++++++++++++++++++++++++++++--------- src/terminal.go | 10 +----- 3 files changed, 88 insertions(+), 48 deletions(-) diff --git a/src/options.go b/src/options.go index 962e516..723c7fc 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 ef86abe..81845d2 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 f1ddc48..3c6f47c 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)