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
Instead of triming only buffer ranges, add a trim_from method to
display line to keep the initial N columns, we know how many columns
are used by non-trimable widgets in DisplaySetup::widget_columns so
we can just pass this.
Also restore the previous logic for face merging
Fixes#4670
Make the column highlighter faces final, and change final logic to
give precedence to the base face when both the base and new face are
final.
Fixes#4669
Always start with full buffer lines and trim the display buffer at
the very end, treat non-range display atoms as non-trimable in that
case and keep track of how many columns are occupied by "widgets"
such as line numbers or flags.
Fixes#4659
`x` is often criticized as hard to predict due to its slightly complex
behaviour of selecting next line if the current one is fully selected.
Change `x` to use the previous `<a-x>` behaviour, and change `<a-x>` to
trim to fully selected lines as `<a-X>` did.
Adapt existing indentation script to the new behaviour
The canonical way to disable all auto-insertion hooks is
set-option global disabled_hooks .*-insert
A recent change allowed to disable hooks that insert ) and }
independent of hooks that insert // (a step in the right
direction, we should do it for more filetypes).
Since the new hook ("go-insert-closing-delimiter") doesn't match
.*-insert, it broke the above snippet. Fix this by renaming it to
"go-closing-delimiter-insert".
This makes it a bit less obvious how to disable only comment insertion.
Not sure if there's interest in that, but make it easier by renaming
"go-insert" to "go-comment-insert".
Changed the indentation behavior such that an extra level of
indentation is added after a line containing a ( or { that is
not closed on the same line instead of aligning to the unclosed
( or {. This is consistent with how `go fmt` formats source code.
Added regression tests.
