It turns out that neither <a-f> or <a-t> make sense when run at the
beginning of the buffer. When I first created the check, I thought
that <a-f> made sense if the character under the cursor was the
character being searched for. I was wrong, <a-f> should always go
at least one character backwards.
This commit fixes a bug in Buffer::advance where it would first access
m_lines[-1], and then check whether or not that access would have
segfaulted. This commit moves the check to before the segfault would
occur.
is run as
> ./kak -f '2oK.k<c-n><ret><c-n>' /dev/null
The crash occurs because when <c-n> is pressed for the second time,
it attempts to use m_completions from the first press of <c-n>.
This only happens when kakoune is run with -f, because when this is done
interactively, there is a client, which means that m_completions gets
reset. This removes the check that causes that difference.
I am not *completely* sure that this is the best way to solve the
problem, since I am not completely sure why that check was put there
in the first place.
Using BufferIterator adds overhead, but we know that DisplayAtoms
cannot span multiple buffer lines and hence we can directly iterate
using char pointers.
Each draft context gets its own private copy of the selections.
Any selection changes will be thrown away when the draft context
is disposed. Since selection-undo is only supported as top-level
command, it can never be used inside a draft context, so let's stop
recording it.
No functional change.
Calculating the length of an atom means we need to decode every
codepoint and compute its column width. This can prove quite expensive
in trim_from as we can have full buffer lines, so on buffer with long
lines we might have to go through megabytes of undisplayed data.
Pass the first buffer on the the command line explicitely to client
creation. This ensure the buffer list matches the command line, which
makes buffer-next/buffer-previous a bit more useful.
Fixes#2705
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
Comparing iterators between buffers should never happen, and the
only place we did was with default constructed BufferIterator which
we replace by casting the iterator to bool.
This should improve performance on iterator heavy code.
When the file system runs out of space, "write -force" will fail but
doesn't print "No space left on device".
Let's fix this by including such an underlying error. Untested.
Backstory: I alias "w" to a command that runs "write -force %arg{@}".
so I can overwrite files that already exist outside the editor (I
should probably get used to the new behavior).
Commit 69053d962 (Use menu behavior when completing change-directory,
2022-07-19) made ":cd dir/" actually run ":cd dir/first-subdir",
which can be surprising.
This is usually irrelevant because you rarely type the trailing slash.
However it does happen after correcting an error with `<backspace>`
and friends. For for example,
:cd d<tab>/f<backspace>
results in
:cd dir/
We should probably fix user expectations here. Do this by adding "dir/"
as valid completion. This requires us to allow empty candidates in
"RankedMatch" but there's no harm in that. This means we need to
filter out empty completions from shell-script-candidates elsewhere.
Alternative fix: we could revert 69053d962. This would remove the
convenient menu behavior but that wouldn't be a huge deal.
Fixes#4775
Mapping upper case keys is legitimate, for exampled so that they behave
the same as a lower case mapping. The current rejection of those mappings
is a misguided attempt to prevent mapping *to* upper case keys as those
will never get triggered.
Closes#4769
After a failed
:write file-that-already-exists
a user might want to type ":<up> -f<ret>" to force-overwrite.
This doesn't work because :write's switches must precede the filename.
It's dual :edit does not have this restriction.
Some commands require switches to precede positional arguments for a
good reason; for example because positional arguments might start with
"-" (like ":echo 1 - 1").
There seems to be no reason for the :write restriction, so remove
it. Same for :enter-user-mode.
Thanks to alexherbo2 for reporting.
Running %sYeti<ret>casdf on file
[example.journal.txt](https://github.com/mawww/kakoune/issues/4685#issuecomment-1193243588)
can cause noticeable lag. This is because we insert text at 6000
selections, which means we need to update highlighters in those lines.
The runtime for updating range highlighters is quadratic in the
number of selections: for each selection, we call on_new_range(),
which calls add_matches(), which calls std::rotate(), which needs
needs linear time.
Fix the quadratic runtime by calling std::inplace_merge() once instead
of repeatedly calling std::rotate(). This is works because ranges
are already sorted.
I used this script to benchmark the improvements.
(In hindsight I could have just used "-ui json" instead of tmux).
#!/bin/sh
set -ex
N=${1:-100}
kak=${2:-./kak.opt}
for i in $(seq "$N")
do
echo -n "\
2022-02-06 * Earth
expense:electronics:audio 116.7 USD
liability:card -116.7 USD
2022-02-06 * Blue Yeti USB Microphone
expense:electronics:audio 116.7 USD
liability:card -116.7 USD
"
done > big-journal.ledger
echo > .empty-tmux.conf 'set -sg escape-time 5'
test_tmux() {
tmux -S .tmux-socket -f .empty-tmux.conf "$@"
}
test_tmux new-session -d "$kak" big-journal.ledger
test_tmux send-keys '%sYeti' Enter c 1234567890
sleep .2
test_tmux send-keys Escape
while ! test_tmux capture-pane -p | grep 123
do
sleep .1
done
test_tmux send-keys ':wq' Enter
while test_tmux ls
do
sleep .1
done
rm -f .tmux-socket .empty-tmux.conf
This script's runtime used to grow super-linearly but now it grows
linearly:
kak.old kak.new
N=10000 1.142 0.897
N=20000 2.879 1.400
Detailed results:
$ hyperfine -w 1 './bench.sh 10000 ./kak.opt.'{old,new}
Benchmark 1: ./bench.sh 10000 ./kak.opt.old
Time (mean ± σ): 1.142 s ± 0.072 s [User: 0.252 s, System: 0.059 s]
Range (min … max): 1.060 s … 1.242 s 10 runs
Benchmark 2: ./bench.sh 10000 ./kak.opt.new
Time (mean ± σ): 897.2 ms ± 19.3 ms [User: 241.6 ms, System: 57.4 ms]
Range (min … max): 853.9 ms … 923.6 ms 10 runs
Summary
'./bench.sh 10000 ./kak.opt.new' ran
1.27 ± 0.09 times faster than './bench.sh 10000 ./kak.opt.old'
$ hyperfine -w 1 './bench.sh 20000 ./kak.opt.'{old,new}
Benchmark 1: ./bench.sh 20000 ./kak.opt.old
Time (mean ± σ): 2.879 s ± 0.065 s [User: 0.553 s, System: 0.126 s]
Range (min … max): 2.768 s … 2.963 s 10 runs
Benchmark 2: ./bench.sh 20000 ./kak.opt.new
Time (mean ± σ): 1.400 s ± 0.018 s [User: 0.428 s, System: 0.083 s]
Range (min … max): 1.374 s … 1.429 s 10 runs
Summary
'./bench.sh 20000 ./kak.opt.new' ran
2.06 ± 0.05 times faster than '../repro.sh 20000 ./kak.opt.old'
LineRangeSet::add_range() calls Vector::erase() in a loop over the
same vector. This could cause performance problems when there are many
selections. Fix this by only calling Vector::erase() once. I didn't
measure anything because my benchmark is dominated by another issue
(see next commit).
LineRangeSet::remove_range() also has a suspicious call to erase()
but that one is only used in test code, so it doesn't matter.
From the issue:
> It often happens to me that I carefully craft a selection with multiple
> cursors, ready to make changes elegantly, only to completely mess it
> up by pressing a wrong key (by merging the cursors for example). Being
> able to undo the last selection change (even if only until the previous
> buffer change) would make this much less painful.
Fix this by recording selection changes and allowing simple linear
undo/redo of selection changes.
The preliminary key bindings are <c-h> and <c-k>.
Here are some other vacant normal mode keys I considered
X Y
<backspace> <minus>
# ^ =
<plus> '
unfortunately none of them is super convenient to type. Maybe we
can kick out some other normal mode command?
---
This feature has some overlap with the jump list (<c-o>/<c-i>) and
with undo (u) but each of the three features have their moment.
Currently there's no special integration with either peer feature;
the three histories are completely independent. In future we might
want to synchronize them so we can implement Sublime Text's "Soft
undo" feature.
Note that it is possible to restore selections that predate a buffer
modification. Depending on the buffer modification, the selections
might look different of course. (When trying to apply an old buffer's
selection to the new buffer, Kakoune computes a diff of the buffers
and updates the selection accordingly. This works quite well for
many practical examples.)
This makes us record the full history of all selections for each
client. This seems wasteful, we could set a limit. I don't expect
excessive memory usage in practice (we also keep the full history of
buffer changes) but I could be wrong.
Closes#898
To be able to undo selection changes, we want to record selections
from all commands that modify selections. Each such command will get
its own private copy of the selections object.
This copy will live until the command is finished executing.
All child commands that are run while the command is executing,
will also use the same copy, because to the user it's all just one
selection change anyway.
Add an RAII object in all places where we might modify selections.
The next commit will use this to create the private selections copy
in the constructor (if there is none) and remove redundant history
items in the destructor.
We could avoid the RAII object in some places but that seems worse.
For lifetimes that don't correspond to a lexical scope, we use a
std::unique_ptr. For lambdas that require conversion to std::function,
we use std::shared_ptr because we need something that's copyable.
The next commit changes the selections to a history of
selections. Today we directly access the selections data member. Let's
instead use an accessor method, to reduce the number of changes in
the next commit.
clang/clangd complain about the new HashSet type:
hash_map.cc:98:20: warning: braces around scalar initializer [-Wbraced-scalar-init]
set.insert({10});
^~~~
The argument to HashSet<int>::insert is just an int, so we don't
need braces. Only an actual HashMap would need braces to construct
a HashItem object.
When passing a filename parameter to "write", the -force parameter
allows overwriting an existing file.
The "write!" variant (which allows writing files where the current
user does not have write permissions) already implies -force.
All other variants (like write-quit or write-all) do not take a
file parameter.
Hence -force is relevant only for "write". Let's hide it from the
autoinfo of the other commands.
It's difficult to avoid duplication when constructing the constexpr
SwitchMap because String is not constexpr-enabled. Today, all our
SwitchMap objects are known at compile time, so we could make SwitchMap
use StringView to work around this. In future we might want to allow
adding switches at runtime, which would need String again to avoid
lifetime issues.
Instead of storing regexes in each regions, move them to the core
highlighter in a hash map so that shared regexes between different
regions are only applied once per update instead of once per region
Also change iteration logic to apply all regex together to each
changed lines to improve memory locality on big buffers.
For the big_markdown.md file described in #4685 this reduces
initial display time from 3.55s to 2.41s on my machine.
When I wrote this line I wanted to avoid adding the array size but
I didn't know about make_array().
I had unsuccessfully tried some alternatives, for example
Array{"a", "b", "c"}
which doesn't work because we need StringView (c.f. git blame on
this line)
also
Array<StringView>{"a", "b", "c"}
doesn't work because it's missing a template argument.
This makes the function easier to find for newcomers because
to_string() is the obvious name. It enables format() to do the
conversion automatically which seems like good idea (since there is
no other obvious representation).
Of course this change makes it a bit harder to grep but that's not
a problem with clang tooling.
We need to cast the function in one place when calling transform()
but that's acceptable.
Commit d470bd2cc (Make numeric registers setable, 2017-02-14) removed
the user-provided StaticRegister::operator= in favor of a set()
member function, so this comment is no longer valid.
Recently, switch completion were given the menu behavior.
Unfortunately this breaks cases like
:echo -- -mark<ret>
where the hypothetical user wanted to actually display "-mark", not
"-markup".
Simply bail if there is a double-dash. This is not fully correct,
for example it wrongly disables switch completion on
echo -to-file -- -
but that's minor, we can fix it later.
In future, we should reuse the ParametersParser when computing completions,
which will obsolete this workaround.
Commit 217dd6a1d (Disable history when executing maps, 2015-11-10)
made it so with
map global normal X %{:echo 123<ret>}
X does not add to prompt history (%reg{:}).
Unfortunately this behavior was not extended to mappings in the "user"
keymap, nor to mappings in custom user modes.
In my experience, not adding to history is almost always the expected
behavior for mappings. Most users achieve this by adding a leading space:
map global user X %{: echo 123<ret>}
but that's awkward. We should have good defaults (no nnoremap)
and map should work the same way across all modes.
Fix this by also disabling history when executing user mappings. This
is a breaking change but I think it only breaks hypothetical scenarios.
I found some uses where user mappings add to history but none of them
looks intentional.
f702a641d1/.config/kak/kakrc (L169)604ef1c1c2/kakrc (L96)d22e7d6f68/kak/kakrc (L71)https://grep.app/search?q=map%20%28global%7Cbuffer%7Cwindow%29%20user%20.%2A%5B%21%3A/%5D%5B%5E%20%5D.%2A%3Cret%3E®exp=true
This allows to select completions without pressing Tab.
There are two different obvious ways to add the menu bit.
1. When creating the "Completions" object, pass the
Completions::Flags::Menu parameter.
2. If there is a completer function like "complete_scope", wrap
it, e.g. "menu(complete_scope)".
I have settled on always using 2 if there is a completer function
and 1 otherwise.
The advantage of 2 over 1 is that it allows to use the completer
function in a context where we don't want the menu behavior
(e.g. "complete-command").
---
Now the only* completion type where we usually don't use menu behavior
is file completion. Unfortunately, menu behavior has poor interaction
with directories' trailing slashes. Consider this (contrived) example:
define-command ls -docstring "list directory contents" -params .. %{
echo -- %sh{ls "$@"}
}
complete-command -menu ls file
Run ":ls kakoun<ret>". The prompt expands to ":ls kakoune/"
before executing. Next, run ":<c-p>". This recalls ":ls kakoune/"
and immediately selects the first completion, so on validation,
the command will be ":ls kakoune/colors/", which is weird.
[*] Also, expansions like %val{bufname} also don't use menu
behavior. It wouldn't add value since validation doesn't add a
closing delimiter. I have an experimental patch that adds closing
delimiters automatically but I'm not sure if that's the right
direction.
This makes "cd<space><ret>" change to the first completion,
not to $HOME. This might be surprising but it should make sense.
I don't have a concrete motivation but this should save a Tab press
in some scenarios.
We allow to abbreviate scopes ("set g" means the same thing as "set
global") but that feature is a bit obscure. Users might figure out the
menu completion behavior faster, so let's maybe use it here as well?
Not really attached to this but it enables the next commit to use
menu() for completing scopes.
This refactoring is possible because we always have
params[token_to_complete].length() == pos_in_token
---
Instead of three separate functions, I originally tried to add
template arguments to complete_scope(). That worked fine with g++
12.1 but clang 14.0 complained when wrapping a menu() around a
complete_scope() that relied on defaulted template arguments:
commands.cc:1087:20: error: no matching function for call to 'menu'
make_completer(menu(complete_scope), menu(complete_hooks), complete_nothing,
^~~~
commands.cc:116:6: note: candidate template ignored: couldn't infer template argument 'Completer'
auto menu(Completer completer)
^
On a command prompt like
"set-option -remove buffer aligntab "
we fail to show the aligntab-specific info . Fix this by skipping a
leading -remove, just like we skip -add.
Add an explicit specialization of contains() because otherwise I'd
need to write something like
contains(Array{"-add", "remove"}, param)
This gives the "prompt" command the same "-menu" switch as
"complete-command" and "define-command". I don't think anyone has
asked for it but it seems like a good idea?
Both "define-command" and "prompt" use the same logic, so share
it. This will make it easy to implement "prompt -menu".
This reveals a problem with PromptCompleterAdapter: when converting
it to std::function and then to bool, it always evaluates to true
because it has an operator(). However, it should evaluate to false
if the adaptee holds no valid function (e.g. is a default-constructed
std::function). Otherwise we try to call a non-existant function. Tweak
PromptCompleterAdapter to work for empty inputs.
If I type
:echo -mx
I get no completions, even when I move the cursor on the x.
If I replace the x with a k, I get a completion "-markup".
The odd thing is that if I accept the completion while the cursor is
on the k, then the commandline will be
:echo markupk
Evidently, the characters under cursor (x/k) influence the completion
(actually all letters right of the cursor do), but they are not
incorporated in the final result, which is weird.
I think there are two consistent behaviors:
1. Compute completions only from characters left of the cursor. We already
do this in insert mode completion, and when completing the command name
in a prompt.
2. Compute completions from the whole token, and when accepting a completion,
replace the whole token.
Most readline-style completion systems out there implement 1. A
notable exception is fish's tab-completion. I think we should stick
to 1 because it's more predictable and familiar. Do that.
This allows us to get rid of a special case for completing command
names, because the new behavior subsumes it.
In fact, I think this would allow us to get rid of most "pos_in_token"
or "cursor_pos" completer parameters. I believe the only place where we
it's actually different from the end of the query is in "shell-script"
completers, where we expose "$kak_pos_in_token". I think we can still
remove that parameter and just cut the commandline at the cursor
position before passing it to a "shell-script" completer. Then we
also don't need "$kak_token_to_complete" (but we can still keep
expose them for backwards compatibility).
Just like in the parent commit, this requires us to use a non-owning
type. Technically, these strings all benefit from SSO, so there is
no lifetime issue, but we can't deduce that from the types.
I guess we could use InplaceString just as well.
This means that typing
:add-highlighter g c 80
results in
:add-highlighter global/ column 80
Paths for add-highlighter do not get the menu behavior because we
want to be able to type "global/foo" even if "global/foobar" exists.
complete() merely iterates over its input, so we can pass it a range
instead of a vector. For some reason, this requires specifying the
type of the static array, or else its elements become String which
triggers this assertion:
static_assert(not std::is_same<decltype(*begin(container)), String>::value,
"complete require long lived strings, not temporaries");
Specify the type with an explicit Array<StringView, 8>. This is
pretty ugly but the alternative of appending "_sv" to every single
array element seems worse?
If we modify the vector of user modes while complete() is iterating
over, we could crash -- but that scenario is impossible since both
only happen inside the single-threaded server process.
We already use the menu behavior in complete_command_name(); let's do
the same for switches, since we can complete all valid inputs and
it can save a Tab key in some scenarios.
CommandManager::complete is fairly complex. We can't expect callers
to add the menu bit when appropriate, so we currently do it here.
The next commit will give switch completions the menu behavior, so this
is necessary so we can still type "echo --" without an auto-expansion
to "echo -to-file".
"complete_alias_name" is a better name then "complete_alias" because
it's consistent with more similar names, which are:
complete_client_name
complete_command_name
complete_module_name
complete_option_name
complete_register_name
complete_scope
complete_face
We define
using PromptCompleter = std::function<Completions (const Context&, CompletionFlags,
StringView, ByteCount)>;
Some command completers are *almost* convertible to PromptCompleter;
the only difference is the string type of the prefix argument.
The later commits in this series want to use menu() on the
completers. Enable this by harmonizing the types.
