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 in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD #8674

Merged
merged 6 commits into from
May 9, 2020
Merged

Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD #8674

merged 6 commits into from
May 9, 2020

Conversation

vomindoraan
Copy link
Contributor

Description

  1. There is a bug in cycle_unicode_input_mode() that prevents cycling in reverse. The bug is due to unsigned arithmetic. As a consequence, the UC_RMOD keycode does not currently work. This PR changes the uint8_t offset parameter to int8_t and fixes the bug.

  2. This PR further allows Shift to be used for inverting the direction of the UC_MOD, UC_RMOD keycodes, similar to how RGB_MOD, RGB_RMOD work.
    I've tested UC_RMOD and cycling with Shift after these changes and can confirm that they work as intended.

  3. Currently, audio feedback for changing input modes currently only plays for specific UC_M_* keycodes. This PR allows it to play when changing input modes with UC_MOD, UC_RMOD as well.

Types of Changes

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

Checklist

  • My code follows the code style of this project.
  • 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).

@vomindoraan
Copy link
Contributor Author

I don't own a board with audio to test the audio changes, but they should be okay. @drashna, I recall you using this feature; if it's not too much of a bother, could you try it out and let me know if it still works with these changes?

vomindoraan
vomindoraan commented Apr 10, 2020
View reviewed changes
quantum/process_keycode/process_unicode_common.c
Comment on lines -63 to +70
void cycle_unicode_input_mode(uint8_t offset) {
void cycle_unicode_input_mode(int8_t offset) {
#if UNICODE_SELECTED_MODES != -1
selected_index = (selected_index + offset) % selected_count;
selected_index = (selected_index + offset) % selected_count;
if (selected_index < 0) {
selected_index += selected_count;
}
Copy link
Contributor Author

@vomindoraan vomindoraan Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Explanation:
First off, since you're supposed to be able to cycle backward (using a negative offset) as well as forward, the parameter shouldn't be unsigned. The index calculation tries to take (selected_index + offset) mod selected_count, but only succeeds in doing so partially because the C % operator is a remainder, not a modulo operation. In other words, its result can be negative, and if it is, it needs to be shifted back into the [0, selected_count) range.

The current behavior of this section (prior to the changes) is equivalent to the following pseudocode:

if offset < 0 then
    selected_index ← (selected_index + offset + 1) mod selected_count 
else
    selected_index ← (selected_index + offset) mod selected_count
fi

This is a quirk of the operands being unsigned. Needless to say, it's not the desired behavior, and any arithmetic that expects logically negative operands should be changed to signed arithmetic.

@mtei mtei requested review from skullydazed, fauxpark, yanfali, drashna and a team April 10, 2020 16:43
@drashna drashna requested a review from a team April 12, 2020 17:34
ridingqwerty
ridingqwerty approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@ridingqwerty ridingqwerty left a comment

Choose a reason for hiding this comment

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

👍

noroadsleft
noroadsleft approved these changes Apr 25, 2020
View reviewed changes
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.

Looks good to me, with the caveat that I don't use either Unicode or Audio.

zvecr
zvecr requested changes Apr 25, 2020
View reviewed changes
Copy link
Member

@zvecr zvecr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

clang-format puts back a few of the changes to quantum/process_keycode/process_unicode_common.c, can it be run on the changes please. (And if some of the style change is required, worked around.)

@vomindoraan
Copy link
Contributor Author

Done.

@vomindoraan vomindoraan requested a review from zvecr May 7, 2020 00:27
@drashna drashna merged commit 94fc32f into qmk:master May 9, 2020
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
@vomindoraan @lorenzoernesto
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
96c77fd 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
@vomindoraan @drashna
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
6a2449f 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
@vomindoraan @sowbug
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
4b2aec5 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
@vomindoraan @fdidron
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
f91afd0 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
@vomindoraan @turky
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
60d7528 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
@vomindoraan @jakobaa
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
8ba8fd4 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
sjmacneil pushed a commit to sjmacneil/qmk_firmware that referenced this pull request Feb 19, 2021
@vomindoraan @sjmacneil
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
cdaff9a 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@vomindoraan
Fix bug in UC_RMOD, add shift and audio support for UC_MOD/UC_RMOD(qm…
97a3bad 
…k#8674)

* Invert UC_MOD/UC_RMOD direction when Shift is held

Also use MOD_MASK_SHIFT in process_rgb.c

* Allow audio to be played for UC_MOD, UC_RMOD keycodes as well

* Fix signedness bug in reverse input mode cycling

* Misc formatting in process_unicode_common.c

* Address clang-format issues

* Make decode_utf8 helper function file-local (static)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noroadsleft noroadsleft noroadsleft approved these changes

@ridingqwerty ridingqwerty ridingqwerty approved these changes

@drashna drashna drashna approved these changes

@skullydazed skullydazed Awaiting requested review from skullydazed

@fauxpark fauxpark Awaiting requested review from fauxpark

@yanfali yanfali Awaiting requested review from yanfali

@zvecr zvecr Awaiting requested review from zvecr

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

Successfully merging this pull request may close these issues.
6 participants
@vomindoraan @drashna @ridingqwerty @zvecr @noroadsleft @mtei