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

Add Compose support #36

Merged
merged 3 commits into from
Feb 14, 2022
Merged

Add Compose support #36

merged 3 commits into from
Feb 14, 2022

Conversation

viccie30
Copy link

@viccie30 viccie30 commented Feb 6, 2022

@snakeroot linked to their implementation in #23. I had independently written a (worse) patch as well. I've taken inspiration from both @snakeroot's and my own implementation for this PR. The main functional difference between their version and this one is that this PR does not require a command-line option to enable Compose support.

Only the first and fourth commits are actually needed. The second commit adds a new launcher to also query the locale information from systemd-localed. The third commit allows the user to specify a Compose file explicitly, but I don't know if that's really useful?

@Aetf
Copy link
Owner

Aetf commented Feb 6, 2022

Thanks for putting up the PR! I do have some early questions.

Why it's necessary to set locale explicitly? Per systemd manual:

The locale settings configured in /etc/locale.conf are system-wide and are inherited by every service or user, unless overridden or unset by individual programs or individual users.

Also, the dbus service org.freedesktop.locale1 itself is already systemd specific. It makes little sense to use the helper script without systemd.

Overall, I feel if we were introducing a dependency on sd-bus in C, I think it's better to completely get rid of the helper script/program and have the ability directly built into the main executable behind a compile time switch.

@viccie30
Copy link
Author

viccie30 commented Feb 6, 2022

I was actually hoping for feedback, thanks for that.

I read systemd-localed(8), which was a bit sparse, but you're right, systemd takes care of it itself. It might make sense to roll the querying of org.freedesktop.locale1 into kmscon itself. While it looks a bit hackish calling dbus-send from a script, it does actually work, so it's a matter of taste.

@Aetf
Copy link
Owner

Aetf commented Feb 7, 2022

Let's drop ef1310e for now then. We can reconsider this in the future when kmscon has more use of dbus internally.

@Aetf
Copy link
Owner

Aetf commented Feb 7, 2022

Also, let me know when you are ready for me to review other parts.

@viccie30
Copy link
Author

viccie30 commented Feb 7, 2022

I've dropped the launcher commit. The rest is ready for review.

One thing I'd like your view on is what to do with cancelled sequences (https://xkbcommon.org/doc/current/group__compose.html#compose-cancellation). Right now the cancelling keysym is swallowed, but perhaps it should be let through? I've followed @snakeroot's example for now.

@viccie30 viccie30 marked this pull request as ready for review February 7, 2022 12:09
@snakeroot
Copy link

Hi @viccie30 thanks for pushing this forward. Credit where credit is due, the patch linked to in #23 was written by @bluetech not me.

@viccie30
Copy link
Author

viccie30 commented Feb 7, 2022

You're absolutely right, it's even in their repository.

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.

Overall looks good! I have just a few nitpicks marked inline.

Also, please make sure to credit @bluetech in the commit message. Or even better, incorporate some lines in the original commit message bluetech@89aeb0f. It provides a good overview of the feature. Maybe that should also go into the man page?

src/uterm_input_uxkb.c Outdated Show resolved Hide resolved
docs/man/kmscon.xml Outdated Show resolved Hide resolved
src/uterm_input_uxkb.c Show resolved Hide resolved
src/uterm_input_uxkb.c Show resolved Hide resolved
src/uterm_input_uxkb.c Outdated Show resolved Hide resolved
src/uterm_input_uxkb.c Show resolved Hide resolved
@viccie30
Copy link
Author

I have force-pushed some changes you asked for. I have credited @bluetech in the first commit and used part of their commit message as well. Should I add credits in the other two commits as well?

src/uterm_input_uxkb.c Outdated Show resolved Hide resolved
src/uterm_input_uxkb.c Outdated Show resolved Hide resolved
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!! One last thing before we can merge this.

Should I add credits in the other two commits as well?

Your current way is all good.

The code is partially based on and partially inspired by Ran Benita's
code at bluetech@89aeb0f.

The main difference is that Compose support is enabled by default,
instead of requiring an extra command-line option.

Dead keys are special keys which have no immediate effect when pressed,
but instead they modify the result of subsequent keys. For example,
pressing the <`> key (the dead key) and then <u> may produce the symbol
ú. Such sequences can be more elaborate and consist of several keysyms.

The compose mechanism has two main pieces of configuration:

1. The Compose file, which defines the compose sequences. It is picked
   by the user's locale. See the Compose(5) man page; most of it is
   supported. These files are currently a part of libX11, but some users
   write their own. For example:
   /usr/share/X11/locale/en_US.UTF-8/Compose

2. The user's XKB keymap, which defines which keys are dead keys (that
   is, produce keysyms of the form dead_*, which are normally used in
   Compose sequences) and which key is the Compose key (which starts
   many Compose sequences). For example, the us 'intl' variant.

This feature (and more) is usually provided by an input method. But we
use the lightweight support provided by xkbcommon.
Analogously to --xkb-keymap this allows a user to directly specify an
Xkb compose file. This overrides the automatic creation of a compose
table from the current locale.
Feed all key events into the Compose state and use the resulting
keysyms.
@viccie30
Copy link
Author

It should be alright now. I've tested it myself over the last few days and it seems to work so far.

@viccie30 viccie30 requested a review from Aetf February 14, 2022 19:27
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.

🎉

@Aetf Aetf merged commit ed9adf1 into Aetf:develop Feb 14, 2022
@Aetf Aetf mentioned this pull request Feb 14, 2022
@viccie30 viccie30 deleted the compose branch February 22, 2022 11:27
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.

3 participants