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 Onboard Feature #8359

Closed
wants to merge 4 commits into from
Closed

Add Compose Onboard Feature #8359

wants to merge 4 commits into from

Conversation

oberien
Copy link

@oberien oberien commented Mar 9, 2020

This PR adds the Compose Onboard feature, which allows composing directly on the keyboard instead of requiring userland composing (like .XCompose on Linux).

In short, after pressing the KC_COMPOSE_ONBOARD key, a sequence of keys can be pressed, which can be mapped to actions in code. This is somewhat similar to the Leader Key feature with the main distinction that the leader key is time-based, while this is only input-based.

I used the Leader Key feature implementation as a guideline and I think I have added code / documentation everywhere where the Leader Key appears (according to grep ;) ).

I use it in my own keymap (the compose part can be seen here).

Checklist

  • My code follows the code style of this project.
    • hopefully, if not, please tell me so I can change it
  • 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 don't know how this can be tested via unit tests. Also I don't think I saw any test file for the Leader Key feature.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Edit: One possible problem is that the mapping dictionary requires quite a bit of space, increasing the binary size. For example just the shrug mapping in the documentation example requires close to 200 bytes.

@drashna drashna requested a review from a team March 9, 2020 17:45
@drashna
Copy link
Member

drashna commented Mar 9, 2020

Just a heads up, the UCIS method does something like this, but explicitly for unicode type stuff. It's not as customizable, where this is somewhere between UCIS and leader keys.

@stale
Copy link

stale bot commented Apr 24, 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.

@oberien
Copy link
Author

oberien commented Apr 25, 2020

I rebased it onto master to resolve merge conflicts in common_features.mk (order changed) and docs/ja/_summary.md (order changed in #8508). For the Japanese summary, I don't know if I put this feature in the correct category as I don't understand Japanese. However, I think that it is similar enough to the leader key, that it should also fit into "Advanced key code" as Google Translator translated it.

Is there anything else missing that blocks a review and possible merge?

Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Haven't fully reviewed this code, but here's what I have so far:


|Key |Aliases |Description |
|--------------------|-----------|-----------------------------------------|
|`KC_COMPOSE_ONBOARD`|`KC_COMPOB`|Begins Composing directly on the keyboard|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|`KC_COMPOSE_ONBOARD`|`KC_COMPOB`|Begins Composing directly on the keyboard|
|`KC_COMPOSE_ONBOARD`|`KC_CMOB`|Begins Composing directly on the keyboard|

We like these keycode aliases to be seven characters or less, when possible.

Also, this entire section should be moved up in the document to be between the Bluetooth and Dynamic Macros sections, as this page is organized alphabetically (with the exception of the Basic and Quantum sections).

Copy link
Member

Choose a reason for hiding this comment

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

IMO Quantum keycodes should not have the KC_ prefix either, as that is reserved for the basic keycode set.

Comment on lines +81 to +85
## Compose Onboard
Allows composing directly on the keyboard. After pressing the compose onboard key, the following keystrokes trigger a preconfigured action like pressing other keys, inserting a character, or any other quantum features.

* [Compose Onboard Documentation](feature_compose_onboard.md)

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that this needs to be added to the glossary, but if its decided that it should be so, this section should move up between Compile and Dvorak, as this page is organized alphabetically.

Comment on lines +126 to +130
#ifdef COMPOSE_ONBOARD_ENABLE
KC_COMPOSE_ONBOARD,
KC_COMPOB = KC_COMPOSE_ONBOARD,
#endif

Copy link
Member

Choose a reason for hiding this comment

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

  1. The KC_COMPOSE_ONBOARD keycode should be added near the end of the quantum_keycodes enum, just before SAFE_RANGE on Line 520, so as not to break compatibility with VIA Configurator.
    2 The KC_COMPOB alias should be created using a #define statement around Line 814, as #define KC_CMOB KC_COMPOSE_ONBOARD if you've applied the suggestion I gave above.

@noroadsleft noroadsleft requested a review from a team April 26, 2020 01:46
@drashna
Copy link
Member

drashna commented Apr 28, 2020

Honestly, I am opposed to this change.

It feels like it is very much what UCIS does already.
And even if that wasn't the case, if #6580 gets merged, then I believe that this will be duplicating functionality.

In either case, it feels redundant. I think the better direction would be to get #6580 merged in.

That said, we absolutely appreciate the time and effort that went into this. Even if it doesn't get merged, we're happy to see the contribution!

@tzarc tzarc changed the base branch from master to develop July 25, 2020 23:06
@tzarc
Copy link
Member

tzarc commented Jul 25, 2020

Retargeted to develop due to core changes. Discussion regarding the relevance is still outstanding.

@drashna
Copy link
Member

drashna commented Dec 17, 2020

There is a merge conflict here, now. Needs a rebase.

@tzarc tzarc closed this Feb 27, 2021
@tzarc tzarc deleted the branch qmk:develop February 27, 2021 20:27
@noroadsleft noroadsleft reopened this Feb 27, 2021
@oberien
Copy link
Author

oberien commented Apr 30, 2021

With #6580 now being merged, I don't think there is a reason left for this PR to stay open. Thanks for your effort in reviewing and rebasing it.

@oberien oberien closed this Apr 30, 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