bubbletea icon indicating copy to clipboard operation
bubbletea copied to clipboard

Feedback requested: Read INPUT_RECORDs on Windows

Open erikgeiser opened this issue 2 years ago • 14 comments

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

erikgeiser avatar Oct 04 '21 15:10 erikgeiser

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

jon4hz avatar Nov 08 '21 19:11 jon4hz

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.

erikgeiser avatar Nov 08 '21 19:11 erikgeiser

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

meowgorithm avatar Nov 08 '21 19:11 meowgorithm

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.

meowgorithm avatar Dec 20 '21 03:12 meowgorithm

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.

erikgeiser avatar Dec 20 '21 09:12 erikgeiser

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.

meowgorithm avatar Dec 20 '21 14:12 meowgorithm

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.

erikgeiser avatar Dec 20 '21 16:12 erikgeiser

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.

meowgorithm avatar Dec 20 '21 19:12 meowgorithm

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.

erikgeiser avatar Dec 21 '21 14:12 erikgeiser

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.

jon4hz avatar Dec 29 '21 20:12 jon4hz

@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. 😅

michaeltlombardi avatar Apr 19 '22 20:04 michaeltlombardi

@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 https://github.com/charmbracelet/bubbletea/issues/121#issue-970564727

meowgorithm avatar Apr 19 '22 20:04 meowgorithm

@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 #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.

erikgeiser avatar Apr 19 '22 21:04 erikgeiser

@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).

michaeltlombardi avatar Apr 19 '22 21:04 michaeltlombardi

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.

meowgorithm avatar Aug 18 '22 19:08 meowgorithm