From 00490cd08427250aaabc4fca791d8c0381b95fde Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 31 Jul 2022 20:48:38 +0200 Subject: [PATCH 1/3] Rename "disable_history" stack state to "noninteractive" The commit after next will fix a bug where we wrongly disable prompt history in some scenarios. The root cause is that life span of "disable_history" does not model when we actually want to disable history. Let's rename the state variable to "noninteractive". It's set whenever we are executing a hook, mapping or command. Note that it's also active inside ":prompt"'s callback, which doesn't play well with the new name :( --- src/commands.cc | 14 +++++++------- src/context.hh | 6 +++--- src/hook_manager.cc | 2 +- src/input_handler.cc | 4 ++-- src/normal.cc | 8 ++++---- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/commands.cc b/src/commands.cc index 1075c332..446ecd29 100644 --- a/src/commands.cc +++ b/src/commands.cc @@ -1993,7 +1993,7 @@ void context_wrap(const ParametersParser& parser, Context& context, StringView d Context::Flags::Draft}; Context& c = input_handler.context(); - ScopedSetBool disable_history(c.history_disabled()); + ScopedSetBool noninteractive(c.noninteractive()); func(parser, c); }; @@ -2044,7 +2044,7 @@ void context_wrap(const ParametersParser& parser, Context& context, StringView d Context& c = *effective_context; - ScopedSetBool disable_history(c.history_disabled()); + ScopedSetBool noninteractive(c.noninteractive()); ScopedEdition edition{c}; ScopedSelectionEdition selection_edition{c}; @@ -2222,7 +2222,7 @@ const CommandDesc prompt_cmd = { sc.env_vars.erase("text"_sv); }); - ScopedSetBool disable_history{context.history_disabled()}; + ScopedSetBool noninteractive{context.noninteractive()}; StringView cmd; switch (event) @@ -2269,7 +2269,7 @@ const CommandDesc menu_cmd = { if (count == modulo and parser.get_switch("auto-single")) { - ScopedSetBool disable_history{context.history_disabled()}; + ScopedSetBool noninteractive{context.noninteractive()}; CommandManager::instance().execute(parser[1], context); return; @@ -2293,7 +2293,7 @@ const CommandDesc menu_cmd = { CapturedShellContext sc{shell_context}; context.input_handler().menu(std::move(choices), [=](int choice, MenuEvent event, Context& context) { - ScopedSetBool disable_history{context.history_disabled()}; + ScopedSetBool noninteractive{context.noninteractive()}; if (event == MenuEvent::Validate and choice >= 0 and choice < commands.size()) CommandManager::instance().execute(commands[choice], context, sc); @@ -2324,7 +2324,7 @@ const CommandDesc on_key_cmd = { parser.get_switch("mode-name").value_or("on-key"), KeymapMode::None, [=](Key key, Context& context) mutable { sc.env_vars["key"_sv] = to_string(key); - ScopedSetBool disable_history{context.history_disabled()}; + ScopedSetBool noninteractive{context.noninteractive()}; CommandManager::instance().execute(command, context, sc); }); @@ -2646,7 +2646,7 @@ void enter_user_mode(Context& context, String mode_name, KeymapMode mode, bool l return; ScopedSetBool disable_keymaps(context.keymaps_disabled()); - ScopedSetBool disable_history(context.history_disabled()); + ScopedSetBool noninteractive(context.noninteractive()); InputHandler::ScopedForceNormal force_normal{context.input_handler(), {}}; diff --git a/src/context.hh b/src/context.hh index 34bb9e12..69377148 100644 --- a/src/context.hh +++ b/src/context.hh @@ -126,8 +126,8 @@ public: NestedBool& keymaps_disabled() { return m_keymaps_disabled; } const NestedBool& keymaps_disabled() const { return m_keymaps_disabled; } - NestedBool& history_disabled() { return m_history_disabled; } - const NestedBool& history_disabled() const { return m_history_disabled; } + NestedBool& noninteractive() { return m_noninteractive; } + const NestedBool& noninteractive() const { return m_noninteractive; } Flags flags() const { return m_flags; } @@ -207,7 +207,7 @@ private: NestedBool m_hooks_disabled; NestedBool m_keymaps_disabled; - NestedBool m_history_disabled; + NestedBool m_noninteractive; }; struct ScopedEdition diff --git a/src/hook_manager.cc b/src/hook_manager.cc index 406b803f..afab5461 100644 --- a/src/hook_manager.cc +++ b/src/hook_manager.cc @@ -37,7 +37,7 @@ struct HookManager::HookData enum_desc(Meta::Type{})[to_underlying(hook)].name, param, group)); - ScopedSetBool disable_history{context.history_disabled()}; + ScopedSetBool noninteractive{context.noninteractive()}; EnvVarMap env_vars{ {"hook_param", param.str()} }; for (size_t i = 0; i < captures.size(); ++i) diff --git a/src/input_handler.cc b/src/input_handler.cc index a36d13f1..4e953738 100644 --- a/src/input_handler.cc +++ b/src/input_handler.cc @@ -1197,7 +1197,7 @@ private: void history_push(StringView entry) { - if (entry.empty() or context().history_disabled() or + if (entry.empty() or context().noninteractive() or (m_flags & PromptFlags::DropHistoryEntriesWithBlankPrefix and is_horizontal_blank(entry[0_byte]))) return; @@ -1772,7 +1772,7 @@ void InputHandler::handle_key(Key key) KeymapManager& keymaps = m_context.keymaps(); if (keymaps.is_mapped(key, keymap_mode) and not m_context.keymaps_disabled()) { - ScopedSetBool disable_history{context().history_disabled()}; + ScopedSetBool noninteractive{context().noninteractive()}; for (auto& k : keymaps.get_mapping_keys(key, keymap_mode)) process_key(k); diff --git a/src/normal.cc b/src/normal.cc index 08ce028d..01f56a01 100644 --- a/src/normal.cc +++ b/src/normal.cc @@ -1016,7 +1016,7 @@ void select_regex(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.history_disabled()) + if (not context.noninteractive()) RegisterManager::instance()[reg].set(context, ex.str()); auto& selections = context.selections(); @@ -1038,7 +1038,7 @@ void split_regex(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.history_disabled()) + if (not context.noninteractive()) RegisterManager::instance()[reg].set(context, ex.str()); auto& selections = context.selections(); @@ -1142,7 +1142,7 @@ void keep(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.history_disabled()) + if (not context.noninteractive()) RegisterManager::instance()[reg].set(context, regex.str()); if (regex.empty() or regex.str().empty()) @@ -2049,7 +2049,7 @@ void exec_user_mappings(Context& context, NormalParams params) return; ScopedSetBool disable_keymaps(context.keymaps_disabled()); - ScopedSetBool disable_history(context.history_disabled()); + ScopedSetBool noninteractive(context.noninteractive()); InputHandler::ScopedForceNormal force_normal{context.input_handler(), params}; From 6563b820929772a32ce1afe1d9fce1cc5d1faf2c Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 31 Jul 2022 21:34:13 +0200 Subject: [PATCH 2/3] Use auto to avoid repeating type of dynamic cast I think the clang-tidy lint is called modernize-use-auto --- src/input_handler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/input_handler.cc b/src/input_handler.cc index 4e953738..c134c3e9 100644 --- a/src/input_handler.cc +++ b/src/input_handler.cc @@ -1705,7 +1705,7 @@ void InputHandler::prompt(StringView prompt, String initstr, String emptystr, void InputHandler::set_prompt_face(Face prompt_face) { - InputModes::Prompt* prompt = dynamic_cast(¤t_mode()); + auto* prompt = dynamic_cast(¤t_mode()); if (prompt) prompt->set_prompt_face(prompt_face); } From 163eb6dbc6758e84f8ce107632777fd06c8ac92d Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Mon, 29 Aug 2022 08:00:53 +0200 Subject: [PATCH 3/3] Disable history only for prompts that are never shown in the UI My terminal allows to map and independently. I like to use as escape key so I have this mapping: map global prompt Unfortunately, this is not equivalent to . Since mappings are run with history disabled, will not add the command to the prompt history. So disabling command history inside mappings is wrong in case the command prompt was created before mapping execution. The behavior should be: "a prompt that is both created and closed inside a noninteractive context does not add to prompt history", where "noninteractive" means inside a mapping, hook, command, execute-keys or evaluate-commands. Implement this behavior, it should better meet user expectations. Scripts can always use "set-register" to add to history. Here are my test cases: 1. Basic regression test (needs above mapping): :nop should be added to history --- 2. Create the prompt in a noninteractive context: :exec %{:} now we're back in the interactive context, so we can type: nop should be added to history --- 3. To check if it works for nested prompts, first set up this mapping. map global prompt ':nop should NOT be added to history' map global prompt ':nop should be added to history first' Then type :nop should be added to history second the inner command run by should not be added to history because it only existed in a noninteractive context. --- See also the discussion https://github.com/mawww/kakoune/pull/4692 We could automate the tests if we had a test setup that allowed feeding interactive key input into Kakoune instead of using "execute-commands". Some projects use tmux, or maybe we can mock the terminal. --- src/input_handler.cc | 28 ++++++++++++++++++++++++++-- src/input_handler.hh | 1 + src/normal.cc | 6 +++--- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/input_handler.cc b/src/input_handler.cc index c134c3e9..70ceab44 100644 --- a/src/input_handler.cc +++ b/src/input_handler.cc @@ -788,6 +788,7 @@ public: m_prompt(prompt.str()), m_prompt_face(face), m_empty_text{std::move(emptystr)}, m_line_editor{context().faces()}, m_flags(flags), + m_was_interactive{not context().noninteractive()}, m_history{RegisterManager::instance()[history_register]}, m_current_history{-1}, m_auto_complete{context().options()["autocomplete"].get() & AutoComplete::Prompt}, @@ -1062,6 +1063,16 @@ public: } } + bool was_interactive() + { + return m_was_interactive; + } + + void set_was_interactive() + { + m_was_interactive = true; + } + DisplayLine mode_line() const override { return { "prompt", context().faces()["StatusLineMode"] }; @@ -1189,6 +1200,7 @@ private: LineEditor m_line_editor; bool m_line_changed = false; PromptFlags m_flags; + bool m_was_interactive; Register& m_history; int m_current_history; bool m_auto_complete; @@ -1197,7 +1209,7 @@ private: void history_push(StringView entry) { - if (entry.empty() or context().noninteractive() or + if (entry.empty() or not was_interactive() or (m_flags & PromptFlags::DropHistoryEntriesWithBlankPrefix and is_horizontal_blank(entry[0_byte]))) return; @@ -1710,6 +1722,12 @@ void InputHandler::set_prompt_face(Face prompt_face) prompt->set_prompt_face(prompt_face); } +bool InputHandler::history_enabled() const +{ + auto* prompt = dynamic_cast(¤t_mode()); + return prompt and prompt->was_interactive(); +} + void InputHandler::menu(Vector choices, MenuCallback callback) { push_mode(new InputModes::Menu(*this, std::move(choices), std::move(callback))); @@ -1760,7 +1778,13 @@ void InputHandler::handle_key(Key key) const bool was_recording = is_recording(); ++m_handle_key_level; - auto dec = on_scope_end([this]{ --m_handle_key_level; }); + auto dec = on_scope_end([this]{ + --m_handle_key_level; + if (m_handle_key_level == 0) + for (auto& mode : m_mode_stack) + if (auto* prompt = dynamic_cast(&*mode)) + prompt->set_was_interactive(); + }); auto process_key = [&](Key key) { if (m_last_insert.recording) diff --git a/src/input_handler.hh b/src/input_handler.hh index b7e53aaf..4a776e84 100644 --- a/src/input_handler.hh +++ b/src/input_handler.hh @@ -83,6 +83,7 @@ public: Face prompt_face, PromptFlags flags, char history_register, PromptCompleter completer, PromptCallback callback); void set_prompt_face(Face prompt_face); + bool history_enabled() const; // enter menu mode, callback is called on each selection change, // abort or validation with corresponding MenuEvent value diff --git a/src/normal.cc b/src/normal.cc index 01f56a01..e34c76d5 100644 --- a/src/normal.cc +++ b/src/normal.cc @@ -1016,7 +1016,7 @@ void select_regex(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.noninteractive()) + if (context.input_handler().history_enabled()) RegisterManager::instance()[reg].set(context, ex.str()); auto& selections = context.selections(); @@ -1038,7 +1038,7 @@ void split_regex(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.noninteractive()) + if (context.input_handler().history_enabled()) RegisterManager::instance()[reg].set(context, ex.str()); auto& selections = context.selections(); @@ -1142,7 +1142,7 @@ void keep(Context& context, NormalParams params) RegisterManager::instance()[reg].restore(context, saved_reg); if (event == PromptEvent::Abort) return; - if (not context.noninteractive()) + if (context.input_handler().history_enabled()) RegisterManager::instance()[reg].set(context, regex.str()); if (regex.empty() or regex.str().empty())