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

Fix OSM on a OSL activated layer #20410

Merged
merged 4 commits into from
Sep 25, 2023
Merged

Fix OSM on a OSL activated layer #20410

merged 4 commits into from
Sep 25, 2023

Conversation

NapOli1084
Copy link
Contributor

@NapOli1084 NapOli1084 commented Apr 11, 2023

Description

This change follows #19214 by @kosorin, which allowed mod-tap hold action on one shot layers. The purpose of this change is to do the same for one shot mods and allow them on one shot layers.

I attempted adding OSM(MOD_LSFT) on one of my OSL activated layers in order to use it along with accented letters I have on that layer. I realized it didn't work and after looking into the code I noticed the condition that prevents deactivating one shot layers with mods, which wasn't considering one shot mods, but was considering mod-tap. So it seemed like a simple oversight, doesn't seem intended that OSM wasn't considered. At least I can't think of a reason why it would be supported with other mods but not with OSM.

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).

@github-actions github-actions bot added the core label Apr 11, 2023
quantum/action.c Outdated
@@ -373,7 +373,8 @@ void process_action(keyrecord_t *record, action_t action) {
if (is_oneshot_layer_active() && event.pressed &&
(action.kind.id == ACT_USAGE || !(IS_MODIFIER_KEYCODE(action.key.code)
# ifndef NO_ACTION_TAPPING
|| (tap_count == 0 && (action.kind.id == ACT_LMODS_TAP || action.kind.id == ACT_RMODS_TAP))
|| ((action.kind.id == ACT_LMODS_TAP || action.kind.id == ACT_RMODS_TAP) &&
(action.layer_tap.code <= MODS_TAP_TOGGLE || tap_count == 0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tap_count == 0 accounted for mod-tap, but not for OSM. action.layer_tap.code <= MODS_TAP_TOGGLE accounts for MODS_ONESHOT and MODS_TAP_TOGGLE. TBH I don't know how the latter can occur, can't figure a keycode leading to it. But considering it's for mods also I think it makes sense to consider them too.

@NapOli1084
Copy link
Contributor Author

I intended to look into adding a unit test for this, but I can't seem to build them. I'm getting "undefined reference" linker errors on many functions, seems to be the ones with weak linking. I'm on QMK MSYS, don't know if it's a setup issue. I'll try asking around on Discord.

@NapOli1084
Copy link
Contributor Author

I intended to look into adding a unit test for this, but I can't seem to build them. I'm getting "undefined reference" linker errors on many functions, seems to be the ones with weak linking. I'm on QMK MSYS, don't know if it's a setup issue. I'll try asking around on Discord.

I didn't figure out my linker issue, but gave it a try anyway, relying on the CI tests. Unfortunately means I failed on first attempt, but second succeeded.

@sigprof
Copy link
Contributor

sigprof commented Apr 11, 2023

Currently the tests could be built either on Linux, or on MacOS if you compile them using GCC instead of Clang (which is apparently the default compiler there). The MSYS2 toolchain for Windows has some issues with weak symbols (LTO_ENABLE=yes might help, but it's not guaranteed to be a reliable fix).

@NapOli1084
Copy link
Contributor Author

Currently the tests could be built either on Linux, or on MacOS if you compile them using GCC instead of Clang (which is apparently the default compiler there). The MSYS2 toolchain for Windows has some issues with weak symbols (LTO_ENABLE=yes might help, but it's not guaranteed to be a reliable fix).

I see, thanks for the info. I guess I should use a container/WSL if I have another need to run them. I used vagrant in the past (have not tried running tests with it though), but since support was removed I went back to MSYS, and TBH was happy about it cause the container was extremely slow.

@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jul 16, 2023
@drashna drashna requested a review from a team July 27, 2023 03:16
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jul 28, 2023
@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Sep 12, 2023
@drashna drashna requested a review from a team September 14, 2023 08:25
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Sep 15, 2023
@tzarc tzarc merged commit e0eb90a into qmk:develop Sep 25, 2023
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Oct 25, 2023
christrotter pushed a commit to christrotter/qmk_firmware that referenced this pull request Nov 28, 2023
bencollerson added a commit to bencollerson/qmk_firmware that referenced this pull request Nov 29, 2023
@bencollerson
Copy link

bencollerson commented Nov 29, 2023

This change breaks the functionality of putting one shot mods on a layer and using them on the default (or other) layer.

I raised an issue related to this: #22566

Edit: Oops not sure why my revert commit was added here. I am sure some less heavy-handed solution is possible. haha ;)

@elshize
Copy link

elshize commented Dec 2, 2023

Same here, I am relying on the old functionality, and had to revert these changes to make it work again.

@NapOli1084 NapOli1084 deleted the osl-osm branch December 12, 2023 02:11
zgagnon pushed a commit to zgagnon/qmk_firmware_waterfowl that referenced this pull request Dec 15, 2023
future-figs pushed a commit to future-figs/qmk_firmware that referenced this pull request Dec 27, 2023
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Jan 12, 2024
alycklama pushed a commit to alycklama/qmk_firmware that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants