Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235
Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235
Changes from all commits
faccc1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit weird to reference this PR. Let's just keep the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually like referencing the issue in the code near a bug fix like this. It can oftentimes provide a lot more context than just the comment in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need that extra
,
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a list initialization expression, which is kinda akin to an array and I believe this project uses trailing commas for this. Right?
Do you want me to revert this change? It's technically not needed at all for this PR. 🙂
(It only snuck in, because I structured the code entirely differently in a previous attempt for this PR and I preferred block indents over visual indents. 🙈)
P.S.: In fact modern languages often allow (or even enforce) trailing commas on multiline expressions even, because it simplifies refactoring and diffs in the future.
If you want to add a trailing argument in the future and your
vkey
doesn't have a trailing comma already, your diff will contain changes to two lines, even though one of them only adds a comma. This also makes git blame harder to use. Realistically it's unlikely someone is going to write a better VCS anytime soon either, that would understand this change contextually.In Go for instance this will yield an
missing ',' before newline in argument list
error.Rust and Python (PEP8) follow the same reasoning after lengthy discussions each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the name of this method could probably be better - the important meat of what this function is trying to accomplish isn't the sending of the void keypress, it's the dismissing of the selection and the cursor visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion for the function name? I can't come up with anything. 😕
(If we had something different to
SendKeyEvent
- like e.g.GenerateKeyEvent
, which returns an optional input sequence - we could more comfortably abstract "reset selection and cursor" I suppose. That way you could move the "reset selection and cursor" handling to the more correct char handler, without the key event handler being responsible for this, right?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're just sending two constants over to
_TrySendKeyEvent()
, why not get rid of the helper function overall and just add a comment where you use it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that
_TrySendKeyEvent()
supports avkey == 0
seemed kinda hacky to me and I wanted to hide this hacky fact even from calls within the same class. So I created a hacky-wrapper function and prevent other methods from being "spoiled". 😄Do you want me to inline this function?