Merge change for lengthened and configurable escape key timeout

doc_src/bind.txt

@@ -131,3 +131,12 @@ set -g fish_key_bindings fish_vi_key_bindings
 bind -M insert \cc kill-whole-line force-repaint
 \endfish
 Turns on Vi key bindings and rebinds @key{Control,C} to clear the input line.
+
+
+\subsection special-case-escape Special Case: The escape Character
+
+Since the escape key (or character) plays two distinct roles it poses a special challenge for fish. In its first role it stands by itself. In Vi mode, for example, escape is used to switch from insert to normal (aka command) mode. In its second role escape is used as a <a href="https://en.wikipedia.org/wiki/Meta_key">"meta" key</a> where it is only the beginning of some longer character sequence. Coming back to the Vi mode example, sometimes fish is expected to realize that the escape key should be regarded as a meta key, meaning that the escape character is part of a multi-char sequence. Function keys (e.g., F1, F2, etc...) and arrow keys are common cases of multi-char sequences that begin with the escape character. Custom bindings can also be defined that begin with an escape character. Obviously, fish is not supposed to exit insert mode when the escape is part of a longer character sequence.
+
+To be able distinguish between these two roles fish has to wait after it sees an escape character. In this waiting period any additional key presses make the escape key behave as a meta key. Otherwise, it remains an isolated escape key press. The waiting period is set to 300 milliseconds (0.3 seconds) in the default key bindings and 10 milliseconds in the vi key bindings. It can be configured by setting the `fish_escape_delay_ms` variable to a value between 10 and 5000 ms. It is recommended that this be a universal variable that you set once from an interactive session.
+
+Note: fish versions up thru 2.2.0 used a default of 10 ms and provided no way to configure it. That effectively made it impossible to use escape as a meta key.

doc_src/index.hdr.in

@@ -755,17 +755,18 @@ All arrays are one-dimensional and cannot contain other arrays, although it is p
 
 \subsection variables-special Special variables
 
-The user can change the settings of `fish` by changing the values of
+The user can change the settings of `fish` by changing the values of certain variables.
-certain environment variables.
-
-- `BROWSER`, the user's preferred web browser. If this variable is set, fish will use the specified browser instead of the system default browser to display the fish documentation.
-
-- `CDPATH`, an array of directories in which to search for the new directory for the `cd` builtin. By default, the fish configuration defines `CDPATH` to be a universal variable with the values `.` and `~`.
 
 - A large number of variable starting with the prefixes `fish_color` and `fish_pager_color.` See <a href='#variables-color'>Variables for changing highlighting colors</a> for more information.
 
 - `fish_greeting`, the greeting message printed on startup.
 
+- `fish_escape_delay_ms` overrides the default timeout of 300ms (default key bindings) or 10ms (vi key bindings) after seeing an escape character before giving up on matching a key binding. See the documentation for the <a href='bind.html#special-case-escape'>bind</a> builtin command. This delay facilitates using escape as a meta key.
+
+- `BROWSER`, the user's preferred web browser. If this variable is set, fish will use the specified browser instead of the system default browser to display the fish documentation.
+
+- `CDPATH`, an array of directories in which to search for the new directory for the `cd` builtin. By default, the fish configuration defines `CDPATH` to be a universal variable with the values `.` and `~`.
+
 - `LANG`, `LC_ALL`, `LC_COLLATE`, `LC_CTYPE`, `LC_MESSAGES`, `LC_MONETARY`, `LC_NUMERIC` and `LC_TIME` set the language option for the shell and subprograms. See the section <a href='#variables-locale'>Locale variables</a> for more information.
 
 - `fish_user_paths`, an array of directories that are prepended to `PATH`. This can be a universal variable.

share/functions/fish_vi_key_bindings.fish

@@ -1,5 +1,10 @@
 function fish_vi_key_bindings --description 'vi-like key bindings for fish'
-  bind --erase --all
+  # The default escape timeout is 300ms. But for users of Vi bindings that can
+  # be slightly annoying when trying to switch to Vi "normal" mode. Too,
+  # vi-mode users are unlikely to use escape-as-meta. So set a much shorter
+  # timeout in this case.
+  set -q fish_escape_delay_ms; or set -g fish_escape_delay_ms 10
+
   set -l init_mode insert
   set -l eol_keys \$ g\$ \e\[F
   set -l bol_keys \^ 0 g\^ \e\[H
@@ -7,25 +12,20 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
     set init_mode $argv[1]
   end
 
-  # Inherit default key bindings
+  # Inherit default key bindings.
-  # Do this first so vi-bindings win over default
+  # Do this first so vi-bindings win over default.
+  bind --erase --all
   fish_default_key_bindings -M insert
   fish_default_key_bindings -M default
 
-  # Add a way to get out of insert mode
+  # Add a way to switch from insert to normal (command) mode.
   bind -M insert -m default \cc force-repaint
   bind -M insert -m default \e backward-char force-repaint
 
-  ##
+  #
-  ## command mode
+  # normal (command) mode
-  ##
+  #
-
   bind :q exit
-
-  #
-  # normal (default) mode
-  #
-
   bind \cd exit
   bind \cc 'commandline ""'
   bind h backward-char
@@ -173,7 +173,6 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
   #
   # Lowercase r, enters replace-one mode
   #
-
   bind -m replace-one r force-repaint
   bind -M replace-one -m default '' delete-char self-insert backward-char force-repaint
   bind -M replace-one -m default \e cancel force-repaint
@@ -181,7 +180,6 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
   #
   # visual mode
   #
-
   bind -M visual \e\[C forward-char
   bind -M visual \e\[D backward-char
   bind -M visual -k right forward-char

src/env.cpp

@@ -303,7 +303,7 @@ static void handle_locale()
 }
 
 
-/** React to modifying hte given variable */
+/** React to modifying the given variable */
 static void react_to_variable_change(const wcstring &key)
 {
     if (var_is_locale(key))
@@ -319,6 +319,10 @@ static void react_to_variable_change(const wcstring &key)
     {
         reader_react_to_color_change();
     }
+    else if (key == L"fish_escape_delay_ms")
+    {
+        update_wait_on_escape_ms();
+    }
 }
 
 /**

src/input_common.cpp

@@ -28,12 +28,11 @@ Implementation file for the low level input library
 #include "env.h"
 #include "iothread.h"
 
-/**
+// Time in milliseconds to wait for another byte to be available for reading
-   Time in milliseconds to wait for another byte to be available for
+// after \x1b is read before assuming that escape key was pressed, and not an
-   reading after \\x1b is read before assuming that escape key was
+// escape sequence.
-   pressed, and not an escape sequence.
+#define WAIT_ON_ESCAPE_DEFAULT 300
-*/
+static int wait_on_escape_ms = WAIT_ON_ESCAPE_DEFAULT;
-#define WAIT_ON_ESCAPE 10
 
 /** Characters that have been read and returned by the sequence matching code */
 static std::deque<wint_t> lookahead_list;
@@ -77,6 +76,7 @@ static int (*interrupt_handler)();
 void input_common_init(int (*ih)())
 {
     interrupt_handler = ih;
+    update_wait_on_escape_ms();
 }
 
 void input_common_destroy()
@@ -212,36 +212,48 @@ static wint_t readb()
     return arr[0];
 }
 
+// Update the wait_on_escape_ms value in response to the fish_escape_delay_ms
+// user variable being set.
+void update_wait_on_escape_ms()
+{
+    env_var_t escape_time_ms = env_get_string(L"fish_escape_delay_ms");
+    if (escape_time_ms.missing_or_empty())
+    {
+        wait_on_escape_ms = WAIT_ON_ESCAPE_DEFAULT;
+        return;
+    }
+
+    wchar_t *endptr;
+    long tmp = wcstol(escape_time_ms.c_str(), &endptr, 10);
+
+    if (*endptr != '\0' || tmp < 10 || tmp >= 5000)
+    {
+        fwprintf(stderr, L"ignoring fish_escape_delay_ms: value '%ls' "
+                "is not an integer or is < 10 or >= 5000 ms\n",
+                escape_time_ms.c_str());
+    }
+    else
+    {
+        wait_on_escape_ms = (int)tmp;
+    }
+}
+
+
 wchar_t input_common_readch(int timed)
 {
     if (! has_lookahead())
     {
         if (timed)
         {
-            int count;
             fd_set fds;
-            struct timeval tm=
-            {
-                0,
-                1000 * WAIT_ON_ESCAPE
-            }
-            ;
-
             FD_ZERO(&fds);
             FD_SET(0, &fds);
-            count = select(1, &fds, 0, 0, &tm);
+            struct timeval tm = {wait_on_escape_ms / 1000,
-
+                                 1000 * (wait_on_escape_ms % 1000)};
-            switch (count)
+            int count = select(1, &fds, 0, 0, &tm);
+            if (count <= 0)
             {
-                case 0:
+                return WEOF;
-                    return WEOF;
-
-                case -1:
-                    return WEOF;
-                    break;
-                default:
-                    break;
-
             }
         }
 

src/input_common.h

@@ -35,6 +35,9 @@ void input_common_init(int (*ih)());
 */
 void input_common_destroy();
 
+// Adjust the escape timeout.
+void update_wait_on_escape_ms();
+
 /**
    Function used by input_readch to read bytes from stdin until enough
    bytes have been read to convert them to a wchar_t. Conversion is

tests/bind.expect

@@ -1,48 +1,134 @@
 # vim: set filetype=expect:
-
 spawn $fish
-
 expect_prompt
 
-# test switching key bindings
+# Fish should start in default-mode (i.e., emacs) bindings. The default escape
-# this should leave the mode in the appropriate state
+# timeout is 300ms.
 
-send_line "set -g fish_key_bindings fish_vi_key_bindings"
+# Verify the emacs transpose word (\et) behavior using various delays,
+# including none, after the escape character.
+
+# Start by testing with no delay. This should transpose the words.
+send "echo abc def"
+send "\033t\r"
+expect_prompt -re {\r\ndef abc\r\n} {
+    puts "emacs transpose words, default timeout: no delay"
+} unmatched {
+    puts stderr "emacs transpose words fail, default timeout: no delay"
+}
+
+# Now test with a delay > 0 and < the escape timeout. This should transpose
+# the words.
+send "echo ghi jkl"
+send "\033"
+sleep 0.200
+send "t\r"
+expect_prompt -re {\r\njkl ghi\r\n} {
+    puts "emacs transpose words, default timeout: short delay"
+} unmatched {
+    puts stderr "emacs transpose words fail, default timeout: short delay"
+}
+
+# Now test with a delay > the escape timeout. The transposition should not
+# occur and the "t" should become part of the text that is echoed.
+send "echo mno pqr"
+send "\033"
+sleep 0.400
+send "t\r"
+expect_prompt -re {\r\nmno pqrt\r\n} {
+    puts "emacs transpose words, default timeout: long delay"
+} unmatched {
+    puts stderr "emacs transpose words fail, default timeout: long delay"
+}
+
+# Test vi key bindings.
+# This should leave vi mode in the insert state.
+send "set -g fish_key_bindings fish_vi_key_bindings\r"
 expect_prompt
-send_line -h "echo fail\033ddiecho success"
+
-expect_prompt -re {\r\nsuccess\r\n} {
+# These vi tests assume the fish_vi_key_bindings default escape timeout of
-    puts "success"
+# 10ms is in effect; not the 300ms timeout for the default-mode.
-} -nounmatched -re {\r\nfail} {
+#
-    puts stderr "fail"
+# This test is only present to make the Travis-CI framework succeed
+# consistently. It's not clear why the subsequent tests succeed without this
+# test when executed on a local machine but not in the Travis-CI framework.
+send "echo success: default escape timeout\r"
+expect_prompt -re {\r\nsuccess: default escape timeout\r\n} {
+    puts "prime vi mode, default timeout"
 } unmatched {
-    puts stderr "Couldn't find expected output 'success'"
+    puts stderr "prime vi mode, default timeout"
-}
-# try again without the human typing
-send_line -h "echo fail\033ddiecho success"
-expect_prompt -re {\r\nsuccess\r\n} {
-    puts "success"
-} -nounmatched -re {\r\nfail} {
-    puts stderr "fail"
-} unmatched {
-    puts stderr "Couldn't find expected output 'success'"
 }
 
-# Test lowercase-r replace
+send "echo fail: default escape timeout"
-send_line -h "\033ddiecho TEXT\033hhrAi"
+send "\033"
+# Delay needed to allow fish to transition to vi "normal" mode.
+sleep 0.020
+send "ddi"
+send "echo success: default escape timeout\r"
+expect_prompt -re {\r\nsuccess: default escape timeout\r\n} {
+    puts "vi replace line, default timeout: long delay"
+} unmatched {
+    puts stderr "vi replace line, default timeout: long delay"
+}
+
+# Verify that a human can transpose words using \et (which is an emacs default
+# binding but should be valid while in vi insert or normal mode).
+send "echo abc def"
+send "\033"
+sleep 0.005
+send "t\r"
+expect_prompt -re {\r\ndef abc\r\n} {
+    puts "vi transpose words, default timeout: short delay"
+} unmatched {
+    puts stderr "vi transpose words, default timeout: short delay"
+}
+
+# Test replacing a single character.
+send "echo TEXT"
+send "\033"
+# Delay needed to allow fish to transition to vi "normal" mode.
+sleep 0.020
+send "hhrAi\r"
 expect_prompt -re {\r\nTAXT\r\n} {
-    puts "replace success"
+    puts "vi mode replace char, default timeout: long delay"
-} -nounmatched -re {\r\nfail} {
-    puts stderr "replace fail"
 } unmatched {
-    puts stderr "Couldn't find expected output 'TAXT'"
+    puts stderr "vi mode replace char, default timeout: long delay"
+}
+
+# Verify that changing the escape timeout has an effect.
+send "set -g fish_escape_delay_ms 100\r"
+
+expect_prompt
+send "echo fail: lengthened escape timeout"
+send "\033"
+sleep 0.150
+send "ddi"
+send "echo success: lengthened escape timeout\r"
+expect_prompt -re {\r\nsuccess: lengthened escape timeout\r\n} {
+    puts "vi replace line, 100ms timeout: long delay"
+} unmatched {
+    puts stderr "vi replace line, 100ms timeout: long delay"
+}
+
+# Verify that we don't switch to vi normal mode if we don't wait long enough
+# after sending escape.
+send "echo fail: no normal mode"
+send "\033"
+sleep 0.050
+send "ddi"
+send "inserted\r"
+expect_prompt -re {\r\nfail: no normal modediinserted\r\n} {
+    puts "vi replace line, 100ms timeout: short delay"
+} unmatched {
+    puts stderr "vi replace line, 100ms timeout: short delay"
 }
 
 # Test 't' binding that contains non-zero arity function (forward-jump) followed
 # by another function (and) https://github.com/fish-shell/fish-shell/issues/2357
 send "\033"
-sleep 0.500
+sleep 0.510
 send "ddiecho TEXT\033"
-sleep 0.500
+sleep 0.510
 send "hhtTrN\r"
 expect_prompt -re {\r\nTENT\r\n} {
     puts "t-binding success"
@@ -52,17 +138,44 @@ expect_prompt -re {\r\nTENT\r\n} {
     puts stderr "Couldn't find expected output 'TENT'"
 }
 
-# still in insert mode, switch back to regular key bindings
+# Switch back to regular (emacs mode) key bindings.
-send_line -h "set -g fish_key_bindings fish_default_key_bindings"
+# The custom escape timeout of 100ms set earlier should still be in effect.
+send "set -g fish_key_bindings fish_default_key_bindings\r"
 expect_prompt
-send_line "echo success"
+
-expect_prompt -re {\r\nsuccess\r\n} {
+# Verify the emacs transpose word (\et) behavior using various delays,
-    puts "success"
+# including none, after the escape character.
+
+# Start by testing with no delay. This should transpose the words.
+send "echo abc def"
+send "\033"
+send "t\r"
+expect_prompt -re {\r\ndef abc\r\n} {
+    puts "emacs transpose words, 100ms timeout: no delay"
 } unmatched {
-    puts stderr "Couldn't find expected output 'success'"
+    puts stderr "emacs transpose words fail, 100ms timeout: no delay"
-} timeout {
+}
-    set msg ""
+
-    append msg "Timeout after setting fish_key_bindings to fish_default_key_bindings\n" \
+
-               "\$fish_bind_mode is most likely still set to 'insert'"
+# Same test as above but with a slight delay less than the escape timeout.
-    abort $msg
+send "echo ghi jkl"
+send "\033"
+sleep 0.080
+send "t\r"
+expect_prompt -re {\r\njkl ghi\r\n} {
+    puts "emacs transpose words, 100ms timeout: short delay"
+} unmatched {
+    puts stderr "emacs transpose words fail, 100ms timeout: short delay"
+}
+
+# Now test with a delay > the escape timeout. The transposition should not
+# occur and the "t" should become part of the text that is echoed.
+send "echo mno pqr"
+send "\033"
+sleep 0.120
+send "t\r"
+expect_prompt -re {\r\nmno pqrt\r\n} {
+    puts "emacs transpose words, 100ms timeout: long delay"
+} unmatched {
+    puts stderr "emacs transpose words fail, 100ms timeout: long delay"
 }

tests/bind.expect.out

@@ -1,5 +1,13 @@
-success
+emacs transpose words, default timeout: no delay
-success
+emacs transpose words, default timeout: short delay
-replace success
+emacs transpose words, default timeout: long delay
+prime vi mode, default timeout
+vi replace line, default timeout: long delay
+vi transpose words, default timeout: short delay
+vi mode replace char, default timeout: long delay
+vi replace line, 100ms timeout: long delay
+vi replace line, 100ms timeout: short delay
 t-binding success
-success
+emacs transpose words, 100ms timeout: no delay
+emacs transpose words, 100ms timeout: short delay
+emacs transpose words, 100ms timeout: long delay

tests/interactive.expect.rc

@@ -5,7 +5,7 @@ log_file -noappend interactive.tmp.log
 
 set fish ../fish
 
-set timeout 4
+set timeout 5
 
 set send_human {.05 .1 5 .02 .2}
 

tests/interactive.fish

@@ -2,7 +2,17 @@
 #
 # Interactive tests using `expect`
 
-source test_util.fish (status -f); or exit
+# Change to directory containing this script
+cd (dirname (status -f))
+
+# Test files specified on commandline, or all *.expect files
+if set -q argv[1]
+    set files_to_test $argv.expect
+else
+    set files_to_test *.expect
+end
+
+source test_util.fish (status -f) $argv; or exit
 
 say -o cyan "Testing interactive functionality"
 if not type -q expect
@@ -65,7 +75,7 @@ function test_file
 end
 
 set -l failed
-for i in *.expect
+for i in $files_to_test
     if not test_file $i
         set failed $failed $i
     end

tests/test.fish

@@ -6,7 +6,6 @@
 cd (dirname (status -f))
 
 # Test files specified on commandline, or all *.in files
-set -q argv[1]
 if set -q argv[1]
     set files_to_test $argv.in
 else