Remove R_TIMEOUT

Promote timeout to a char_event_type_t, moving it out of the "char" namespace.
This will help simplify the readline implementation.

src/fish_key_reader.cpp

@@ -208,9 +208,10 @@ static void process_input(bool continuous_mode) {
         if (reader_test_and_clear_interrupted()) {
             wc = shell_modes.c_cc[VINTR];
         } else {
-            wc = input_common_readch(true);
+            auto mwc = input_common_readch_timed(true);
+            wc = mwc.is_char() ? mwc.get_char() : R_EOF;
         }
-        if (wc == R_TIMEOUT || wc == R_EOF) {
+        if (wc == R_EOF) {
             output_bind_command(bind_chars);
             if (first_char_seen && !continuous_mode) {
                 return;

src/fish_tests.cpp

@@ -2996,9 +2996,12 @@ static void test_input() {
     }
 
     // Now test.
-    wint_t c = input_readch();
+    auto evt = input_readch();
-    if (c != R_DOWN_LINE) {
+    if (!evt.is_char()) {
-        err(L"Expected to read char R_DOWN_LINE, but instead got %ls\n", describe_char(c).c_str());
+        err(L"Event is not a char");
+    } else if (evt.get_char() != R_DOWN_LINE) {
+        err(L"Expected to read char R_DOWN_LINE, but instead got %ls\n",
+            describe_char(evt.get_char()).c_str());
     }
 }
 

src/input.cpp

@@ -51,6 +51,9 @@ struct input_mapping_t {
         static unsigned int s_last_input_map_spec_order = 0;
         specification_order = ++s_last_input_map_spec_order;
     }
+
+    /// \return true if this is a generic mapping, i.e. acts as a fallback.
+    bool is_generic() const { return seq.empty(); }
 };
 
 /// A struct representing the mapping from a terminfo key name to a terminfo character sequence.
@@ -297,17 +300,19 @@ wchar_t input_function_pop_arg() { return input_function_args[--input_function_a
 
 void input_function_push_args(int code) {
     int arity = input_function_arity(code);
-    std::vector<wchar_t> skipped;
+    std::vector<char_event_t> skipped;
 
     for (int i = 0; i < arity; i++) {
-        wchar_t arg;
-
         // Skip and queue up any function codes. See issue #2357.
-        while ((arg = input_common_readch(0)) >= R_BEGIN_INPUT_FUNCTIONS &&
+        wchar_t arg{};
-               arg < R_END_INPUT_FUNCTIONS) {
+        for (;;) {
-            skipped.push_back(arg);
+            auto evt = input_common_readch();
+            if (evt.is_char() && !evt.is_readline()) {
+                arg = evt.get_char();
+                break;
+            }
+            skipped.push_back(evt);
         }
-
         input_function_push_arg(arg);
     }
 
@@ -384,15 +389,13 @@ static bool input_mapping_is_match(const input_mapping_t &m) {
 
     bool timed = false;
     for (size_t i = 0; i < str.size(); ++i) {
-        wchar_t read = input_common_readch(timed);
+        auto evt = timed ? input_common_readch_timed() : input_common_readch();
-
+        if (!evt.is_char() || evt.get_char() != str[i]) {
-        if (read != str[i]) {
+            // We didn't match the bind sequence/input mapping, (it timed out or they entered
-            // We didn't match the bind sequence/input mapping, (it timed out or they entered something else)
+            // something else) Undo consumption of the read characters since we didn't match the
-            // Undo consumption of the read characters since we didn't match the bind sequence and abort.
+            // bind sequence and abort.
-            input_common_next_ch(read);
+            input_common_next_ch(evt);
-            while (i--) {
+            while (i--) input_common_next_ch(str[i]);
-                input_common_next_ch(str[i]);
-            }
             return false;
         }
 
@@ -404,87 +407,77 @@ static bool input_mapping_is_match(const input_mapping_t &m) {
     return true;
 }
 
-void input_queue_ch(wint_t ch) { input_common_queue_ch(ch); }
+void input_queue_ch(char_event_t ch) { input_common_queue_ch(ch); }
 
-static void input_mapping_execute_matching_or_generic(bool allow_commands) {
+/// \return the first mapping that matches, walking first over the user's mapping list, then the
+/// preset list. \return null if nothing matches.
+static const input_mapping_t *find_mapping() {
     const input_mapping_t *generic = NULL;
-
     const auto &vars = parser_t::principal_parser().vars();
     const wcstring bind_mode = input_get_bind_mode(vars);
 
-    for (auto& m : mapping_list) {
+    const auto lists = {&mapping_list, &preset_mapping_list};
+    for (const auto *listp : lists) {
+        for (const auto &m : *listp) {
             if (m.mode != bind_mode) {
                 continue;
             }
 
-        if (m.seq.length() == 0) {
+            if (m.is_generic()) {
-            generic = &m;
-        } else if (input_mapping_is_match(m)) {
-            input_mapping_execute(m, allow_commands);
-            return;
-        }
-    }
-
-    // HACK: This is ugly duplication.
-    for (auto& m : preset_mapping_list) {
-        if (m.mode != bind_mode) {
-            continue;
-        }
-
-        if (m.seq.length() == 0) {
-            // Only use this generic if the user list didn't have one.
                 if (!generic) generic = &m;
             } else if (input_mapping_is_match(m)) {
-            input_mapping_execute(m, allow_commands);
+                return &m;
-            return;
             }
         }
+    }
+    return generic;
+}
 
-    if (generic) {
+static void input_mapping_execute_matching_or_generic(bool allow_commands) {
-        input_mapping_execute(*generic, allow_commands);
+    const input_mapping_t *mapping = find_mapping();
+    if (mapping) {
+        input_mapping_execute(*mapping, allow_commands);
     } else {
         debug(2, L"no generic found, ignoring char...");
-        wchar_t c = input_common_readch(0);
+        auto evt = input_common_readch();
-        if (c == R_EOF) {
+        if (evt.is_char() && evt.get_char() == R_EOF) {
-            input_common_next_ch(c);
+            input_common_next_ch(evt);
         }
     }
 }
 
 /// Helper function. Picks through the queue of incoming characters until we get to one that's not a
 /// readline function.
-static wchar_t input_read_characters_only() {
+static char_event_t input_read_characters_only() {
-    std::vector<wchar_t> functions_to_put_back;
+    std::vector<char_event_t> saved_events;
-    wchar_t char_to_return;
+    char_event_t char_to_return{0};
     for (;;) {
-        char_to_return = input_common_readch(0);
+        auto evt = input_common_readch();
-        bool is_readline_function =
+        if (evt.is_char()) {
-            (char_to_return >= R_BEGIN_INPUT_FUNCTIONS && char_to_return < R_END_INPUT_FUNCTIONS);
+            auto c = evt.get_char();
-        // R_NULL and R_EOF are more control characters than readline functions, so check specially
+            if (!evt.is_readline() || c == R_NULL || c == R_EOF) {
-        // for those.
+                char_to_return = evt;
-        if (!is_readline_function || char_to_return == R_NULL || char_to_return == R_EOF) {
                 break;
             }
-        // This is a readline function; save it off for later re-enqueing and try again.
+        }
-        functions_to_put_back.push_back(char_to_return);
+        saved_events.push_back(evt);
     }
     // Restore any readline functions, in reverse to preserve their original order.
-    size_t idx = functions_to_put_back.size();
+    for (auto iter = saved_events.rbegin(); iter != saved_events.rend(); ++iter) {
-    while (idx--) {
+        input_common_next_ch(*iter);
-        input_common_next_ch(functions_to_put_back.at(idx));
     }
     return char_to_return;
 }
 
-wint_t input_readch(bool allow_commands) {
+char_event_t input_readch(bool allow_commands) {
     // Clear the interrupted flag.
     reader_reset_interrupted();
     // Search for sequence in mapping tables.
     while (true) {
-        wchar_t c = input_common_readch(0);
+        auto evt = input_common_readch();
 
-        if (c >= R_BEGIN_INPUT_FUNCTIONS && c < R_END_INPUT_FUNCTIONS) {
+        if (evt.is_readline()) {
-            switch (c) {
+            switch (evt.get_char()) {
                 case R_SELF_INSERT: {
                     // Issue #1595: ensure we only insert characters, not readline functions. The
                     // common case is that this will be empty.
@@ -494,21 +487,20 @@ wint_t input_readch(bool allow_commands) {
                     if (input_function_status) {
                         return input_readch();
                     }
-                    c = input_common_readch(0);
+                    do {
-                    while (c >= R_BEGIN_INPUT_FUNCTIONS && c < R_END_INPUT_FUNCTIONS) {
+                        evt = input_common_readch();
-                        c = input_common_readch(0);
+                    } while (evt.is_readline());
-                    }
+                    input_common_next_ch(evt);
-                    input_common_next_ch(c);
                     return input_readch();
                 }
-                default: { return c; }
+                default: { return evt; }
             }
-        } else if (c == R_EOF) {
+        } else if (evt.is_char() && evt.get_char() == R_EOF) {
             // If we have R_EOF, we need to immediately quit.
             // There's no need to go through the input functions.
-            return R_EOF;
+            return evt;
         } else {
-            input_common_next_ch(c);
+            input_common_next_ch(evt);
             input_mapping_execute_matching_or_generic(allow_commands);
             // Regarding allow_commands, we're in a loop, but if a fish command
             // is executed, R_NULL is unread, so the next pass through the loop

src/input.h

@@ -9,6 +9,7 @@
 
 #include "builtin_bind.h"
 #include "common.h"
+#include "input_common.h"
 
 #define FISH_BIND_MODE_VAR L"fish_bind_mode"
 
@@ -32,11 +33,11 @@ void init_input();
 /// The argument determines whether fish commands are allowed to be run as bindings. If false, when
 /// a character is encountered that would invoke a fish command, it is unread and R_NULL is
 /// returned.
-wint_t input_readch(bool allow_commands = true);
+char_event_t input_readch(bool allow_commands = true);
 
 /// Enqueue a character or a readline function to the queue of unread characters that input_readch
 /// will return before actually reading from fd 0.
-void input_queue_ch(wint_t ch);
+void input_queue_ch(char_event_t ch);
 
 /// Add a key mapping from the specified sequence to the specified command.
 ///

src/input_common.cpp

@@ -33,8 +33,8 @@
 #define WAIT_ON_ESCAPE_DEFAULT 30
 static int wait_on_escape_ms = WAIT_ON_ESCAPE_DEFAULT;
 
-/// Characters that have been read and returned by the sequence matching code.
+/// Events which have been read and returned by the sequence matching code.
-static std::deque<wchar_t> lookahead_list;
+static std::deque<char_event_t> lookahead_list;
 
 // Queue of pairs of (function pointer, argument) to be invoked. Expected to be mostly empty.
 typedef std::list<std::function<void(void)>> callback_queue_t;
@@ -43,17 +43,24 @@ static void input_flush_callbacks();
 
 static bool has_lookahead() { return !lookahead_list.empty(); }
 
-static wint_t lookahead_pop() {
+static char_event_t lookahead_pop() {
-    wint_t result = lookahead_list.front();
+    auto result = lookahead_list.front();
     lookahead_list.pop_front();
     return result;
 }
 
-static void lookahead_push_back(wint_t c) { lookahead_list.push_back(c); }
+/// \return the next lookahead char, or none if none. Discards timeouts.
+static maybe_t<wchar_t> lookahead_pop_char() {
+    while (has_lookahead()) {
+        auto evt = lookahead_pop();
+        if (evt.is_char()) return evt.get_char();
+    }
+    return none();
+}
 
-static void lookahead_push_front(wint_t c) { lookahead_list.push_front(c); }
+static void lookahead_push_back(char_event_t c) { lookahead_list.push_back(c); }
 
-static wint_t lookahead_front() { return lookahead_list.front(); }
+static void lookahead_push_front(char_event_t c) { lookahead_list.push_front(c); }
 
 /// Callback function for handling interrupts on reading.
 static int (*interrupt_handler)();
@@ -109,7 +116,9 @@ static wint_t readb() {
                 if (interrupt_handler) {
                     int res = interrupt_handler();
                     if (res) return res;
-                    if (has_lookahead()) return lookahead_pop();
+                    if (auto mc = lookahead_pop_char()) {
+                        return *mc;
+                    }
                 }
 
                 do_loop = true;
@@ -133,8 +142,8 @@ static wint_t readb() {
 
             if (ioport > 0 && FD_ISSET(ioport, &fdset)) {
                 iothread_service_completion();
-                if (has_lookahead()) {
+                if (auto mc = lookahead_pop_char()) {
-                    return lookahead_pop();
+                    return *mc;
                 }
             }
 
@@ -173,29 +182,18 @@ void update_wait_on_escape_ms(const environment_t &vars) {
     }
 }
 
-wchar_t input_common_readch(int timed) {
+char_event_t input_common_readch() {
-    if (!has_lookahead()) {
+    if (auto mc = lookahead_pop_char()) {
-        if (timed) {
+        return *mc;
-            fd_set fds;
-            FD_ZERO(&fds);
-            FD_SET(0, &fds);
-            struct timeval tm = {wait_on_escape_ms / 1000, 1000 * (wait_on_escape_ms % 1000)};
-            int count = select(1, &fds, 0, 0, &tm);
-            if (count <= 0) {
-                return R_TIMEOUT;
     }
-        }
-
     wchar_t res;
     mbstate_t state = {};
-
     while (1) {
         wint_t b = readb();
 
         if (b >= R_NULL && b < R_END_INPUT_FUNCTIONS) return b;
 
         if (MB_CUR_MAX == 1) {
-                // return (unsigned char)b;  // single-byte locale, all values are legal
             return b;  // single-byte locale, all values are legal
         }
 
@@ -217,19 +215,32 @@ wchar_t input_common_readch(int timed) {
             default: { return res; }
         }
     }
+}
+
+char_event_t input_common_readch_timed(bool dequeue_timeouts) {
+    char_event_t result{char_event_type_t::timeout};
+    if (has_lookahead()) {
+        result = lookahead_pop();
     } else {
-        if (!timed) {
+        fd_set fds;
-            while (has_lookahead() && lookahead_front() == R_TIMEOUT) lookahead_pop();
+        FD_ZERO(&fds);
-            if (!has_lookahead()) return input_common_readch(0);
+        FD_SET(STDIN_FILENO, &fds);
-        }
+        struct timeval tm = {wait_on_escape_ms / 1000, 1000 * (wait_on_escape_ms % 1000)};
-
+        if (select(1, &fds, 0, 0, &tm) > 0) {
-        return lookahead_pop();
+            result = input_common_readch();
         }
     }
+    // If we got a timeout, either through dequeuing or creating, ensure it stays on the queue.
+    if (result.is_timeout()) {
+        if (!dequeue_timeouts) lookahead_push_front(char_event_type_t::timeout);
+        return char_event_type_t::timeout;
+    }
+    return result.get_char();
+}
 
-void input_common_queue_ch(wint_t ch) { lookahead_push_back(ch); }
+void input_common_queue_ch(char_event_t ch) { lookahead_push_back(ch); }
 
-void input_common_next_ch(wint_t ch) { lookahead_push_front(ch); }
+void input_common_next_ch(char_event_t ch) { lookahead_push_front(ch); }
 
 void input_common_add_callback(std::function<void(void)> callback) {
     ASSERT_IS_MAIN_THREAD();

src/input_common.h

@@ -7,6 +7,7 @@
 #include <functional>
 
 #include "common.h"
+#include "maybe.h"
 
 enum {
     R_MIN = INPUT_COMMON_BASE,
@@ -74,35 +75,75 @@ enum {
     R_REPEAT_JUMP,
     R_REVERSE_REPEAT_JUMP,
 
-    R_TIMEOUT,  // we didn't get interactive input within wait_on_escape_ms
-
     // The range of key codes for inputrc-style keyboard functions that are passed on to the caller
     // of input_read().
     R_BEGIN_INPUT_FUNCTIONS = R_BEGINNING_OF_LINE,
     R_END_INPUT_FUNCTIONS = R_REVERSE_REPEAT_JUMP + 1
 };
 
+/// Represents an event on the character input stream.
+enum class char_event_type_t {
+    /// A character was entered.
+    charc,
+
+    /// A timeout was hit.
+    timeout,
+};
+
+class char_event_t {
+    /// Set if the type is charc.
+    wchar_t c_;
+
+   public:
+    char_event_type_t type;
+
+    bool is_timeout() const { return type == char_event_type_t::timeout; }
+
+    bool is_char() const { return type == char_event_type_t::charc; }
+
+    bool is_readline() const {
+        return is_char() && c_ >= R_BEGIN_INPUT_FUNCTIONS && c_ < R_END_INPUT_FUNCTIONS;
+    }
+
+    wchar_t get_char() const {
+        assert(type == char_event_type_t::charc && "Not a char type");
+        return c_;
+    }
+
+    /* implicit */ char_event_t(wchar_t c) : c_(c), type(char_event_type_t::charc) {}
+
+    /* implicit */ char_event_t(char_event_type_t type) : c_(0), type(type) {
+        assert(type != char_event_type_t::charc &&
+               "Cannot create a char event with this constructor");
+    }
+};
+
 /// Init the library.
 void input_common_init(int (*ih)());
 
 /// Adjust the escape timeout.
+class environment_t;
 void update_wait_on_escape_ms(const environment_t &vars);
 
 /// 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 done using mbrtowc. If a character has previously been
-/// read and then 'unread' using \c input_common_unreadch, that character is returned. If timed is
+/// read and then 'unread' using \c input_common_unreadch, that character is returned.
-/// true, readch2 will wait at most WAIT_ON_ESCAPE milliseconds for a character to be available for
+/// This function never returns a timeout.
-/// reading before returning with the value R_EOF.
+char_event_t input_common_readch();
-wchar_t input_common_readch(int timed);
+
+/// Like input_common_readch(), except it will wait at most WAIT_ON_ESCAPE milliseconds for a
+/// character to be available for reading.
+/// If \p dequeue_timeouts is set, remove any timeout from the queue; otherwise retain them.
+char_event_t input_common_readch_timed(bool dequeue_timeouts = false);
 
 /// Enqueue a character or a readline function to the queue of unread characters that input_readch
 /// will return before actually reading from fd 0.
-void input_common_queue_ch(wint_t ch);
+void input_common_queue_ch(char_event_t ch);
 
 /// Add a character or a readline function to the front of the queue of unread characters.  This
 /// will be the first character returned by input_readch (unless this function is called more than
 /// once).
-void input_common_next_ch(wint_t ch);
+void input_common_next_ch(char_event_t ch);
 
 /// Adds a callback to be invoked at the next turn of the "event loop." The callback function will
 /// be invoked and passed arg.

src/reader.cpp

@@ -2382,8 +2382,14 @@ static bool text_ends_in_comment(const wcstring &text) {
     return token.type == TOK_COMMENT;
 }
 
+/// \return true if an event is a normal character that should be inserted into the buffer.
+static bool event_is_normal_char(const char_event_t &evt) {
+    if (!evt.is_char()) return false;
+    auto c = evt.get_char();
+    return !fish_reserved_codepoint(c) && c > 31 && c != 127;
+}
+
 maybe_t<wcstring> reader_data_t::readline(int nchars) {
-    wint_t c;
     int last_char = 0;
     size_t yank_len = 0;
     const wchar_t *yank_str;
@@ -2436,38 +2442,45 @@ maybe_t<wcstring> reader_data_t::readline(int nchars) {
         // Sometimes strange input sequences seem to generate a zero byte. I believe these simply
         // mean a character was pressed but it should be ignored. (Example: Trying to add a tilde
         // (~) to digit).
+        maybe_t<char_event_t> event_needing_handling{};
         while (1) {
             int was_interactive_read = is_interactive_read;
             is_interactive_read = 1;
-            c = input_readch();
+            event_needing_handling = input_readch();
             is_interactive_read = was_interactive_read;
             // std::fwprintf(stderr, L"C: %lx\n", (long)c);
 
-            if (((!fish_reserved_codepoint(c))) && (c > 31) && (c != 127) && can_read(0)) {
+            if (event_is_normal_char(*event_needing_handling) && can_read(STDIN_FILENO)) {
-                wchar_t arr[READAHEAD_MAX + 1];
+                // This is a normal character input.
-                size_t i;
+                // We are going to handle it directly, accumulating more.
+                // Clear 'mevt' to mark that we handled this.
+                char_event_t evt = event_needing_handling.acquire();
                 size_t limit = 0 < nchars ? std::min((size_t)nchars - command_line.size(),
                                                      (size_t)READAHEAD_MAX)
                                           : READAHEAD_MAX;
 
-                std::memset(arr, 0, sizeof(arr));
+                wchar_t arr[READAHEAD_MAX + 1] = {};
-                arr[0] = c;
+                arr[0] = evt.get_char();
+                last_char = arr[0];
 
-                for (i = 1; i < limit; ++i) {
+                for (size_t i = 1; i < limit; ++i) {
                     if (!can_read(0)) {
-                        c = 0;
                         break;
                     }
                     // Only allow commands on the first key; otherwise, we might have data we
                     // need to insert on the commandline that the commmand might need to be able
                     // to see.
-                    c = input_readch(false);
+                    auto next_event = input_readch(false);
-                    if (!fish_reserved_codepoint(c) && c > 31 && c != 127) {
+                    if (event_is_normal_char(next_event)) {
-                        arr[i] = c;
+                        arr[i] = next_event.get_char();
-                        c = 0;
+                        last_char = arr[i];
-                    } else
+                    } else {
+                        // We need to process this in the outer loop.
+                        assert(!event_needing_handling && "Should not have an unhandled event");
+                        event_needing_handling = next_event;
                         break;
                     }
+                }
 
                 editable_line_t *el = active_edit_line();
                 insert_string(el, arr, true);
@@ -2476,17 +2489,24 @@ maybe_t<wcstring> reader_data_t::readline(int nchars) {
                 if (el == &command_line) {
                     clear_pager();
                 }
-                last_char = c;
             }
 
-            if (c != 0) break;
+            // If there's still an event that we were unable to handle, then end the coalescing
+            // loop.
+            if (event_needing_handling.has_value()) break;
 
             if (0 < nchars && (size_t)nchars <= command_line.size()) {
-                c = R_NULL;
+                event_needing_handling.reset();
                 break;
             }
         }
 
+        if (!event_needing_handling) {
+            event_needing_handling = R_NULL;
+        }
+        assert(event_needing_handling->is_char() && "Should have a char event");
+        wchar_t c = event_needing_handling->get_char();
+
         // If we get something other than a repaint, then stop coalescing them.
         if (c != R_REPAINT) coalescing_repaints = false;