Skip to content

dump: Fix rountrip#137

Merged
legionus merged 6 commits intomasterfrom
github-pull-136
Jul 1, 2025
Merged

dump: Fix rountrip#137
legionus merged 6 commits intomasterfrom
github-pull-136

Conversation

@legionus
Copy link
Copy Markdown
Owner

This is an adaptation of the #136 with additional changes.

wismill and others added 3 commits June 29, 2025 13:57
There are two issues related when dumping a keymap in text format:
- These is a conflict between skipping the keymaps in `lk_dump_keymaps`
  and forcing all keymaps in `lk_dump_keys`.

  Fixed by always including all defined keymaps, independently of the
  `LK_KEYWORD_ALTISMETA` flag.
- The function `lk_add_constants` is not called, so implicit mappings
  such as `Ctrl + q = Control_q` are not set.

  Fixed by properly calling `lk_add_constants` before each call of
  `lk_dump_keymap`. `lk_add_constants` is not added to `lk_dump_keymap`,
  since it modifies the keymap.

  Note that since it is easy to forget to call `lk_add_constants`, a
  better fix could be to call `lk_add_constants` just before returning
  from `lk_parse_keymap`.

Signed-off-by: Pierre Le Marre <dev@wismill.eu>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
It is necessary to output keymaps only if there is a range. Otherwise, a
syntactically incorrect keymap will be output.

Before:

$ src/loadkeys --tkeymap - < /dev/null
keymaps
$

After:

$ src/loadkeys --tkeymap - < /dev/null
$

Signed-off-by: Alexey Gladkov <legion@kernel.org>
The lk_dump_bkeymap() and lk_dump_ctable() add implicit mappings by
calling lk_add_constants(). The lk_dump_keymap() does not do this.

To improve consistency let's add an additional function that does
the same way.

Signed-off-by: Alexey Gladkov <legion@kernel.org>
@legionus
Copy link
Copy Markdown
Owner Author

@dmage could you take a look and tell me what you think?

@@ -496,7 +494,8 @@ void lk_dump_keys(struct lk_ctx *ctx, FILE *fd, lk_table_shape table, char numer
if (table == LK_SHAPE_SEPARATE_LINES) {
for (j = 0; j < keymapnr; j++) {
//if (buf[j] != K_HOLE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The time to remove this comment has come

kw = i = n = m = s = 0;

for (i = 0; i < ctx->keymap->total; i++) {
if (ctx->keywords & LK_KEYWORD_ALTISMETA && i == (i | M_ALT))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it no longer needed?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This code skipped (did not output) those keymaps that were ALT variants (i | M_ALT) if the ALTISMETA flag was set. That is, if alt_is_meta was active, ALT tables were not explicitly output, implying that they were automatically restored if alt_is_meta is specified. But dump_keymaps does not restore alt_is_meta itself.

Now the keymap dump always fully reflects all real tables, without the "magic" of silencing ALT tables. This makes the dump more transparent and unambiguous.

Copy link
Copy Markdown
Contributor

@dmage dmage left a comment

Choose a reason for hiding this comment

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

lgtm, though I don't understand the part related to alt_is_meta

Signed-off-by: Alexey Gladkov <legion@kernel.org>
@legionus
Copy link
Copy Markdown
Owner Author

I've noticed that if the original keymap specifies alt_is_meta, it is far from always possible to recover it in lk_dump_keys(). But this keyword in original keymap will affect the keymaps keyword.

@legionus legionus mentioned this pull request Jun 30, 2025
1 task
legionus added 2 commits June 30, 2025 18:30
Signed-off-by: Alexey Gladkov <legion@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
@legionus legionus merged commit 54711d6 into master Jul 1, 2025
8 checks passed
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