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

Implement lightweight trie-based Compose Key for QMK. #10063

Closed
wants to merge 15 commits into from

Conversation

agdphd
Copy link

@agdphd agdphd commented Aug 17, 2020

Description

Implement a knockoff of the Linux compose key which can emit a specified string (including Unicode if support is compiled in) when the user hits a new modifier COMPOSE followed by a configurable sequence of keys. Distinct from the Leader Key in two ways:

  • No timeouts--purely based on sequence of keypresses
  • Prints the specified string as soon as a complete sequence is keyed

Changes are mostly isolated to a new key processor which:

  • Holds a global static search trie for compose codes (learning enough C to write a prefix trie to run on a keyboard was definitely a fun weekend project :)
  • When COMPOSE is pressed, starts a state machine to search the trie with subsequent keypresses, which
    • Exits whenever the user misses a prefix, emitting the last keypress to the rest of the processing stack
    • Exits when the user completes a code, printing the corresponding string
    • Exits when the user hits COMPOSE again
  • Configuration is via a very simple DSL with a compiler implemented in the qmk binary under the new command qmk compose, which generates a C file that statically initializes the user's compose sequence trie.

I have added test coverage in /tests/basic and confirmed that make test:basic passes. (make test:all seems to be broken at master.) I have also tested this on my Gergo with the Linux Unicode string printer.

New files list copyright of Google LLC instead of me per https://opensource.google/docs/patching/.

Types of Changes

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

Issues Fixed or Closed by This PR

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.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@agdphd agdphd force-pushed the compose branch 3 times, most recently from a0851a7 to 9cba98e Compare August 17, 2020 02:04
@drashna
Copy link
Member

drashna commented Aug 24, 2020

New files list copyright of Google LLC instead of me per opensource.google/docs/patching.

I'm not sure that this actually applies here,

Also, this seems to be a similar implementation to leader keys.

Also, nice on the additional tests

@drashna drashna requested a review from a team August 29, 2020 03:26
@agdphd
Copy link
Author

agdphd commented Aug 31, 2020

Here's the relevant section of the Google guidelines: https://opensource.google/docs/patching/#license-headers-and-copyright-notices

In particular, for the new files, "Googlers should add Google’s copyright notice (or a “The Project Authors” style copyright notice) to new files being added to the library if permitted by the project maintainers."

The code is, of course, offered under the GPLv2 and may be used by the project as with any other contribution--it's just copyright of Google instead of me personally.

And yeah, this is similar to leader key, but it's sequence-based rather than time-based and supports arbitrarily long command sequences at relatively low per-keypress byte cost. I haven't yet taken on the whole project, but I plan to migrate most of my existing Linux .XCompose (hundreds of sequences, some as long as five or six keypresses) to this framework and expect it to fit comfortably on my Gergo.

tests/basic/test_tapping.cpp Outdated Show resolved Hide resolved
tests/basic/keymap.c Outdated Show resolved Hide resolved
@skullydazed skullydazed requested a review from a team September 1, 2020 05:04
@tzarc
Copy link
Member

tzarc commented Sep 1, 2020

https://opensource.google/docs/patching/#license-headers-and-copyright-notices

When contributing to third-party projects, Googlers do not need to add Google’s copyright notice when authoring patches to existing files. Googlers should add Google’s copyright notice (or a “The Project Authors” style copyright notice) to new files being added to the library if permitted by the project maintainers.

Any file that has already been published with a copyright notice attached to it must include that copyright notice when the file, or any portion thereof, is added to any project.

@tzarc
Copy link
Member

tzarc commented Oct 3, 2020

Seems you've force-pushed the previous Google LLC copyright notices back into your PR.

Co-authored-by: Zach White <skullydazed@drpepper.org>
@agdphd
Copy link
Author

agdphd commented Oct 4, 2020

Ugh, sorry, looks like I made a mess of things while trying to catch up to master. I think I have things figured out now...

tests/test_common/matrix.c Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team October 18, 2020 19:46
@github-actions github-actions bot added cli qmk cli command python labels Oct 18, 2020
@agdphd
Copy link
Author

agdphd commented Oct 19, 2020

Note that this is not currently working--it crashes my keyboard on boot when the compose sequence trie is large :( I'll update here when I get it figured out.

@agdphd
Copy link
Author

agdphd commented Oct 19, 2020

OK, this now actually works on my Gergo, with a trie of ~100 compose sequences: https://github.com/agdphd/qmk_firmware/blob/argd/keyboards/gergo/keymaps/argd/compose.conf

@theol0403
Copy link

I imagine you have already seen it, but since #8359 hasn't been mentioned in this PR I thought it would be a good idea to link it, so people can compare the implementation (assuming they do the same thing).

@stale
Copy link

stale bot commented Dec 10, 2020

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 awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale
Copy link

stale bot commented Jan 9, 2021

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 stale bot closed this Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants