Commits e49c0fb04 (unmap: fail if the mapping is currently executing,
2023-05-14) 42be0057a (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes [1] [2].
For example with,
set -remove global autocomplete insert
hook global InsertCompletionShow .* %{
map window insert <esc> <c-o>
}
hook global InsertCompletionHide .* %{
unmap window insert <esc> <c-o>
}
The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).
Fix the regression by allowing map and unmap again while keeping the
mappings alive until they have finished executing.
Applying map/unmap immediately seems like the most obvious semantics.
Alternatively, we could apply them in between key presses.
[1]: <https://github.com/kak-lsp/kak-lsp/issues/689>
[2]: <https://github.com/alexherbo2/auto-pairs.kak/issues/60>
If during execution of a mapping, that same mapping is replaced,
there is undefined behavior because we destroy a mapping that we are
still iterating over.
I have been using this mapping inside my kakrc to re-source the kakrc.
map global user s %{:source "%val{config}/kakrc"<ret>} -docstring 'source "%val{config}/kakrc"'
Now <space>s happens to not trigger undefined behavior because the
mapping stays the same.
However it triggers an assertion added by Commit e49c0fb04 (unmap:
fail if the mapping is currently executing, 2023-05-14), specifically
the destructor of ScopedSetBool that guards mapping execution.
Fix these by banning map of a key that is executing, just like we
did for unmap.
Alternative solution: we could allow mapping (and even unmapping)
keys at any time and keep them alive by moving them into a trash can,
like we do for clients and others.
When a line only contains non-range atoms we can end-up accessing
past the end atom.
Add a test that shows the issue when run with valgrind, it is
unfortunately quite hard to trigger a crash because the invalidly
accessed byte usually leads to the correct code path being taken
(when != DisplayAtom::Range) so we have only 1 in 255 chance of
triggerring a crash.
Fixes#4927
In some cases such as with folding we can end-up with regions
not having any atoms to highlight which can trigger a crash as
we assume display buffers not to be empty
Fixes#4926
My terminal allows to map <c-[> and <esc> independently. I like
to use <c-[> as escape key so I have this mapping:
map global prompt <c-[> <esc>
Unfortunately, this is not equivalent to <esc>. Since mappings are
run with history disabled, <c-[> 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<c-[>
---
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<ret>
---
3. To check if it works for nested prompts, first set up this mapping.
map global prompt <c-j> '<a-semicolon>:nop should NOT be added to history<ret>'
map global prompt <c-h> '<a-semicolon>:nop should be added to history first'
Then type
:nop should be added to history second<c-j><c-h><ret><ret>
the inner command run by <c-j> 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.
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 :(
This switch makes show-matching fallback to the character preceeding
the cursor if the character under the cursor is not a matching
character, which should make show-matching more useful in insert mode.
Cache get fully invalidated whenever the regions change, so there
should be no risk of referencing a removed region, and this removes
one hash map lookup for every region in the displayed buffer range.
Range atoms should always appear in order, so we can iterate a single
time through the display lines and display atoms while applying
hightlighters to regions
When unmapping a key sequence that is currently executing, we continue
executing freed memory which can have weird effects. Let's instead
throw an error if that happens. In future we can support unmap in
this scenario.
Closes#4896
The current implementation only does this during regex operations,
but should be extensible to other operations that might take a long
time by regularly calling EventManager::handle_urgent_events().
Although Kakoune responds to modified mouse events, they show up in the
debug buffer corrupted. to_string() tests for equality on the mouse event
modifiers rather than testing just the relevant bits, so the modified
mouse events incorrectly fall through to the normal key handling.
Fix this and restructure to allow mouse events to be modifier-prefixed.
Signed-off-by: Chris Webb <chris@arachsys.com>
Change the initial <c-h>/<c-k> bindings to the recently freed-up
<a-u></a-U>.
Pros:
- easier to remember
- the redo binding is logical.
- works on legacy terminals, unlike <c-h>
Cons:
- It's less convenient to toggle between selection undo and redo
keys. I think this is okay since this scenario does not happen that
often in practice.
%exp{...} just expands its content the same way double quoted strings
do, but using a named expansion type makes it possible to use the
more quoting mechanism to avoid quoting hell.
When Kakoune's terminal is shown on my laptop monitor and I plug
in my external monitor, the terminal's workspace will move to that
external monitor. When this happens, Kakoune may segfault.
There are multiple resize events (SIGWINCH) in quick succession;
it crashes because we handle SIGWINCH during rendering.
The problem happens during execution of "TerminalUI::Screen::output"
(frame #18). When we receive SIGWINCH while writing to stdout, write(2)
fails with EAGAIN, prompting us to handle pending events (See ae001a1f9
(Run EventManager whenever writing to a file descriptor would block,
2022-05-10)). We update the screen size in check_resize() here:
#4 Kakoune::TerminalUI::check_resize (force=<optimized out>) at terminal_ui.cc:683
#5 Kakoune::TerminalUI::get_next_key () at terminal_ui.cc:719
#6 operator() (__closure=0x555555984198) at terminal_ui.cc:484
#7 std::__invoke_impl<void, Kakoune::TerminalUI::TerminalUI()::<lambda(Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode)>&, Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode> (__f=...) at /usr/include/c++/12.2.1/bits/invoke.h:61
#8 std::__invoke_r<void, Kakoune::TerminalUI::TerminalUI()::<lambda(Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode)>&, Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode> (__fn=...) at /usr/include/c++/12.2.1/bits/invoke.h:111
#9 std::_Function_handler<void(Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode), Kakoune::TerminalUI::TerminalUI()::<lambda(Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode)> >::_M_invoke(const std::_Any_data &, Kakoune::FDWatcher &, Kakoune::FdEvents &&, Kakoune::EventMode &&) (__functor=..., __args#0=..., __args#1=<optimized out>, __args#2=<optimized out>) at /usr/include/c++/12.2.1/bits/std_function.h:290
#10 std::function<void (Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode)>::operator()(Kakoune::FDWatcher&, Kakoune::FdEvents, Kakoune::EventMode) const (__args#2=<optimized out>, __args#1=<optimized out>, __args#0=...) at /usr/include/c++/12.2.1/bits/std_function.h:591
#11 Kakoune::FDWatcher::run (mode=Kakoune::EventMode::Urgent, events=<optimized out>) at event_manager.cc:28
#12 Kakoune::EventManager::handle_next_events (mode=mode@entry=Kakoune::EventMode::Urgent, sigmask=sigmask@entry=0x0, block=<optimized out>, block@entry=false) at event_manager.cc:143
#13 Kakoune::write (fd=1, data=...) at file.cc:273
#14 Kakoune::BufferedWriter<4096>::flush () at string.hh:236
#15 Kakoune::BufferedWriter<4096>::write (data="t file.hh:145
#16 Kakoune::TerminalUI::Screen::set_face (face=..., writer=...) at terminal_ui.cc:255
#17 operator() (line=..., __closure=<synthetic pointer>) at terminal_ui.cc:326
#18 Kakoune::TerminalUI::Screen::output (force=force@entry=true, synchronized=<optimized out>, writer=...) at terminal_ui.cc:402
#19 Kakoune::TerminalUI::redraw (force=force@entry=true) at terminal_ui.cc:571
#20 Kakoune::TerminalUI::refresh (force=<optimized out>) at terminal_ui.cc:592
#21 Kakoune::Client::redraw_ifn () at client.cc:282
#22 Kakoune::ClientManager::redraw_clients () at client_manager.cc:232
#23 Kakoune::run_server (session=..., server_init=..., client_init=..., init_buffer="fish-rust/src/ast.rs", init_coord=..., flags=Kakoune::ServerFlags::None, ui_type=Kakoune::UIType::Terminal,
debug_flags=<optimized out>, files=ArrayView<Kakoune::StringView> = {...}) at main.cc:893
#24 main (argc=<optimized out>, argv=<optimized out>) at main.cc:1243
Thereafter, "TerminalUI::Screen::output" resumes and crashes due to
a buffer overflow in "lines" which has been resized.
input_handler.cc:1476:16: error: alias template 'ConstArrayView' requires template arguments; argument deduction only allowed for class templates
insert(ConstArrayView{content});
^
input_handler.cc:1522:16: error: alias template 'ConstArrayView' requires template arguments; argument deduction only allowed for class templates
insert(ConstArrayView{str});
^
Whenever a new history node is committed after some undo steps, instead
of creating a new branch in the undo graph, we first append the inverse
modifications starting from the end of the undo list up to the current
position before adding the new node.
For example let's assume that the undo history is A-B-C, that a single undo
has been done (bringing us to state B) and that a new change D is committed.
Instead of creating a new branch starting at B, we add the inverse of C
(noted ^C) at the end, and D afterwards. This results in the undo history
A-B-C-^C-D. Since C-^C collapses to a null change, this is equivalent to
A-B-D but without having lost the C branch of the history.
If a new change is committed while no undo has been done, the new history
node is simply appended to the list, as was the case previously.
This results in a simplification of the user interaction, as two bindings
are now sufficient to walk the entire undo history, as opposed to needing
extra bindings to switch branches whenever they occur.
The <a-u> and <a-U> bindings are now free.
It also simplifies the implementation, as the graph traversal and
branching code are not needed anymore. The parent and child of a node are
now respectively the previous and the next elements in the list, so there
is no need to store their ID as part of the node.
Only the committing of an undo group is slightly more complex, as inverse
history nodes need to be added depending on the current position in the
undo list.
The following article was the initial motivation for this change:
https://github.com/zaboople/klonk/blob/master/TheGURQ.md
