veeso / tui-realm

👑 A ratatui framework to build stateful applications with a React/Elm inspired approach
MIT License
590 stars 25 forks source link

[BUG] - Windows Crossterm Duplicate Key Event #54

Closed k86td closed 1 year ago

k86td commented 1 year ago

Description

Crossterm will register multiple KeyPress types. In a Windows environment, I was getting twice the keypresses instead of once. It would be interesting to allow the user to target specific KeyPressKind to avoid this issue.

Steps to reproduce

Clone the repo and run the demo example from the examples folder. When pressing keys, the KeyPress message is sent 2 times (KeyEventKind::Pressedand KeyEventKind::Released). This results in the counter being incremented twice, instead of once.

Expected behaviour

The user should be able to define the KeyEventKind when listening for KeyEvent to avoid receiving twice the same KeyEvent message.

Environment

Additional information

See this issue for reference and more information. The actual docs for the KeyEvent in crossterm are here.

Fix (?)

I think changing the struct of KeyEvent to allow to specify KeyEventKind and passing it to crossterm would work.

strowk commented 1 year ago

This seems to also cause demo app to switch focus on Tab twice, making it impossible to activate digit counter and effectively disabling this framework for windows users.

Note recommendation from ratatui is

check KeyEvent.kind == KeyEventKind.Press https://github.com/ratatui-org/ratatui/issues/347#issuecomment-1651028733

The problem is that while crossterm has this "kind" field, it is lost in here: https://github.com/veeso/tui-realm/blob/bb2fe5f448b5d1bd370eb8a41d57535283e26267/src/core/event.rs#L78

And termion does not have that field at all (I guess it sends event only on press?): https://docs.rs/termion/latest/termion/event/enum.Event.html

Maybe defining enum KeyEventKind with variants Unknown, Press, Release would be solution? Unknown would be field for termion and any backend that does not support the distinction, Press and Release corresponding to this - https://docs.rs/crossterm/latest/crossterm/event/struct.KeyEvent.html

Note that crossterm docs mention that on unix the kind might also be set in case if configured:

Only set if:

Unix: KeyboardEnhancementFlags::REPORT_EVENT_TYPES has been enabled with PushKeyboardEnhancementFlags. Windows: always

strowk commented 1 year ago

An attempt at fix: https://github.com/strowk/tui-realm/commit/fa784b9440cc8c883e83963134d0c6c9484c7f3a

The issue with this is that it introduces back incompatibility, since all apps which were pattern matching on current KeyEvent would get error now, so strictly speaking this can only go into version 2 :(

If you run into this and need workaround, you are welcome to use my temp fix like this:

tuirealm = { git = 'https://github.com/strowk/tui-realm.git', branch = "issue-54-fix" }
veeso commented 1 year ago

fixed 1.9.1