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.

Sign up for GitHub

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

Jump to bottom

Fix bug with layer caching in get_event_keycode #8693

Merged
merged 2 commits into from
Apr 12, 2020
Merged

Fix bug with layer caching in get_event_keycode #8693

merged 2 commits into from
Apr 12, 2020

Conversation

drashna
Copy link
Member

@drashna drashna commented Apr 5, 2020

Description

Basically, the get_event_keycode handler was updating the layer cache when it shouldn't have been. This was causing Layer Tap to get stuck, since it was no longer valid.

This adds an additional check to ensure if the layer cache should be updated or not. And updates all of the functions that are calling it (to be "false", so they don't update the layer cache, and instead read the layer cache.

Types of Changes

  • Core
  • Bugfix

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • 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).

@drashna drashna requested a review from a team April 5, 2020 19:15
tzarc
tzarc requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

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

Are there any docs that need updating, as well?

tmk_core/common/action.c Outdated Show resolved Hide resolved
tmk_core/common/action.c Outdated Show resolved Hide resolved
newtonapple
newtonapple approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@newtonapple newtonapple left a comment

Choose a reason for hiding this comment

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

I've tested this using my test case as well as using the Planck rev6, XD75 & Candy Bar. This patch fixed the LT problem mentioned in #8600 👍

@drashna drashna force-pushed the bug/process_after_fix branch from 175fb39 to 9102640 Compare April 6, 2020 01:56
@drashna
Copy link
Member Author

drashna commented Apr 6, 2020

Are there any docs that need updating, as well?

No, it shouldn't.

There are a number of per-key features that use this to get the keycode. That's why the "bool" was added and the check for it. It's only "true" and updates the layer cache if it's actually being called from process_action, and never anywhere else.

As for needing documentation.... possibly, but not in an existing docs. However, I should probably document why this is the case in the code.

@tzarc
Copy link
Member

tzarc commented Apr 6, 2020

Could we move the main implementation (i.e. with true) to be a "hidden" api only callable from process_action? Just worried people will use this with true without understanding the implications, and in the future if someone creates a new feature and doesn't understand the semantics of the API it'll end up requiring a whole bunch of investigation.

@drashna drashna force-pushed the bug/process_after_fix branch from 9102640 to 9565a96 Compare April 6, 2020 02:09
@drashna drashna force-pushed the bug/process_after_fix branch from 9565a96 to b1b5176 Compare April 6, 2020 02:10
@drashna
Copy link
Member Author

drashna commented Apr 6, 2020

in that case, I'll add a note in the code, and change the bool to update_layer_cache to make it more clear what is going on (naming, ugh)

@drashna drashna requested review from tzarc and a team April 6, 2020 02:22
@tzarc tzarc requested a review from a team April 6, 2020 02:37
newtonapple
newtonapple approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@newtonapple newtonapple left a comment

Choose a reason for hiding this comment

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

I retested the new changes again and everything is still working.

@zvecr zvecr mentioned this pull request Apr 8, 2020
Closed
13 tasks
@EricGebhart
Copy link
Contributor

Tested on xd75:ericgebhart, It is the same code I have for ergodox_ez:ericgebhart. Everything works as expected. Thank you.

@Erovia Erovia merged commit 23124b9 into qmk:master Apr 12, 2020
@drashna drashna deleted the bug/process_after_fix branch April 12, 2020 15:27
calmh added a commit to calmh/qmk_firmware that referenced this pull request Apr 13, 2020
@calmh
Merge branch 'master' into calmh
a855052 
* master: (973 commits)
  Fix AVR SPI parameter configuration, remove timeouts due to sync protocol. (qmk#8775)
  VIA Support: Jane V2 (qmk#8735)
  Add a simple custom keymap for Gergo. (qmk#8662)
  Add via support to keebio/bdn9 (qmk#8620)
  DP60 VIA cleanups (qmk#8697)
  Adding Niu Mini to VIA (qmk#8702)
  Allow trailing whitespace in markdown docs, for formatting purposes. (qmk#8774)
  Add support for hardware and board initialisation overrides. (qmk#8330)
  [Keyboard] Add IDOBAO ID80 (qmk#8728)
  [Keyboard] Quefrency Rev2 Caps Lock LED, set lighting defaults (qmk#8729)
  [Keyboard] Add handwired Fc200rt qmk board (qmk#8726)
  Bugfix for quantum/dip_switch.c (qmk#8731)
  Add *OPT aliases for *ALT keycodes and macros (qmk#8714)
  [Keymap] Add keymap for Nyquist rev3 (qmk#8706)
  format code according to conventions [skip ci]
  Added Workman ZXCVM variation (qmk#8686)
  [Keyboard] jotpad16 status leds (qmk#8643)
  [Keyboard] Add handwired BDN9-BLE (qmk#8192)
  Fix bug with layer caching in get_event_keycode (qmk#8693)
  [Keyboard] Add CannonKeys Atlas keyboard (qmk#8207)
  ...
Quarren42 pushed a commit to Quarren42/qmk_firmware that referenced this pull request Apr 15, 2020
@drashna @Quarren42
Fix bug with layer caching in get_event_keycode (qmk#8693)
d375fc8 
* Fix bug with layer caching in get_event_keycode

* Improve naming
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@drashna @kujawakk
Fix bug with layer caching in get_event_keycode (qmk#8693)
2076d9a 
* Fix bug with layer caching in get_event_keycode

* Improve naming
@Syzygies Syzygies mentioned this pull request Apr 30, 2020
Closed
violet-fish pushed a commit to violet-fish/qmk_firmware that referenced this pull request May 3, 2020
@drashna @violet-fish
Fix bug with layer caching in get_event_keycode (qmk#8693)
2f2b181 
* Fix bug with layer caching in get_event_keycode

* Improve naming
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
@drashna @lorenzoernesto
Fix bug with layer caching in get_event_keycode (qmk#8693)
4950864 
* Fix bug with layer caching in get_event_keycode

* Improve naming
drashna added a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
@drashna
Fix bug with layer caching in get_event_keycode (qmk#8693)
b99811a 
* Fix bug with layer caching in get_event_keycode

* Improve naming
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
@drashna @sowbug
Fix bug with layer caching in get_event_keycode (qmk#8693)
22075c9 
* Fix bug with layer caching in get_event_keycode

* Improve naming
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
@drashna @fdidron
Fix bug with layer caching in get_event_keycode (qmk#8693)
582fbbc 
* Fix bug with layer caching in get_event_keycode

* Improve naming
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
@drashna @turky
Fix bug with layer caching in get_event_keycode (qmk#8693)
f97f279 
* Fix bug with layer caching in get_event_keycode

* Improve naming
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
@drashna @jakobaa
Fix bug with layer caching in get_event_keycode (qmk#8693)
ccc5ffc 
* Fix bug with layer caching in get_event_keycode

* Improve naming
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@drashna
Fix bug with layer caching in get_event_keycode (qmk#8693)
9fd2d6d 
* Fix bug with layer caching in get_event_keycode

* Improve naming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkoinad arkoinad arkoinad approved these changes

@tzarc tzarc tzarc approved these changes

@newtonapple newtonapple newtonapple approved these changes

Assignees
No one assigned
Labels
bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Recent commit breaks LT(5,KC_J) on FC660C
6 participants
@drashna @tzarc @EricGebhart @newtonapple @arkoinad @Erovia