Skip to content

feat: Make qmk module - #39

Merged
stasmarkin merged 9 commits into
stasmarkin:mainfrom
alextverdyy:make_qmk_module
Aug 20, 2025
Merged

stasmarkin merged 9 commits into
stasmarkin:mainfrom
alextverdyy:make_qmk_module

Conversation

@alextverdyy

Copy link
Copy Markdown
Contributor

Description

This PR updates the README to provide clear, concise instructions for using sm_td as a QMK community module, following the official QMK community modules guide.

  • Added step-by-step instructions for enabling the module via keymap.json
  • Provided minimal integration examples for keymap usage
  • Clarified that users do not need to clone the source directly

This makes it easier for users to add sm_td to their QMK builds using the recommended module

@stasmarkin

Copy link
Copy Markdown
Owner

Thank you for your PR. I have a couple of questions before accepting:

  1. Have you tested it? Does it work as intended? Could you please describe the testing steps you followed?
  2. Have you considered migration for users who already have sm_td installed in the old-fashioned way?

@alextverdyy

Copy link
Copy Markdown
Contributor Author

Thank you for your PR. I have a couple of questions before accepting:

  1. Have you tested it? Does it work as intended? Could you please describe the testing steps you followed?
  2. Have you considered migration for users who already have sm_td installed in the old-fashioned way?

Hello!

Yes, actually I done it to use in my own keymap, all seems working fine and have 0 issues with it, the only difference it's the way to import it into the user keymap.

For user that already have sm_td installed don't need to do nothing, same import, same code, the only thing that have to consider it's the definition of rhe module. WDYT?

@stasmarkin

Copy link
Copy Markdown
Owner

Sorry for the late reply.

I'm almost ready to merge this PR, but I'd like to clarify one more thing.
Do you think it's possible to ensure versioning in this community modules scheme?
I'm planning to release version 0.5.0 soon, which will not be backward compatible with 0.4.1. If I simply push 0.5.0 to the repository with this module system, as I understand it, people's builds will simply break and they won't even be able to roll back to 0.4.1 (except by manually changing the files).
Am I correct in my understanding?

@stasmarkin

Copy link
Copy Markdown
Owner

Hi again!

I’ve looked into the documentation about community modules, and it turns out updates are simpler than I expected. I initially thought modules would be auto-downloaded with new version releases, but it’s actually just a basic git submodule setup, so user will have to update everything manually. That makes the update concern irrelevant now.

That said, there are four things I’d like to propose with this PR:

  1. We need to resolve the merge conflicts, since I’ve released a new version of sm_td.
  2. There’s a qmk_module.json file where we can add "features": { "deferred_exec": true }. This would enable deferred_exec by default, so users won’t have to enable it manually.
  3. I’d also like to add "license": "MIT" too
  4. Finally, there’s no need for users to manually insert if (!process_smtd(keycode, record)) { ... } into their process_record_user() function, since the module system provides a pre_process_record API. I believe we can simplify the installation even further by using that.

What do you think?

No pressure to implement all of these in this PR, resolving the first point (merge conflicts) would already be enough to get this merged. The rest can be handled in separate PRs, either by you, me, or someone else later on.

@alextverdyy

Copy link
Copy Markdown
Contributor Author

Hello!

Thanks for the detailed feedback and no worries about the delay!

You're absolutely right — since community modules are handled via regular git submodules, users won't get automatic updates when a new version is released. That means breaking changes in 0.5.0 shouldn’t affect users sticking with 0.4.1, as long as they don’t manually update the submodules. So yes, your understanding is spot on, and versioning shouldn’t be a blocker for merging this.

As for your suggestions — I really appreciate them, and I agree with all four points. Here's how I plan to proceed:

  1. Merge conflicts: I’ll resolve these right away so we can move forward with the PR.
  2. qmk_module.json updates: I’ll add the features.deferred_exec and license fields as you suggested.
  3. Simplified install via pre_process_record: Love this idea! I’ll refactor the setup to use pre_process_record to streamline integration for users.

If anything feels out of scope, I can split it up — but I’ll aim to include as much as possible in this PR to reduce follow-up work.

Thanks again for the thoughtful review!

@stasmarkin

Copy link
Copy Markdown
Owner

That would be great, thank you very much for your contribution!
No need of splitting up, I'd also prefer to put everything related to a task into one PR

- Add auto init to QMK Module
- Fix path used in tests
@alextverdyy

Copy link
Copy Markdown
Contributor Author

@stasmarkin

Thanks for the suggestions — I've implemented them in this update!

✅ Resolved merge conflicts
✅ Added license and features.deferred_exec to qmk_module.json
✅ Simplified integration using pre_process_record_sm_td()
✅ Fixed include path in tests

Let me know if there's anything else you'd like to adjust. Otherwise, this should be good to go!

Comment thread sm_td/README.md Outdated
In your keymap, add an `on_smtd_action` handler:

```c
void on_smtd_action(uint16_t keycode, smtd_action action, uint8_t tap_count) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Don't you mind if I change README in this PR by myself? It's correct for 0.4.0, but outdated for 0.5.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure!

@stasmarkin

Copy link
Copy Markdown
Owner

Fixed some documetation. I will test the whole new installation process tonight (or tomorrow) and then I will merge PR

@stasmarkin
stasmarkin merged commit ba8801a into stasmarkin:main Aug 20, 2025
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.

2 participants