Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make code sent by backspace key configurable #23

Merged
merged 2 commits into from
Jul 21, 2022
Merged

Conversation

viccie30
Copy link
Contributor

Keep the default code as ASCII backspace, but allow setting the code to
ASCII delete. This is the code used by the Linux console and most modern
terminal emulators by default.

Fixes #22.

@Aetf
Copy link
Owner

Aetf commented Jul 13, 2022

I agree this is something we should fix. I want to give this a little more thinking though. I'm sure there will be more similar quirks in keycodes. Adding options for each of those will quickly become unwieldy.

My gut feeling is that a general keybinding system could be an elegant solution, which should also allow potentially other custom shortcuts. But maybe that's too much. Anyway, will take another look at this in the weekend. Meanwhile, feel free to propose other ideas :)

@viccie30
Copy link
Contributor Author

viccie30 commented Jul 13, 2022

I thought about that, but xkb already takes care of that for most "regular" keys. I don't know if there's many people wanting to remap the tab key, for example, but maybe there are.

I'd be happy to code up another more general suggestion if you prefer to go that way.

@Aetf
Copy link
Owner

Aetf commented Jul 13, 2022

I thought about that, but xkb already takes care of that for most "regular" keys. I don't know if there's many people wanting to remap the tab key, for example, but maybe there are.

Yeah I feel the same way.

I'd be happy to code up another more general suggestion if you prefer to go that way.

What do you have in mind? If that's not too much work then go ahead. Otherwise let me take a look at the codebase this weekend and we can discuss whether that really worths it.

@viccie30
Copy link
Contributor Author

I'd be happy to code up another more general suggestion if you prefer to go that way.

What do you have in mind? If that's not too much work then go ahead. Otherwise let me take a look at the codebase this weekend and we can discuss whether that really worths it.

There's two main directions, I think. Either make all keysyms remappable or only the "special" ones, for some definition of special.

In both approaches there's also the issue of what to do when Shift/Alt/Ctrl are pressed. And what to do about some really special keys with important special semantics, like the different codes for the keypad numbers in application mode.

I don't really have the answer to that. Maybe I can have a look around at some other implementations: xterm, konsole, vte, see what they do.

@viccie30
Copy link
Contributor Author

All right, I've looked into this today:

xterm's upstream default is to have the "backarrow key" send backspace (010), this is set in the backarrowKey resource. Separately from that the ptyInitialErase resource determines whether xterm sets VERASE in the pseudoterminal settings from its termcap/terminfo entry. This termcap/terminfo entry also has backspace by default.
Completely changing xterm to send delete (0177) thus involves changing both the actual key sent with the backarrowKey resource or alternatively through the translations mechanism. Besides that the erase character in the pseudoterminal needs to be changed by either setting it directly with the ttyModes resource or through changing the termName resource to select a different termcap/terminfo entry.

Fedora and Arch Linux use this upstream default, Debian and Ubuntu change the upstream default and use delete instead.

Gnome's VTE makes both backspace and delete selectable. The default for backspace is to send the VERASE character of the active terminal and backspace (010) if that's not set or not readable. The default for delete is to send the termcap/terminfo entry kD, usually \E[3~. Both keys can also manually be set to backspace, delete or \E[3~. Debian does not patch any defaults, but disables the gnome-pty-helper that sets the VERASE charachter, meaning it stays at LInux's 0177 default. Fedora does build gnome-pty-helper, thus changing the VERASE character to 010. Arch Linux's version is newer and lacks the helper, so presumably it has backspace send the default 0177 as well.

Konsole's default keyboard layouts make backspace send delete, but this can be changed by setting another keyboard layout.

@Aetf
Copy link
Owner

Aetf commented Jul 18, 2022

Now thinking about this, I want to keep the current behavior (sending 010 for backspace) non-configurable because that keeps the codebase simple, given

  • the behavior is pretty common (xterm upstream default, vte default) and doesn't have other major problems,
  • regarding the originating issue about /bin/login on Debian, IIUC using agetty solves the problem.

I'm actually thinking about making agetty the default. This has the additional benefit of supporting /etc/issue (Aetf/kmscon#24) with no code change and a cleaner separation of concern.

Is there any other concerns that I'm missing?

@viccie30
Copy link
Contributor Author

I see no real problem with that. Because kmscon sets $TERM to xterm-256color and xterm's terminfo entry sets backspace to send 0177, I will just have to patch libtsm and kmscon in Debian.

It's only two one-line patches, so that's not too much of a burden for me.

@Aetf
Copy link
Owner

Aetf commented Jul 18, 2022

Ah, maybe kmscon shouldn't report as xterm-256color then. Let me research what the options are.

@viccie30
Copy link
Contributor Author

kmscon/libtsm could ship their own terminfo entries, but that would not work when ssh'ing into a different machine, for example. Also, I hope that libtsm will get more features in the coming time, so the terminfo entry would need to be updated every time something mentioned in the entry changes.

The best option in my eyes would be to work towards better xterm compatibility, so the current lie slowly turns into truth.

@Aetf
Copy link
Owner

Aetf commented Jul 18, 2022

Just to make sure I understand correctly, debian and Ubuntu change the xterm default to send \177 for backspace, and also the matching terminfo database, so kbd=^H. And on Archlinux and those didn't change upstream default, kbs=^?.

What term does vte report on debian?

@viccie30
Copy link
Contributor Author

That is correct.

Gnome-terminal on my debian testing installation has TERM set to xterm-256color.

@Aetf
Copy link
Owner

Aetf commented Jul 19, 2022

I made a mistake in #23 (comment), on debian the terminfo should also be ^? to match \177...

It turns out Archlinux also sets kbs=^? for xterm: https://github.com/archlinux/svntogit-packages/blob/packages/ncurses/trunk/PKGBUILD#L56

I give up. Let's avoid premature generalization and just merge this.

CMakeLists.txt Outdated Show resolved Hide resolved
@Aetf Aetf added this to the 4.1.0 milestone Jul 19, 2022
@Aetf
Copy link
Owner

Aetf commented Jul 19, 2022

Cloud you also add a unit test?

@viccie30
Copy link
Contributor Author

Cloud you also add a unit test?

I've added a unit test, checking both the default state and the two explicit states.

@Aetf
Copy link
Owner

Aetf commented Jul 20, 2022

There's no ck_assert_mem_eq in libcheck on CI. Ubuntu 20.04 has libcheck 0.10

These functions make it possible to test not-null terminated strings.
They were added in Check 0.11.0, but Ubuntu 20.04 only has version 0.10.
Keep the default code as ASCII backspace, but allow setting the code to
ASCII delete. This is the code used by the Linux console and most modern
terminal emulators by default.
@viccie30
Copy link
Contributor Author

I added a fallback implementation in

libtsm/test/test_common.h

Lines 106 to 120 in b26a06a

#ifndef ck_assert_mem_eq
#include <string.h>
#define ck_assert_mem_eq(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) == 0)
#define ck_assert_mem_ne(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) != 0)
#define ck_assert_mem_lt(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) < 0)
#define ck_assert_mem_le(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) <= 0)
#define ck_assert_mem_gt(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) > 0)
#define ck_assert_mem_ge(_x, _y, _len) \
ck_assert(memcmp((_x), (_y), (_len)) >= 0)
#endif
.

I think having this available may come in handy for other correctness tests in the future.

Copy link
Owner

@Aetf Aetf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Aetf Aetf merged commit 9637480 into Aetf:develop Jul 21, 2022
@viccie30 viccie30 deleted the backspace branch July 23, 2022 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keycode for backspace
2 participants