From acad9b05e2ee2066173e101e84e1c04ae6d61343 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 7 Mar 2020 19:44:58 -0800 Subject: [PATCH] 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 | 4 ++-- src/builtin_read.cpp | 21 +++++++++--------- src/fish.cpp | 2 +- src/input.cpp | 11 +++++----- src/parse_execution.cpp | 8 +++---- src/parser.cpp | 19 ++++++++++++++++ src/parser.h | 6 ++++++ tests/bind_mode_events.expect | 36 +++++++++++++++++++++++++++++++ tests/bind_mode_events.expect.err | 0 tests/bind_mode_events.expect.out | 2 ++ 10 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 tests/bind_mode_events.expect create mode 100644 tests/bind_mode_events.expect.err create mode 100644 tests/bind_mode_events.expect.out diff --git a/src/builtin_fg.cpp b/src/builtin_fg.cpp index ca363688c..f4d451c95 100644 --- a/src/builtin_fg.cpp +++ b/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); diff --git a/src/builtin_read.cpp b/src/builtin_read.cpp index 3600953a2..93d67409b 100644 --- a/src/builtin_read.cpp +++ b/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 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); } } } diff --git a/src/fish.cpp b/src/fish.cpp index 57573a25d..674c20471 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -505,7 +505,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); diff --git a/src/input.cpp b/src/input.cpp index 205d40655..09a9232ae 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -182,12 +182,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); } } @@ -361,7 +362,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; } @@ -400,7 +401,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. diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 4316e91a8..2bb810d66 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -403,9 +403,9 @@ eval_result_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 @@ eval_result_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; @@ -1037,7 +1037,7 @@ eval_result_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 eval_result_t::ok; } diff --git a/src/parser.cpp b/src/parser.cpp index 1eae37392..9e7cfa6de 100644 --- a/src/parser.cpp +++ b/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 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}; diff --git a/src/parser.h b/src/parser.h index 0901100da..dd10c9a01 100644 --- a/src/parser.h +++ b/src/parser.h @@ -326,6 +326,12 @@ class parser_t : public std::enable_shared_from_this { 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); diff --git a/tests/bind_mode_events.expect b/tests/bind_mode_events.expect new file mode 100644 index 000000000..bec435d0c --- /dev/null +++ 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" +} diff --git a/tests/bind_mode_events.expect.err b/tests/bind_mode_events.expect.err new file mode 100644 index 000000000..e69de29bb diff --git a/tests/bind_mode_events.expect.out b/tests/bind_mode_events.expect.out new file mode 100644 index 000000000..487091460 --- /dev/null +++ b/tests/bind_mode_events.expect.out @@ -0,0 +1,2 @@ +ready to go +Correct mode changes