Commit 53d9b9b67 (Escaping tweak in git.kak, 2024-02-06) broke
blame-jump when the commit subject contains a single quote.
(Also on unbalanced "{" which is a rare edge case but we already have
it in our Git history.)
git.kak assumes that filenames don't contain ' or unbalanced {,
but we can't really make that assumption about people's names or
commit subjects.
Unfortunately the escaping here is very messy. We need to pass
arbitrary text to callbacks; maybe we should have closures that can
capture private temporary registers.
Internally, all lines have a trailing "\n".
Buffers created empty (like fifo buffers) start with a single line.
When reading data into fifo buffers, we insert *before* the last line's
trailing newline ("last newline"). This enables autoscrolling (enabled
with "edit -scroll") as long as the cursor is on the last newline.
When autoscrolling is disabled, we have a special case to insert
*after* the last newline. This means that a cursor on that newline
won't be moved. Then we transplant the newline character from the
beginning to the end of the buffer. This special case happens only on
the very first fifo read; on subsequent reads, the cursor at position
1.1 will not be moved anway because insertions happen below 1.1.
Since we always insert (effectively) before the last newline, fifo
buffers have a trailing empty line.
For autoscrolling buffers this seems correct; it gives users an
obvious way to toggle autoscrolling.
For non-scrolling buffers the newline is redundant. Remove it.
This requires keeping track of whether the last newline comes from
the fifo, or was added by us. The shortest fix I could find
is to always append to the buffer if not scrolling, and then delete
the added newline character if applicable.
m_buffer.insert(m_scroll ? pos : m_buffer.next(pos), StringView(data, data+count));
if (not m_scroll and not m_had_trailing_newline)
m_buffer.erase(pos, m_buffer.next(pos));
maybe that's the best fix overall; but erasing at the end seems better
than erasing in the middle, so do that whenever possible.
Reported in https://lists.sr.ht/~mawww/kakoune/%3CZbTK7qit9nzvrMkx@gmail.com%3E
When "edit -fifo" reads data without a trailing newline, the fifo
buffer will not have a trailing blank line. But if there is a trailing
newline, we will get a trailing blank line. This is weird because the
trailing blank line exists for scrolling, it should not be determined
by the data read.
Add a test case to demonstrates the inconsistency which is fixed by
the next patch.
Patches as produced by "git format-patch" have a trailing signature
that is separated from the body by a line with "-- " on it. By default
it contains the Git version. We erroneously include this signature
in the diff we pipe to patch, which fails to apply as a result.
Add a targeted fix to suppress these signatures.
Since :patch transforms its inputs into context-only lines, we can
easily get into a state where a file diff has only context lines.
git apply does not accept a "diff" without any hunk, so let's skip
that.
This test doesn't care about testing things like "if I send the next
key before we have finished reacting to the previous ones, nothing
bad ever happens".
Hence we can until exhaustion after each input. This should fix
bespoke flakiness. The handling of "c<esc>" should be atomic.
This reasoning probably applies to most tests; waiting for all events
seems like the safest and easiest approach overall (compared to sleep
or sleep-until). The downside is that the tests need changes when
UI code changes but it rarely does, and if it does we can automate
the updates.
Closes#5073
Read output from the script as it comes and update the candidate
list progressively.
Disable updating of the list when a completion has been explicitely
selected.
ensure cursor is visible after user input except if the command
implementation opted-out. Hooks and timers should not enforce
visible cursor.
PageUp/PageDown and `<c-f>` / `<c-b>` commands still move the cursor
as this seemed a desired behaviour.
This is currently broken on various corner cases and breaks the
"master branch should be good for day to day work" implicit rule,
ongoing work to stabilize this feature will take place on the
no-cursor-move-on-scroll branch until its deemed ready.
This reverts commit 1e38045d70.
Closes#4963
Kakoune now does not touch cursors when scrolling. It checks
if either the buffer or selections has been modified since
last redraw.
Fixes#4124Fixes#2844
The current exponential behaviour does not seem that useful, it seems
more predictible that pressing `+` twice would end up with 3 copies
of the original selections instead of 4.
Fixes#4533
Do not rely on timing but wait for Kakoune to redraw which is what
we actually need: Mouse clicks rely on the current display buffer
to resolve the buffer location, so we need to wait for a redraw
to happen with `ui_out`
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
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
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.
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
The previous code was assuming it was fine to push_next without
growing, which used to be the case with the previous implementation
because we always have poped the current thread that we try to push.
However now that we use a ring-buffer, m_next_begin == m_next_end can
either mean full, or empty. We solve this by assuming it means empty
and never allowing the buffer to become full, which means we need
to grow after pushing to next if we get full.
Fixes#4859
The macOS CI manges to trigger this race. When it happens the
"c" inserted by the last command is not seen by the test runner.
Let's fix this by adding yet another sleep.
Recent changes for selection-undo added an assertion that triggers
when a mouse-drag overlaps with an insert mode, because both events
record selection history. However this is actually fine. The one
that finishes last concludes the selection edition, while the other
one will be a nop.
The test could be simpler (i.e. not require sleeps) but I figured it
doesn't hurt add this since we don't have any comparable tests.
After buffer modification - in particular after deletion - adjacent
selection history entries may correspond to the same effective
selection when applied to the current buffer. This means that we
sometimes need to press <c-h> multiple times to make one visible
change. This is not what the user expects, so let's keep walking the
selection history until we hit an actual change.
Alternatively, we could minimize the selection history after buffer
changes but I think that would make the it worse after content
undo+redo.
Each selection undo operation is surrounded by pair of
begin_edition()/end_edition() calls.
The original reason for adding these was that in one of my preliminary
versions, a WinDisplay hook could break an undo chain, even if the
hook did not affect selections at all. This has since been fixed.
By surrounding the undo with begin_edition()/end_edition(), try to
ensure that any selection modification that happens in a WinDisplay
hook would not break the undo chain. Essentially this means that,
after using <c-h> to undo a buffer change, this was meant to
make sure that <c-k> could redo that buffer change.
However, it turns out this actually doesn't work. The attached test
case triggers an assertion. As described in the first paragraph,
the only real-world motivation for this is gone, so let's simplify
the behavior.
The assertion fix means that we can test the next commit better.
With overlapping selections, pasting after breaks assumption of
SelectionList::for_each as our changes are no longer happening in
increasing locations.
We hence cannot rely on the ForwardChangeTracker in that case and
have to rely on the more general (and more costly) ranges update logic.
This interacts poorly with paste linewise pastes and we try to preserve
the current behaviour by tracking the last paste position.
Overall, this change really begs for overlapping selections to be
removed, but we will fix them like that for now.
Fixes#4779
