Send events more often for variable sets outside of builtin_set

When changing certain variables programmatically, ensure that events
are sent. Fixes #6653

src/builtin_fg.cpp

@@ -98,9 +98,9 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         std::fwprintf(stderr, FG_MSG, job->job_id(), job->command_wcstr());
     }
 
-    const wcstring ft = tok_command(job->command());
+    wcstring ft = tok_command(job->command());
     // For compatibility with fish 2.0's $_, now replaced with `status current-command`
-    if (!ft.empty()) parser.vars().set_one(L"_", ENV_EXPORT, ft);
+    if (!ft.empty()) parser.set_var_and_fire(L"_", ENV_EXPORT, std::move(ft));
     reader_write_title(job->command(), parser);
 
     parser.job_promote(job);

src/builtin_read.cpp

@@ -429,7 +429,6 @@ static int validate_read_args(const wchar_t *cmd, read_cmd_opts_t &opts, int arg
 
 /// The read builtin. Reads from stdin and stores the values in environment variables.
 int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
-    auto &vars = parser.vars();
     wchar_t *cmd = argv[0];
     int argc = builtin_count_args(argv);
     wcstring buff;
@@ -519,22 +518,22 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
                     }
                 }
 
-                vars.set(*var_ptr++, opts.place, tokens);
+                parser.set_var_and_fire(*var_ptr++, opts.place, std::move(tokens));
             } else {
                 maybe_t<tok_t> t;
                 while ((vars_left() - 1 > 0) && (t = tok.next())) {
                     auto text = tok.text_of(*t);
                     if (unescape_string(text, &out, UNESCAPE_DEFAULT)) {
-                        vars.set_one(*var_ptr++, opts.place, out);
+                        parser.set_var_and_fire(*var_ptr++, opts.place, out);
                     } else {
-                        vars.set_one(*var_ptr++, opts.place, text);
+                        parser.set_var_and_fire(*var_ptr++, opts.place, text);
                     }
                 }
 
                 // If we still have tokens, set the last variable to them.
                 if ((t = tok.next())) {
                     wcstring rest = wcstring(buff, t->offset);
-                    vars.set_one(*var_ptr++, opts.place, rest);
+                    parser.set_var_and_fire(*var_ptr++, opts.place, std::move(rest));
                 }
             }
             // The rest of the loop is other split-modes, we don't care about those.
@@ -567,12 +566,12 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
 
             if (opts.array) {
                 // Array mode: assign each char as a separate element of the sole var.
-                vars.set(*var_ptr++, opts.place, chars);
+                parser.set_var_and_fire(*var_ptr++, opts.place, chars);
             } else {
                 // Not array mode: assign each char to a separate var with the remainder being
                 // assigned to the last var.
                 for (const auto &c : chars) {
-                    vars.set_one(*var_ptr++, opts.place, c);
+                    parser.set_var_and_fire(*var_ptr++, opts.place, c);
                 }
             }
         } else if (opts.array) {
@@ -588,14 +587,14 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
                      loc.first != wcstring::npos; loc = wcstring_tok(buff, opts.delimiter, loc)) {
                     tokens.emplace_back(wcstring(buff, loc.first, loc.second));
                 }
-                vars.set(*var_ptr++, opts.place, tokens);
+                parser.set_var_and_fire(*var_ptr++, opts.place, tokens);
             } else {
                 // We're using a delimiter provided by the user so use the `string split` behavior.
                 wcstring_list_t splits;
                 split_about(buff.begin(), buff.end(), opts.delimiter.begin(), opts.delimiter.end(),
                             &splits);
 
-                vars.set(*var_ptr++, opts.place, splits);
+                parser.set_var_and_fire(*var_ptr++, opts.place, splits);
             }
         } else {
             // Not array mode. Split the input into tokens and assign each to the vars in sequence.
@@ -609,7 +608,7 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
                     if (loc.first != wcstring::npos) {
                         substr = wcstring(buff, loc.first, loc.second);
                     }
-                    vars.set_one(*var_ptr++, opts.place, substr);
+                    parser.set_var_and_fire(*var_ptr++, opts.place, substr);
                 }
             } else {
                 // We're using a delimiter provided by the user so use the `string split` behavior.
@@ -620,7 +619,7 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
                             &splits, argc - 1);
                 assert(splits.size() <= (size_t)vars_left());
                 for (const auto &split : splits) {
-                    vars.set_one(*var_ptr++, opts.place, split);
+                    parser.set_var_and_fire(*var_ptr++, opts.place, split);
                 }
             }
         }

src/fish.cpp

@@ -504,7 +504,7 @@ int main(int argc, char **argv) {
                 for (char **ptr = argv + my_optind; *ptr; ptr++) {
                     list.push_back(str2wcstring(*ptr));
                 }
-                parser.vars().set(L"argv", ENV_DEFAULT, list);
+                parser.vars().set(L"argv", ENV_DEFAULT, std::move(list));
 
                 auto &ld = parser.libdata();
                 wcstring rel_filename = str2wcstring(file);

src/input.cpp

@@ -185,12 +185,13 @@ static wcstring input_get_bind_mode(const environment_t &vars) {
 }
 
 /// Set the current bind mode.
-static void input_set_bind_mode(env_stack_t &vars, const wcstring &bm) {
+static void input_set_bind_mode(parser_t &parser, const wcstring &bm) {
     // Only set this if it differs to not execute variable handlers all the time.
     // modes may not be empty - empty is a sentinel value meaning to not change the mode
     assert(!bm.empty());
-    if (input_get_bind_mode(vars) != bm) {
-        vars.set_one(FISH_BIND_MODE_VAR, ENV_GLOBAL, bm);
+    if (input_get_bind_mode(parser.vars()) != bm) {
+        // Must send events here - see #6653.
+        parser.set_var_and_fire(FISH_BIND_MODE_VAR, ENV_GLOBAL, bm);
     }
 }
 
@@ -364,7 +365,7 @@ void inputter_t::mapping_execute(const input_mapping_t &m, bool allow_commands)
 
     // !has_functions && !has_commands: only set bind mode
     if (!has_commands && !has_functions) {
-        if (!m.sets_mode.empty()) input_set_bind_mode(parser_->vars(), m.sets_mode);
+        if (!m.sets_mode.empty()) input_set_bind_mode(*parser_, m.sets_mode);
         return;
     }
 
@@ -401,7 +402,7 @@ void inputter_t::mapping_execute(const input_mapping_t &m, bool allow_commands)
     }
 
     // Empty bind mode indicates to not reset the mode (#2871)
-    if (!m.sets_mode.empty()) input_set_bind_mode(parser_->vars(), m.sets_mode);
+    if (!m.sets_mode.empty()) input_set_bind_mode(*parser_, m.sets_mode);
 }
 
 /// Try reading the specified function mapping.

src/parse_execution.cpp

@@ -403,9 +403,9 @@ end_execution_reason_t parse_execution_context_t::run_for_statement(
     }
     int retval;
     if (var) {
-        retval = parser->vars().set(for_var_name, ENV_LOCAL | ENV_USER, var->as_list());
+        retval = parser->set_var_and_fire(for_var_name, ENV_LOCAL | ENV_USER, var->as_list());
     } else {
-        retval = parser->vars().set_empty(for_var_name, ENV_LOCAL | ENV_USER);
+        retval = parser->set_empty_var_and_fire(for_var_name, ENV_LOCAL | ENV_USER);
     }
     assert(retval == ENV_OK);
 
@@ -424,7 +424,7 @@ end_execution_reason_t parse_execution_context_t::run_for_statement(
             break;
         }
 
-        int retval = parser->vars().set_one(for_var_name, ENV_DEFAULT | ENV_USER, val);
+        int retval = parser->set_var_and_fire(for_var_name, ENV_DEFAULT | ENV_USER, val);
         assert(retval == ENV_OK && "for loop variable should have been successfully set");
         (void)retval;
 
@@ -1014,7 +1014,7 @@ end_execution_reason_t parse_execution_context_t::apply_variable_assignments(
             vals.emplace_back(std::move(completion.completion));
         }
         if (proc) proc->variable_assignments.push_back({variable_name, vals});
-        parser->vars().set(variable_name, ENV_LOCAL | ENV_EXPORT, std::move(vals));
+        parser->set_var_and_fire(variable_name, ENV_LOCAL | ENV_EXPORT, std::move(vals));
     }
     return end_execution_reason_t::ok;
 }

src/parser.cpp

@@ -107,6 +107,25 @@ void parser_t::cancel_requested(int sig) {
     principal->cancellation_signal = sig;
 }
 
+int parser_t::set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals) {
+    std::vector<event_t> events;
+    int res = vars().set(key, mode, std::move(vals), &events);
+    for (const auto &evt : events) {
+        event_fire(*this, evt);
+    }
+    return res;
+}
+
+int parser_t::set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring val) {
+    wcstring_list_t vals;
+    vals.push_back(std::move(val));
+    return set_var_and_fire(key, mode, std::move(vals));
+}
+
+int parser_t::set_empty_var_and_fire(const wcstring &key, env_mode_flags_t mode) {
+    return set_var_and_fire(key, mode, wcstring_list_t{});
+}
+
 // Given a new-allocated block, push it onto our block list, acquiring ownership.
 block_t *parser_t::push_block(block_t &&block) {
     block_t new_current{block};

src/parser.h

@@ -346,6 +346,12 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     statuses_t get_last_statuses() const { return vars().get_last_statuses(); }
     void set_last_statuses(statuses_t s) { vars().set_last_statuses(std::move(s)); }
 
+    /// Cover of vars().set(), which also fires any returned event handlers.
+    /// \return a value like ENV_OK.
+    int set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring val);
+    int set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals);
+    int set_empty_var_and_fire(const wcstring &key, env_mode_flags_t mode);
+
     /// Pushes a new block. Returns a pointer to the block, stored in the parser. The pointer is
     /// valid until the call to pop_block()
     block_t *push_block(block_t &&b);

tests/bind_mode_events.expect

@@ -0,0 +1,36 @@
+# vim: set filetype=expect:
+spawn $fish
+expect_prompt
+
+send "set -g fish_key_bindings fish_vi_key_bindings\r"
+expect_prompt
+
+send "echo ready to go\r"
+expect_prompt -re {\r\nready to go\r\n} {
+    puts "ready to go"
+}
+send "function add_change --on-variable fish_bind_mode ; set -g MODE_CHANGES \$MODE_CHANGES \$fish_bind_mode ; end\r"
+expect_prompt
+
+# normal mode
+send "\033"
+sleep 0.050
+
+# insert mode
+send "i"
+sleep 0.050
+
+# back to normal mode
+send "\033"
+sleep 0.050
+
+# insert mode again
+send "i"
+sleep 0.050
+
+send "echo mode changes: \$MODE_CHANGES\r"
+expect_prompt -re {\r\nmode changes: default insert default insert\r\n} {
+        puts "Correct mode changes"
+} unmatched {
+    puts "Incorrect mode changes"
+}

tests/bind_mode_events.expect.out

@@ -0,0 +1,2 @@
+ready to go
+Correct mode changes