vendor termkey - #1353
Merged
Merged
Conversation
rnpnr
force-pushed
the
termkey
branch
2 times, most recently
from
May 18, 2026 21:51
9631e89 to
1b99c2b
Compare
rnpnr
force-pushed
the
termkey
branch
3 times, most recently
from
May 24, 2026 14:22
549a1d5 to
16e50d8
Compare
2 tasks
Collaborator
Author
|
So having thought about it quite a lot I think we should just remove support for system termkey. It no longer receives updates and I could not find any relevant patches to the source code after looking at a number of different libtermkey packages in various distos/BSDs. In Gentoo Linux the only package with a dependency on libtermkey is vis, it seems to be the same in Arch. Along with the fact that this gets optimized inline with the rest of vis, the other changes I have made will already offer a better experience than the normal termkey. If I don't need to bother with API compatibility I can make further improvements. Once I am done removing system termkey support and making one immediate change to avoid mallocing a termkey struct I will test for a few more days and then I will merge this as I have not received any feedback. I am fully confident that this is the right move going forward. |
rnpnr
force-pushed
the
termkey
branch
4 times, most recently
from
May 31, 2026 13:17
0fc30d8 to
7dcbb9e
Compare
This is modified from the neovim version to compile as a single file. All functions which are not needed by termkey are removed. Everything else is namespaced to unibi_.
rnpnr
force-pushed
the
termkey
branch
3 times, most recently
from
June 3, 2026 12:28
46bd975 to
e193227
Compare
As discussed in martanne#79, martanne expressed his refusal for a vendored libtermkey, however, there are distributions that do not want to package an EOL or deprecated library, hence, this patch, similar to Neovim, vendors libtermkey into the source tree, while still allowing distributors to use the system libtermkey by default, if detected. NOTE(rnp): termkey was amalgamated into a single file, all exported symbols were marked TERMKEY_EXPORT, all code unused by vis was deleted, and all other code was namespaced as "termkey_"/"TermKey". closes: martanne#1071 Co-authored-by: Randy Palamar <randy@rnpnr.xyz>
termkey already had other functions which weren't exported in the library so I don't know why these needed to be treated specially. The person responsible was probably too C++ brained.
no need for this to store pointers to global data, the global data can just be stored directly in this table.
termkey isn't meant to have multiple instances in a single program. there is lots of global data which precludes it from such use. even if that wasn't there we do not care about multiple instances in vis. Instead of allocating DriverNodes and per driver context structs it can all just be stored in the global drivers table. By adding a TermKeyDriver::next termkey can still only link in the active drivers, but now there is no chance of failure due to memory allocation.
no one reads from or stores to these after unibi_from_mem so they have no reason to exist.
the place this table is accessed from is through the unibi_name_str() function which always asks for the long name. therefore we shouldn't waste executable size storing a bunch of unused strings (roughly 4K).
This is just a copy of a statically available table. No one is adding to it at runtime so just use the static table directly.
It's size is never changed at runtime so it should just be allocated as part of TermKey.
The CSI driver cannot fail to start so we will always have at least one driver.
Along with the fact that vendored termkey gets optimized inline with the rest of vis, the other changes I have made will already offer a better experience than normal termkey. If I don't need to bother with API compatibility I can make further improvements. I have looked into a number of packaged libtermkeys and none of them have any relevant changes to the source code. In Gentoo Linux the only package with a dependency on libtermkey is vis and it seems to be the same in Arch. If we remove it as a dependancy then they can also remove from their trees.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a continuation of #1241 which fixes the issue of always needing curses for embedded termkey by using unibilium (also included) to provide standalone terminfo parsing. On its own unibilium is quite small but I removed roughly 50% since it was not needed by termkey. I then made a number of changes to both libraries to generally improve them for their use here (reduce memory allocations, reduce pointer chasing, etc.).
Since this is a continuation I kept the ability for vis to be built against the system's termkey but I kind of want to remove that and make this the only option. I have a few changes in mind to further reduce memory allocations and make them more suitable for vis but I don't think I can do so while maintaining the backwards compatibility with their existing APIs. Since neither of these libraries are receiving updates I feel like its better to go all in on the included versions and change them in whatever way is most optimal for vis. What are other peoples thoughts on this?
Note I have tested this on a couple different terminal emulators (st, alacritty) and the CI is handling it fine but testing on more terminal emulators wouldn't hurt. If you run a different terminal emulator please test and let me know if it is working. I'm particular interested in wayland only emulators (like foot) and whatever people are using on macOS nowadays since these are the ones that tend to cause issues.