Skip to content

Conversation

obsoleszenz
Copy link

This pr adds the text representation of the key to the EguiEvent::Key struct.
This way we get the character that the keyboard layout actually represents and not only
the key combination. For example on EurKey layout, if i press AltRight+A, it's mapped to "ä".
Key { key: A, physical_key: Some(A), pressed: false, repeat: false, modifiers: Modifiers::NONE, text: Some("ä") }

@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch from c4f25d2 to dc46a8a Compare May 31, 2025 13:25
@obsoleszenz obsoleszenz changed the title Extend EguiEvent::Key of it's text representation Feature: Extend EguiEvent::Key of it's text representation May 31, 2025
@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch from dc46a8a to 66f5b26 Compare May 31, 2025 13:52
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I need to understand this a bit better though, and a docstring would be a mighty fine way :)

Also: we should implement this on web too!

@obsoleszenz
Copy link
Author

@emilk If you understand this better and approve it to be the way to go i will write a docstring and add the lost comment back :)

@emilk
Copy link
Owner

emilk commented Jun 24, 2025

I think this makes sense, but I still haven't understood a concrete use case for this. If you use KeyEvent::text and ignore Event::Text, then you miss some events (e.g. IME text, clipboard paste text, etc). If you use both, then you get duplicated text.

What is the concrete problem you are trying to solve?

@obsoleszenz
Copy link
Author

obsoleszenz commented Jun 30, 2025

I think this makes sense, but I still haven't understood a concrete use case for this. If you use KeyEvent::text and ignore Event::Text, then you miss some events (e.g. IME text, clipboard paste text, etc). If you use both, then you get duplicated text.

What is the concrete problem you are trying to solve?

I give you an example of what i currently do:
I'm using egui mainly for painting, not using the input widgets. But i have my own implementations of it, as i focus on having the whole app controllable with the keyboard (think game).
So for example i have a popup which allows editing some text, one can open it by pressing Esc, followed by e. If the user now presses any keys it will edit/insert the text, escape closes the popup, enter saves. But I want to support ä/ü... which is mapped on my keyboard to Alt+a & Alt+u.

I get following events:

  • Egui::Event::Key { ... Escape } => Open main menu popup
  • Egui::Event::Key { ... 'e' } => switch to edit text popup
  • Egui::Event::Text("e") => We now insert text "e" and there's no way to know that we processed this event as Event::Key already
  • Egui::Event::Key(Alt(a)) => We don't know that this represents "ä", so we ignore
  • Egui::Event::Text("ä") => Insert "ä"

@emilk
Copy link
Owner

emilk commented Jul 7, 2025

Thanks for explaining this in more detail! It seems the core need is to be able to match a specific Key event to a specific Text event, and the other way around. The proposed solution is one way of doing so, but I'm not sure it's the best way.

Or rather, maybe it doesn't go far enough 🤔 If we add Option<String> to Event::Key, we should also remove Event::Text. I earlier misremembered and thought we used Event::Text for non-key events too, like paste and IME, but those have their own events. So I think Event::Text is safe to be removed (or rather moved into Event::Key).

@obsoleszenz
Copy link
Author

obsoleszenz commented Jul 10, 2025

Thanks for explaining this in more detail! It seems the core need is to be able to match a specific Key event to a specific Text event, and the other way around. The proposed solution is one way of doing so, but I'm not sure it's the best way.

Or rather, maybe it doesn't go far enough 🤔 If we add Option<String> to Event::Key, we should also remove Event::Text. I earlier misremembered and thought we used Event::Text for non-key events too, like paste and IME, but those have their own events. So I think Event::Text is safe to be removed (or rather moved into Event::Key).

Happy that I could clarify :)
Yes I think removing Event::Text is the way to go.
I think we can introduce text: Option<String> and
mark Event::Text as deprecated with a note to use
Event::Key.text instead.

If we agree on this i can update this PR to do that.

@emilk
Copy link
Owner

emilk commented Aug 5, 2025

go for it!

@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch 8 times, most recently from 78f4fac to 0673b6a Compare August 11, 2025 12:32
@obsoleszenz
Copy link
Author

obsoleszenz commented Aug 11, 2025

@emilk added docstring, deprecated Event::Text and changed all usages in the codebase to Event::Key.

@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch 3 times, most recently from 5f4d47e to a3844d6 Compare August 11, 2025 13:08
@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch from a3844d6 to 832db73 Compare August 11, 2025 13:11
@obsoleszenz obsoleszenz force-pushed the feat_egui_event_key_include_text branch from 832db73 to 92d27d4 Compare August 11, 2025 13:13
@obsoleszenz obsoleszenz requested a review from emilk August 11, 2025 13:15
/// Helpful if you relied on creating [`Event::Text`] before it's deprecation.
pub fn from_text(text: String) -> Self {
Self::Key {
key: Key::from_name(&text.chars().nth(0).unwrap_or('a').to_string()).unwrap_or(Key::A),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this at all - this means any non-recognized text will be interpreted as a Key::A press, which makes no sense

Copy link
Author

@obsoleszenz obsoleszenz Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair! I see following possibilities:
a) Fail if we can't get a Key from the text
b) create a Key::Unknown
c) Make Event::Key.key optional
d) you have a better idea :P

Which one you prefer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and implemented solution a) as I think it's fine to panic in case text is empty or the first character is not representable as Key.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilk friendly ping (:

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.

2 participants