Add key repetition #16

trimental · 2018-07-15T12:35:15Z

This pull request adds key repetition as a possible utility for the keyboard module. The work isn't there yet for release as there will at least need to be a setting for enabling/disabling it and bitflags to control what keys are repeatable. I'm pull requesting to see your views on the key repetition and whether it would be suitable for the client toolkit. I saw the pull request for winit but that looked pretty stale and I think it would fit better as an option in the client toolkit then in winit. I used threads, channels and sleeping as opposed to something like tokio as its simpler and lighter, the only downside I can think of is that threads might last a few hundred milliseconds longer due to sleeping (which I'm not sure whether that matters?) however cpu wise it should be pretty light. I don't know how releases work but maybe it would be a minor breakage if it was defaulted to off?

trimental · 2018-07-15T13:21:55Z

Doesn't build on 1.20.0 but I plan to change the way repeatable characters are chosen anyway

trimental · 2018-07-15T14:00:04Z

Alacrity works but only displays the characters as you hold down the key if you also simultaneously hovering your mouse over the buttons which also means that the hover colors are properly displayed as well. Very strange! Maybe alacrity, glutin or winit suppress window drawing which would explain the buttons? or maybe a bug.

elinorbgr · 2018-07-16T22:38:42Z

Hmm, I'm a little uneasy about integrating it like that. While I agree it would be good to integrate key repetitions, I feel like implementing it using threads is pretty eavy, especially if it means spawning a new thread every key press.

Also, what you encounter here:

Alacrity works but only displays the characters as you hold down the key if you also simultaneously hovering your mouse over the buttons which also means that the hover colors are properly displayed as well.

Is likely because the winit event loop only waits on events from the wayland connection, and not on events from your repetition thread. As such, if you don't move the pointer, no wayland events are generated, and the event loop remains stuck.

Because of this, I don't think it'll be possible to implement key repetition in a way that "just works", without special integration in the event loop (by making it possible to wait on more event sources than just the wayland socket), and thus cooperation from the downstream user.

trimental · 2018-07-17T00:22:44Z

I also think the current implementation is crude at present but I am unsure what you mean by not using threads, I think to do something like this you need to use threads either directly or indirectly. As for the threads spawning every key press I can make it so only one thread exist for the key repetition and thread sleeping can be replaced by tick from the chan crate. That being said if you want it done without threads directly I can use something like tokio. 👍

elinorbgr · 2018-07-17T07:09:10Z

In all cases, independently of the actual way it is implemented:

there should be a flag at keyboard creation for the user to request what kind of key repetition they want, probably something like:

pub KeyRepeatKind {
    None, // no key repeat
    Fixed { rate: u64, delay: u64 }, // use a fixed rate/delay configuration
    System, // use the rate/delay configuration provided by the server
}

the key events should contain something allowing the user to differentiate repetition event from actual keypresses

That said, sorry I had glanced too quickly at your code yesterday, I see you are directly calling the callbacks from the secondary thread.

By my other message still explains your weird bug though: winit actually buffers the events generated in the event_loop.dispatch() call, and only then sends them to the user callback. Meaning it still only blocks on the wayland socket, and the key repetition events will only be properly processed only if other wayland events are generated as well (by example with the user moving the pointer).

I'm not completely clear about what would be the best way to tackle this cleanly, though.

trimental · 2018-07-17T10:02:28Z

I have made the chan crate handle the timing and interaction between the main and secondary thread, this should now mean the secondary thread is immediately closed when not needed.
Technically you were correct, every key press spawns a new thread that controls the sending of that key's events however its designed so that only one thread can exist at any time. If you wanted to you could have one thread made at the start of the keyboard setup and then send the current key through a channel to that thread but you have to weight the resource cost of keeping a thread open even when not needed + sending objects through channels.

trimental · 2018-07-18T10:23:52Z

Hopefully this should be pretty near complete. I have a question about the time on the event, would it be better to use rate and delay to increment the time member on the key event when it's sent or set it to none or something, I ask because I'm not sure how time is calculated or how its used.

heghe · 2018-07-18T13:26:29Z

@trimental Just want to mention that chan crate seems to be EOL and it is replaced by https://github.com/crossbeam-rs/crossbeam-channel. See last comment here BurntSushi/chan#25

trimental · 2018-07-18T13:38:37Z

@heghe crossbeam would be great to replace chan here :) but unfortunately it’s minimum supported rust version is 1.26 and this project has a minimum of 1.20. Technically this is a breaking change pull request so the minimum supported version could be bumped but I assume @vberger wouldn’t want to do that.

trimental · 2018-07-18T14:02:34Z

Mm I’m starting to question if the chan/crossbeam crates are either A - important for function or B - performance efficient when compared to the original thread sleep way. With respect to A it should function the same if a thread kill check is done at least before the sending of an event and with respect to B it should be just as fast if not faster maybe. I think the chan crate might just be adding a nice front end to the original way.

trimental · 2018-07-19T04:18:36Z

Looks like it's tempfile that is actually messing up the build process, they upgraded rand to 5 and released it as a non-major release. The fix would be bumping the minimum supported rust version to 1.22, downgrading tempfile or removing tempfile. Regardless of this I would still prefer to not use the chan crate, I don't think it provides a lot for this usage scenario.

…e unused AsciiExt

elinorbgr · 2018-07-23T06:26:48Z

Re-reading your code, I think it could use some refactoring:

some of your if/else have code shared between the two branches, that can be moved out of them to de-duplicate
the new implement_kbd_with_repeat function shares a lot of code with implement_kbd, as a large part of the two implementation is identical. This could be solved by factoring this logic into a new struct implementing the appropriate Implementation<...> trait.

trimental · 2018-07-23T08:17:52Z

The commit bf3056e changes the behavior of key repetition to this

All key events are guaranteed to send a key press and key release event
Only the last pressed repeatable key sends repeat events

Due to these guarantees, if you ignore key repeat events then map_keyboard_with_repeat will essentially be map_keyboard. Press events of key A will never occur while repeating key B, release of events of key A may occur when repeating key B. I think this is the best behavior for key repetition

trimental · 2018-07-23T08:49:22Z

I think I need some more clarification on

struct implementing the appropriate Implementation<...> trait.

do you mean a alternative to kbd.implement() like repeat_kbd.implement()?

elinorbgr · 2018-07-23T09:04:47Z

Press events of key A will never occur while repeating key B, release of events of key A may occur when repeating key B.

Hmm, I'm not sure I understand what you mean here. I'll put an example of what I think is the expected behavior:

user presses a
- a key press event for a is generated
- a starts repeating
user presses <shift>
- a stops repeating
- a key press event for <shift> is generated
- A starts repeating
user releases <shift>
- A stops repeating
- a release event for <shift> is generated
- a starts repeating
user presses b
- a stops repeating
- a press event for b is generated
- b starts repeating
user releases b
- bstops repeating
- a release event for b is generated
user releases a
- a release event for a is generated

Do we agree on this stream of events?

do you mean a alternative to kbd.implement() like repeat_kbd.implement()?

I mean, when implementing the NewProxy<WlKeyboard>, rather than giving this huge closure to the implement() method, instead add a new struct KbdImplementation which implements the trait Implementation<Proxy<WlKeyboard>, wl_keyboard::Event>:

impl Implementation<Proxy<WlKeyboard>, wl_keyboard::Event> for KbdImplementation {
    fn receive(&mut self, event: wl_keyboard::Event, kbd: Proxy<WlKeyboard>) {
        /* contents of the big closure */
    }
}

This would allow to more easily factor code between the two implementations, and possibly split the whole logic into a few methods of KbdImplementation for improved readability.

trimental · 2018-07-23T11:14:14Z

I'm sorry but I'm not that use to the Implement trait, I got something like this

struct KbdImplementation {
    state: KbState,
    key_repeat_kind: KeyRepeatKind,
    kill_chan: Arc<Mutex<(mpsc::Sender<()>, mpsc::Receiver<()>)>>,
    key_held: Option<u32>,
    system_repeat_timing: Arc<Mutex<(u64, u64)>>,
}

impl Implementation<Proxy<wl_keyboard::WlKeyboard>, wl_keyboard::Event> for KbdImplementation {
    fn receive(&mut self, event: wl_keyboard::Event, kbd: Proxy<wl_keyboard::WlKeyboard>) {
// Big closure matching event
   }
}

does KbdClosure need to take user_impl and repeat_impl?

trimental · 2018-07-23T11:23:10Z

I might have to complete this pull request without this and leave you to implement it later if I can't figure it out from your explanations.

trimental · 2018-07-23T11:59:31Z

Unless I'm mistaken you could also maybe have this

fn implement_kbd<Impl, RepeatImpl>(
    kbd: NewProxy<wl_keyboard::WlKeyboard>,
    mut state: KbState,
    mut user_impl: Impl,
    repeat: Option<(RepeatImpl, KeyRepeatKind)> 
) -> Proxy<wl_keyboard::WlKeyboard>

map_keyboard would pass none and map_keyboard_with_repeat would pass values. Then it would be as simple as a if let in the Key Press arm of the event match. Your way might be better though as I don't fully understand it yet.

elinorbgr · 2018-07-23T12:08:09Z

The idea of the 'Implementation' trait is more or less the concretisation of the fact that a closure if really just a struct that happens to be callable. As such, if you want to transform a closure into a struct, you indeed have to create a struct whose fields are exactly the variables that the closure captured. Then, in the implementation, just use the same code as what the closure had, replacing the captured variable by access to the fields. When doing this kind of migration, I just tend to move the code from the closure to the impl block then follow the compiler error messages until it compiles. ^^" As an example, you can see some direct implementations in smithay, here for example: https://github.com/Smithay/smithay/blob/master/src/wayland/shm/mod.rs

elinorbgr · 2018-07-23T12:09:45Z

Indeed, having an Option argument would work too, if you prefer that way. :)

trimental · 2018-07-23T12:20:11Z

When I did

move the code from the closure to the impl block then follow the compiler error messages until it compiles."

I was left with a final error message saying this for the Impl as well as RepeatImpl

wayland_commons::Implementation<wayland_client::Proxy<wayland_client::protocol::wl_keyboard::WlKeyboard>, keyboard::Event<'a>> + std::marker::Send + 'static` does not have a constant size known at compile-time

I'll write it as a Option for now but you can change it to a struct Implementation if you can get it working

trimental · 2018-07-23T14:23:00Z

That should be almost everything apart from one last thing. At the moment the behavior is like this

user presses v key
v key press event sent
v starts to repeat
user presses shift key
shift key press event sent
V starts to repeat
user releases v key and shift key
shift key release event sent
V key release event sent

Note that although the key changes from v to V, neither the release event for v, nor the press event for V is sent to the user. I want to know whether you think this is the best behavior or whether to send a release for v and a press for V when shift is pressed.

trimental · 2018-07-23T14:47:38Z

Also I forgot to answer

Hmm, I'm not sure I understand what you mean here. I'll put an example of what I think is the expected behavior:

Yes that should be the behavior

elinorbgr

Okay, this is pretty good, I've left a few points, most are me being nitpicky.

Sorry for the long back and forth on this PR, thanks for enduring me this whole time 😅

elinorbgr · 2018-07-23T17:39:37Z

src/keyboard/mod.rs

+    pub keysym: u32,
+    /// utf8 interpretation of the entered text
+    ///
+    /// will always be `None` on key release events


The second line of this comment is no longer relevant I think

elinorbgr · 2018-07-23T17:48:16Z

src/keyboard/mod.rs

+    state: Arc<Mutex<KbState>>,
+    key_repeat_kind: Option<KeyRepeatKind>,
+    mut event_impl: Impl,
+    repeat_impl: Option<Arc<Mutex<Implementation<Proxy<wl_keyboard::WlKeyboard>, KeyRepeatEvent> + Send>>>,


I think it'd be better if the wrapping in Arc<Mutex<...>> was done inside of this function, as the outside world never need to access them anyway.

So, I'd rather see its prototype as:

fn implement_kbd<Impl, RepeatImpl>( kbd: NewProxy<wl_keyboard::WlKeyboard>, state: KbState, event_impl: Impl, repeat: Option<(KeyRepeatKind, RepeatImpl)> ) -> Proxy<wl_keyboard::WlKeyboard> where: for<'a> Impl: Implementation<Proxy<wl_keyboard::WlKeyboard>, Event<'a>> + Send, RepeatImpl: Implementation<Proxy<wl_keyboard::WlKeyboard>, KeyRepeatEvent> + Send { /* ... */ }

elinorbgr · 2018-07-23T17:49:24Z

src/keyboard/mod.rs

    kbd.implement(
        move |event: wl_keyboard::Event, proxy: Proxy<wl_keyboard::WlKeyboard>| {
            match event {
                wl_keyboard::Event::Keymap { format, fd, size } => {
+                    let mut state = state.lock().unwrap();


The state should be locked once and for all before the match, rather than once in each arm.

elinorbgr · 2018-07-23T17:51:46Z

src/keyboard/mod.rs

+                        );
+                        if let Some(repeat_impl) = repeat_impl.clone() {
+                            // Check with xkb if key is repeatable
+                            if unsafe { (XKBH.xkb_keymap_key_repeats)(state.lock().unwrap().xkb_keymap, key + 8) == 1 } {


I think the "check if a key is repeatable" functionality should rather be a method of KbState, to contain all FFI at the same place.

elinorbgr · 2018-07-23T17:59:50Z

src/keyboard/mod.rs

+                                                thread_sym = thread_state.lock().unwrap().get_one_sym_raw(key);
+                                                thread_utf8 = { 
+                                                    let mut thread_state = thread_state.lock().unwrap();
+                                                    if thread_state.compose_feed(sym)


I don't think we should check compose on key repeat like this. Compose does not really make sense for key repetition...

And on my computer, if I do ^ then e (which compose into ê) and let e pressed, I get êeeeeeeeeeeee. Only the first letter gets composed.

So, I'd say this whole arm of the match should be simplified to

let mut thread_state = thread_state.lock().unwrap(); thread_sym = thread_state.get_one_sym_raw(key); thread_utf8 = thread_state.get_utf8_raw(key); thread_modifiers = thread_state.mods_state.clone();

elinorbgr · 2018-07-23T18:02:30Z

src/keyboard/mod.rs

-                } => state.update_modifiers(mods_depressed, mods_latched, mods_locked, group),
+                } => {
+                    state.lock().unwrap().update_modifiers(mods_depressed, mods_latched, mods_locked, group);
+                    state_chan.lock().unwrap().0.send(()).unwrap();


Doesn't that stop any current repetition if one was going on?
Pressing a modifier should change the repeated key but not stop the repetition.

Also, what's the impact of sending a message in this channel if no repetition is going on?

src/keyboard/mod.rs

+/// as such you need to call this method as soon as you have created the keyboard
+/// to make sure this event does not get lost.
+///
+/// Returns an error if xkbcommon could not be initialized.


elinorbgr · 2018-07-24T09:11:47Z

src/keyboard/mod.rs

+                        );
+                        if let Some(repeat_impl) = repeat_impl.clone() {
+                            // Check with xkb if key is repeatable
+                            if unsafe { state.key_repeats(key + 8) } {


the + 8 offsetting should happen inside of the key_repeats method, like for the other methods of KbState.

elinorbgr · 2018-07-24T09:13:30Z

src/keyboard/mod.rs

+                    state.update_modifiers(mods_depressed, mods_latched, mods_locked, group);
+                    if key_held.is_some() {
+                        state_chan.lock().unwrap().0.send(()).unwrap();
+                    }


This code still means that pressing a modifier will stop any repeat, right? I don't think that's the expected behavior.

If I hold a on my computer and press then release <shift> several times, I get something like aaaaaaaaaaaaaAAAAAAAAAAaaaaaaaaaaAAAAAAaaaaaaaaaAAAAAaaaaaaaa.

Maybe you are thinking of kill_chan instead of state_chan, state_chan simply requests the thread to update modifiers when they are changed rather then updating the values every 30ms. I don't really know whether this is much more performant then just updating them for every key repetition though.

Actually re-reading that, is

aaaaaaaaaaaaaAAAAAAAAAAaaaaaaaaaaAAAAAAaaaaaaaaaAAAAAaaaaaaaa

not the expected behavior? what would be?

Maybe you are thinking of kill_chan instead of state_chan

Ah right, my bad. I've read it too quickly. No problem then.

trimental · 2018-07-24T09:23:28Z

I've tried to implement all of your review points, the only one I had difficulty with was

I think it'd be better if the wrapping in Arc<Mutex<...>> was done inside of this function

as I can't move RepeatImpl out of a container as it needs a size, unless I move it to generics, which I can't do because then all calls must provide a RepeatImpl object, when some pass None. (atleast I'm pretty sure thats why)

Sorry for the long back and forth on this PR, thanks for enduring me this whole time

Not at all, I love learning more about the wayland protocol and your reviews have been spot on, its best to get these things as stable as possible :)

elinorbgr · 2018-07-24T09:28:40Z

then all calls must provide a RepeatImpl object, when some pass None

Ah yes, I've seen this problem several times. Rustc can't infer the type parameter of None. I usually just type my Option with a dummy value, like None::<fn(_,_)>.

trimental · 2018-07-24T09:38:55Z

Works beautifully, the parameters and their calling looking a lot better then before

elinorbgr

Alright, this looks good now 👍 I'll just let the travis build finish before merging it.

I still think the implementation needs to be refactored into a stand-alone struct, this closure is getting far too big for its own right. But this can be done in a new PR, at some point.

Thanks!

trimental added 19 commits July 22, 2018 15:37

Add key repetition

46d75d2

Use AsciiExt for older rust versions and fix warnings

1d8161d

Derive default for ModifiersState

d45b354

Control key repetition delay with control statements

aadf70c

Use chan crate to control key repetition

1d84980

Add parameter in keyboard mapping for controlling key repetition

c2fdb05

Fix KeyRepeatKind documentation and idenitify repeated key events

0872611

Add bitflag controls for key repetition

a07ee1b

Fix boolean logic to calculate if a character is repeatable

0400e97

Reflow the checking for repeatable keys and fix shifted characters

96be173

Fix name of repeat timing variable

f038261

Hardcode values instead of is_ascii_punctuation for compatibility

4d50ad6

Downgrade chan for compatibility, fix doc for RepeatedKeyTypes, remov…

c451333

…e unused AsciiExt

Update kbd_input and selection to use key repetition

a584591

Remove unused import from selection example

7e9f5dd

Send release key events

dce3dc4

Revert chan to latest version

60f8aba

Replace chan crate with std::sync::mpsc

4c12c47

Calculate time dynamically when repeating key events

6e0096e

Fix key repeat logic

4ba2f11

Reflow key repetition logic

bf3056e

trimental added 2 commits July 23, 2018 21:07

Combine kbd_implement and kbd_implement_with_repeat

7e76f54

Dynamic state while repeating keys

2ba3dde

Remove unnecessary mut binding

68e325c

Directly pass state instead of through channel

c345d18

elinorbgr requested changes Jul 23, 2018

View reviewed changes

Key repetition cleanup

48a43c8

elinorbgr requested changes Jul 24, 2018

View reviewed changes

Use generics for RepeatImpl, cargo fmt

2287bc2

elinorbgr approved these changes Jul 24, 2018

View reviewed changes

elinorbgr merged commit 1691329 into Smithay:master Jul 24, 2018

trimental deleted the key-repetition branch July 24, 2018 09:52

trimental mentioned this pull request Aug 1, 2018

Update changelog for key repetition #22

Merged

trimental mentioned this pull request Aug 13, 2018

Add key repetition for the wayland backend rust-windowing/winit#628

Merged

2 tasks

Add key repetition #16

Add key repetition #16

Conversation

trimental commented Jul 15, 2018 • edited Loading

trimental commented Jul 15, 2018

trimental commented Jul 15, 2018

elinorbgr commented Jul 16, 2018

trimental commented Jul 17, 2018

elinorbgr commented Jul 17, 2018

trimental commented Jul 17, 2018 • edited Loading

trimental commented Jul 18, 2018 • edited Loading

heghe commented Jul 18, 2018

trimental commented Jul 18, 2018

trimental commented Jul 18, 2018

trimental commented Jul 19, 2018 • edited Loading

elinorbgr commented Jul 23, 2018

trimental commented Jul 23, 2018 • edited Loading

trimental commented Jul 23, 2018

elinorbgr commented Jul 23, 2018

trimental commented Jul 23, 2018 • edited Loading

trimental commented Jul 23, 2018

trimental commented Jul 23, 2018

elinorbgr commented Jul 23, 2018 via email

elinorbgr commented Jul 23, 2018 via email

trimental commented Jul 23, 2018

trimental commented Jul 23, 2018

trimental commented Jul 23, 2018

elinorbgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trimental commented Jul 24, 2018

elinorbgr commented Jul 24, 2018

trimental commented Jul 24, 2018

elinorbgr left a comment

Choose a reason for hiding this comment

trimental commented Jul 15, 2018 •

edited

Loading

trimental commented Jul 17, 2018 •

edited

Loading

trimental commented Jul 18, 2018 •

edited

Loading

trimental commented Jul 19, 2018 •

edited

Loading

trimental commented Jul 23, 2018 •

edited

Loading

trimental commented Jul 23, 2018 •

edited

Loading