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

windows: improve WIN32ungetch #388

Merged
merged 1 commit into from
Jun 17, 2023
Merged

windows: improve WIN32ungetch #388

merged 1 commit into from
Jun 17, 2023

Conversation

avih
Copy link
Contributor

@avih avih commented Jun 17, 2023

WIN32ungetch does serve the purpose it was added for - unget a zero value which is the first-of-two return values of a scan code.

However, there were some cases which never worked, like ungetting any UTF-8 byte except the first (of a sequence of up to 4), where the next WIN32getch call simply ignores the ch argument to WIN32ungetch.

Additionally, some other cases got broken at 6fd6739 , because the unget code was depending on buggy unicode behavior - which is now fixed. E.g. unget the second-of-two scan code return values is broken.

Instead of trying to modify the quite-complex KB queue state (unicode state, x11 mouse state, scan code state) in a way which will restore the current state after the next WIN32getch call, this commit simply bypasses the KB queue state, and should ensure it always works.

WIN32ungetch does serve the purpose it was added for - unget a zero
value which is the first-of-two return values of a scan code.

However, there were some cases which never worked, like ungetting
any UTF-8 byte except the first (of a sequence of up to 4), where the
next WIN32getch call simply ignores the ch argument to WIN32ungetch.

Additionally, some other cases got broken at 6fd6739 , because the
unget code was depending on buggy unicode behavior - which is now
fixed. E.g. unget the second-of-two scan code return values is broken.

Instead of trying to modify the quite-complex KB queue state (unicode
state, x11 mouse state, scan code state) in a way which will restore
the current state after the next WIN32getch call, this commit simply
bypasses the KB queue state, and should ensure it always works.
@avih avih mentioned this pull request Jun 17, 2023
@gwsw gwsw merged commit 6f0e68c into gwsw:master Jun 17, 2023
@avih avih deleted the win32-ungetch branch June 18, 2023 03:08
@avih
Copy link
Contributor Author

avih commented Jun 18, 2023

Thanks for merging it.

I noticed that the commit was merged with the author avih <avih@users.noreply.github.com>, but this PR has a more detailed author - https://patch-diff.githubusercontent.com/raw/gwsw/less/pull/388.patch

Did github change the author during the merge? I don't recall it happening in the past with PRs...

Also, just noticed, for completeness, win32_kbhit should probably return true if win_unget_pending == TRUE.

@gwsw
Copy link
Owner

gwsw commented Jun 18, 2023

I don't know why the Author name is set like that. I just clicked "Squash and merge" like I always do to merge a PR.
I have added the check in win32_kbhit as you suggest.

@avih
Copy link
Contributor Author

avih commented Jun 18, 2023

I don't know why the Author name is set like that. I just clicked "Squash and merge"

Hmm, I'll try to figure out why that happened.

I have added the check in win32_kbhit as you suggest.

Yeah, I noticed. Thanks.

@avih
Copy link
Contributor Author

avih commented Jun 18, 2023

I don't know why the Author name is set like that. I just clicked "Squash and merge" like I always do to merge a PR.

Right. I've been told it might have been the "squash" part.

You could have also done "Rebase and Merge", which should be effectively the same as "rebase and squash" if there's only one commit - as was the case here, though possibly without github rewriting the author.

If the PR has more than one commit then "rebase and merge" would also preserve the individual commits, if you want that.

For what it's worth, In other projects when I merge a PR I typically use "rebase and merge" (and if it needs to be squashed first, then I'd typically ask the author to do that, if they know how).

So just FYI, and maybe next time try "Rebase and merge" ;)

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.

None yet

2 participants