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

Feedback requested: Read INPUT_RECORDs on Windows #140

Closed
wants to merge 13 commits into from

Conversation

erikgeiser
Copy link
Contributor

@erikgeiser erikgeiser commented Oct 4, 2021

This PR implements reading INPUT_RECORDs on Windows in order to support richer keyboard and mouse events and window resize events. Fixes #121.

@erikgeiser erikgeiser changed the title WIP: Read INPUT_RECORDs on Windows Feedback requested: Read INPUT_RECORDs on Windows Oct 17, 2021
@jon4hz
Copy link
Contributor

jon4hz commented Nov 8, 2021

I quickly tested the changes on a linux machine and could not see any change. Window resizes and all other events are detected correctly. I'll post an update, when I get the chance to test it windows too

@erikgeiser
Copy link
Contributor Author

Thanks for your feedback. This PR should not affect Linux or macOS, only Windows. The most critical change is the conversion from Windows INPUT_RECORDs to bubbletea key events, so the key handling could be different for some key combinations or special keys.

@meowgorithm
Copy link
Member

Tested a little bit on Windows today, @erikgeiser, and so far so good.

@meowgorithm
Copy link
Member

meowgorithm commented Dec 20, 2021

I tested this a bit more and I must say…window resize events on Windows? What world is this?! It even works in the ol’ Windows Command Console. Absolutely wonderful. Really nice work, @erikgeiser.

I've rebased on master and then pushed a bunch of commits (which we'll squash) to bring this branch to parity with the latest release. I also added a resize example for verifying resizing on Windows. So far, everything looks good. I still need to test IME on Windows, which can input multiple characters at once, however based on the code I have read I expect IME to work as expected.

So, we should test this a little bit more, but I believe we should be able to merge this one soon.

@antonmedv, @jon4hz: heads up.

@erikgeiser
Copy link
Contributor Author

Glad to help. I wrote the INPUT_RECORD->mouse/keybaord event conversion the way it seemed to make sense but I don't really use CLI tools on Windows so I didn't test it that much. So I'm a bit worried that the event mapping for more complicated inputs could be a bit off. If you want to make sure nothing breaks compare complicated button combinations or special keys and compare the events that are emitted on Windows with this PR and on Linux/macOS.

@meowgorithm
Copy link
Member

meowgorithm commented Dec 20, 2021

This is a situation where tests could help, though I wonder how possible it is to synthesize key and mouse events on Windows for something like this.

In a worst case, if mappings are indeed off, I suppose we could fall back to using the function that’s parsing unix key data for keyboard input as we were previously with Windows, though we’d lose the additional metadata Windows gives us. My hunch is that everything’s fine, though.

@erikgeiser
Copy link
Contributor Author

erikgeiser commented Dec 20, 2021

It would be easy to fix the mapping. I'm talking about some key events that have different names in the Windows API vs. in the Unix ecosystem. So it would be possible that I mis-assigned some things but that would be trivial to fix.
Nobody has noticed anything yet, so if there are still minor errors, I'd expect them to occur in rare key events or other edge cases.

I'm talking about key_windows.go, especially the keyType function. Very trivial code, just mappings, easily fixable.

@meowgorithm
Copy link
Member

meowgorithm commented Dec 20, 2021

That's reassuring. To that note I'm noticing the shift+tab keystroke isn't working in this PR (it registers as simply tab). My brief research suggests that shift may require special handling for Windows.

On that note we should also continue to test with modifiers and special keys.

Also, I'm finding that IME on Windows is working as expected, which is good.

@erikgeiser
Copy link
Contributor Author

that note I'm noticing the shift+tab keystroke isn't working in this PR (it registers as simply tab).

Yeah this is exactly the kind of stuff that I meant. Unfortunately I currently don't have much time to debug this. If someone wants to try, my suggestion would be to look at the INPUT_RECORDS that pass through parseInputMsgsFromInputRecord and see if it is possible to distinguish tab from shift+tab with the flags provided by the struct. Maybe there is an easy solution.

If there is no easy way we might have to introduce some state, like a shiftDown variable that is set to true when a shift press is registered and to false when a shift release is registered. Then we can emit shift+x events if shiftDown is true.

@jon4hz
Copy link
Contributor

jon4hz commented Dec 29, 2021

I played around with it a bit on Windows 11 and I was able to notice the following things:

  • Pasting to a textinput adds unwanted '\x00' inputs
  • Shift+Tab only returns Tab
  • Trying the "mouse example" causes a crash but inputs are still getting parsed and printed to stdout

I've given it a try to fix some of those errors and came to the following solutions:

  • Adding a case here to handle '\x00' fixes the pasting problem
case '\x00':
	return KeyNull
  • Checking if shift is pressed down before returning the KeyType here
case coninput.VK_TAB:
	if e.ControlKeyState.Contains(coninput.SHIFT_PRESSED) {
		return KeyShiftTab
	}
	return KeyTab

On Windows 10 I also had a problem with highlighting text, which I could not reproduce even Windows 11. No idea what that might be.

@meowgorithm meowgorithm mentioned this pull request Jan 11, 2022
@meowgorithm meowgorithm added the enhancement New feature or request label Jan 11, 2022
@michaeltlombardi
Copy link

michaeltlombardi commented Apr 19, 2022

@meowgorithm, this note in #222 implies that the (now-merged) PR invalidates this one - I'm not sure if that means this PR needs to be reworked or is no longer necessary?

I've been working on a Bubble Tea-based project and my primary development environment is Windows, so if there's anything I can do to help get support for a richer Windows experience (particularly interested in window resizing for my own needs), I'm happy to test my way through a checklist or do anything else y'all might find useful. I'm still a bit of a neophyte wrt go and terminal development, so I don't know how effective I'll be at modifying the implementation, but I'm very skilled at setting things on fire. 😅

@meowgorithm
Copy link
Member

@michaeltlombardi alas, you're correct that this is now invalid — however there's still an enormous amount of value in the code that Erik has written here, such as the code for reading window resizes.

Other than window resizes, is there anything you're keen on seeing implemented?


Note for implentors: additional details from Erik's research can be found in #121 (comment)

@erikgeiser
Copy link
Contributor Author

erikgeiser commented Apr 19, 2022

@michaeltlombardi the reason this PR clashes with #222 is the following:

  • I first re-worked the input logic to support cancellation using OS specific APIs. What all implementations had in common was that reading console input produced the same byte sequences representing the input. This logic was later (after this PR here) moved into a separate package (https://github.com/muesli/cancelreader).
  • However, the Windows API for this is old and superseded by a more high level API that supports much more such as resize events and generally more complex input events. The problem is that this API does not produce the aforementioned byte sequences anymore, but Windows specific input structs.
  • In order to provide a common bubbletea-internal API that is not OS-specific, I had to increase the abstraction level from Read() -> byte sequence to Read() -> tea.Msg. This way it was possible to hide the fundamentally different implementations for Windows (which is Read() -> INPUT_RECORD -> tea.Msg under the hood) and all other OS's (with is Read() -> byte sequence -> tea.Msg under the hood)
  • This higher-level abstraction is not incompatible with the the old logic. When this PR was created, this was no problem because the logic could just be updated. However, due to Move cancelreader into a separate package #222 the old logic is not present anymore as it is now in https://github.com/muesli/cancelreader.

What now?

For this PR to be merged, the complete input logic would have to be pulled into bubbletea again, which is probably not a good idea. I also don't see how the changes could be moved into https://github.com/muesli/cancelreader because the high-level API is tied to bubbletea (remember Read() -> tea.Msg) and the package is meant for general usage. Re-building the high-level API within bubbletea based on https://github.com/muesli/cancelreader could be solution for this problem but it would require a bit of work.

As this PR has been stale for so long, I have since moved on. I also don't really need this stuff for anything myself, I only implemented it because I thought I could help using the experience I gained from implementing the cancelable reader in the first place.

@michaeltlombardi
Copy link

@meowgorithm a little hard to tell as:

  • my implementation so far is neither clever nor extensive
  • I think I saw elsewhere a comment that mentioned there wouldn't be any features officially supported which are not xplat (so for example if other terminals can't tell if capslock is on, that functionality wouldn't get more explicit support)
  • I'm not wholly clear on what works in not-Windows that this would enable for Windows builds

That being said, if this adds support for double-clicks/additional mouse button clicks, that could be useful; right now my implementation has stayed keyboard-only but I can see a use for projects where highlighting and being more click-aware would be helpful for navigation in particular (for example, click to get a modal equivalent, double-click to switch views).

@meowgorithm
Copy link
Member

So, as @erikgeiser mentioned, at this point the codebase has moved far, far away this PR. While it's time to close it, I would love it, Erik, if you could keep your fork and branch around for posterity and research as there's an enormous amount of knowledge in there that will inevitably need to be referenced in the future.

And thank you for all your work both on this and cancelreader which, as I'm sure you’re aware, has become a crucial part of Bubble Tea.

aymanbagabas added a commit that referenced this pull request Dec 3, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 3, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 4, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Dec 15, 2023
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Feb 27, 2024
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Feb 27, 2024
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
aymanbagabas added a commit that referenced this pull request Feb 27, 2024
This adds support to the Windows Console Input Buffer API which access
the console API directly without the need for virtual terminal input
(i.e. the current mode that emulates unix inputs).
Since this uses the console input api, we can finally read window size
events.

This is mearly based on the awesome work of @erikgeiser in #140.

Fixes: #538
Fixes: #121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read input events instead of raw bytes in Windows (mouse support/window change events/canceling inputloop)
4 participants