fixes null-character termination in ucis_add()

[rubienr]

Description

With UCIS_ENABLE = yes and UNICODE_COMMON = yes when entering a wrong mnemonic, a second attempt with correct mnemonic would not trigger the ucis input.

This change:

• adds a trailing 0 after the last added character in ucis_add()
• introduces boundary check on ucis_add() to not write out of bounds
• clears buffered character by 0 on ucis_remove_last()
• correctly handles abortion (ESC)
• correctly handles invalid characters (i.e.: ,.-/* ...)

Types of Changes

• [ ] Core
• [x] Bugfix
• [ ] New feature
• [x] Enhancement/optimization
• [ ] Keyboard (addition or update)
• [ ] Keymap/layout/userspace (addition or update)
• [ ] Documentation

Issues Fixed or Closed by This PR

Fixes following example:

keymap.c

const ucis_symbol_t ucis_symbol_table[] = UCIS_TABLE(
    UCIS_SYM("poop", 0x1F4A9),                // 💩
    UCIS_SYM("rofl", 0x1F923),                // 🤣
    UCIS_SYM("ukr", 0x1F1FA, 0x1F1E6),        // 🇺🇦
    UCIS_SYM("look", 0x0CA0, 0x005F, 0x0CA0)  // ಠ_ಠ
);

1. enter ⌨poopxyz (wrong mnemonic)
2. enter ⌨poop (correct mnemonic) would not trigger since the buffer is still poopxyz'0''0'...

Checklist

• [ ] My code follows the code style of this project: C, Python
• [ ] I have read the PR Checklist document and have made the appropriate changes.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.
• [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Relates to https://github.com/qmk/qmk_firmware/pull/21659

[rubienr]

IMHO the fix reveals that match_mnemonic() had an issue because test UnicodeUCIS.does_not_match_shorter_sequence fails. However the bug/feature seems very convenient. If the mnemonic is not fully typed match_mnemonic() will match the first substring which i found to be very handy.

Given:

const ucis_symbol_t ucis_symbol_table[] = UCIS_TABLE(
    UCIS_SYM("poop", 0x1F4A9),                // 💩
    UCIS_SYM("rofl", 0x1F923),                // 🤣
    UCIS_SYM("ukr", 0x1F1FA, 0x1F1E6),        // 🇺🇦
    UCIS_SYM("look", 0x0CA0, 0x005F, 0x0CA0)  // ಠ_ಠ
);

• ⌨p would match poop
• ⌨ro would match rofl
• ...

Should i fix the test (and leave the current behaviour) or should i fix the behaviour:question:

[fauxpark]

I think that behaviour might be less than predictable to most users, even if you personally find it useful. It's probably more straightforward to trigger only on the first exact match (which should really be the only exact match).

[github-actions[bot]]

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[KarlK90]

@rubienr thanks for your PR. It looks good in principle but there are now falling unit-tests as you have changed the behavior. This has to be fixed in order to merge. Also please add the mentioned example in the description of the pr as a new test.

[rubienr]

Apologies for the long delay. I will try to focus on this PR ASAP: comments, suggestions, failing tests. :/

[rojizo]

I think that behaviour might be less than predictable to most users, even if you personally find it useful. It's probably more straightforward to trigger only on the first exact match (which should really be the only exact match).

In fact, empty matching match the first thing in array which is weird...

Moreover, this fix closes #21128 and #22438.

[rojizo]

The match_mnemonic could be implemented as

static bool match_mnemonic(char *mnemonic) {
    uint8_t i = 0;
    for (; input[i]; i++) {
        if (i > count || input[i] != mnemonic[i]) {
            return false;
        }
    }
    return (mnemonic[i] == '\0'); // Here, input[i] is '\0'
}

A conditional compilation variable could be added to switch between behaviours

[tzarc]

Unblocked stalebot from doing its thing -- if there's no appetite for fixing unit tests, then there's no need to keep this PR open.

[github-actions[bot]]

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions[bot]]

Thank you for your contribution! This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. // [stale-action-closed]