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

Added special hotkey config rules for comma and semicolon #916

Merged
merged 1 commit into from
Jan 10, 2021
Merged

Added special hotkey config rules for comma and semicolon #916

merged 1 commit into from
Jan 10, 2021

Conversation

dustinlieu
Copy link

@dustinlieu dustinlieu commented Oct 17, 2020

The config parser interprets commas and semicolons as syntax. This PR adds commas and semicolons to the special keys list so that ',' becomes "Comma" and ';' becomes "Semicolon". This allows hotkeys to contain commas and semicolons.

Fixes issue #778

Edited for additional comments on changes on line 215:
I had to reorder how KeySequence::keyToString() decides how to serialize a key so that it will prioritize checking the special keys list before checking for 7-bit printable characters. This made excluding a space character (key != Qt::Key_Space) from the 7-bit printable character check (if (key < 0x80 && key != Qt::Key_Space)) redundant because Qt::Key_Space is already in the special keys list and will already be substituted for "Space".

// a printable 7 bit character?
if (key < 0x80 && key != Qt::Key_Space)
if (key < 0x80)
Copy link

Choose a reason for hiding this comment

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

hi, pr description has no explanation on this line change, please could you add it?

Copy link
Author

Choose a reason for hiding this comment

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

I edited the pr description to include the explanation

@p12tic
Copy link
Member

p12tic commented Jan 10, 2021

We now have tests, so it's possible to easily see what this PR fixes and add regression tests if the PR breaks something. We can finally merge this improvement. Thanks a lot!

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.

3 participants