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

Add key repetition for the wayland backend #628

Merged
merged 3 commits into from
Aug 19, 2018

Conversation

trimental
Copy link
Contributor

@trimental trimental commented Aug 13, 2018

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users

This PR adds the feature of key repetition to the wayland backend. This PR is requested prematurely so that it may be reviewed before the release of smithay-client-toolkit 0.3.0, currently it points to the git version as a dependency.

I have tested the changes and added a small update to the changelog (might need more explanation and better wording?), however I cannot find any documentation of key repetition (I assume this is just standard behavior).

Please look to Smithay/client-toolkit#16 for further information about the new map_keyboard_with_repeat() function

@elinorbgr
Copy link
Contributor

This is a simpler implementation of key repetition, based on a thread.

It has been developed in #371 that no background thread should be needed. I do have some plans to introduce some real, proper event loop capability to my wayland libs (in a way making it possible to have good support for secondary event sources, including timers). But I don't know when I'll have the bandwidth to do this (this is related to Smithay/wayland-rs#185 for those interested).

In the meantime, this works, and leaves room to easily implement stuff like disabling or discriminating key repetition events, depending on what is decided wrt #310.

Cargo.toml Outdated
@@ -55,7 +55,7 @@ features = [

[target.'cfg(any(target_os = "linux", target_os = "dragonfly", target_os = "freebsd", target_os = "openbsd", target_os = "netbsd"))'.dependencies]
wayland-client = { version = "0.20.10", features = [ "dlopen", "egl", "cursor"] }
smithay-client-toolkit = "0.2.6"
smithay-client-toolkit = { git = "https://github.com/Smithay/client-toolkit" }

Choose a reason for hiding this comment

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

You can use 0.3.0 now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to change this pull request to 'Upgrade to smithay-client-toolkit 0.3.0' as 0.3.0 contains few breaking changes from Smithay/client-toolkit#25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my bad, it seems none of those breaking changes apply

@trimental trimental changed the title Add key repetition for the wayland backend Upgrade to smithay-client-toolkit 0.3.0 Aug 17, 2018
@trimental trimental changed the title Upgrade to smithay-client-toolkit 0.3.0 Add key repetition for the wayland backend Aug 17, 2018
@trimental
Copy link
Contributor Author

@francesca64, are there any places where you would like these changes to be reflected in the documentation, if not can I remove 'Updated documentation...' off the checklist?

@francesca64
Copy link
Member

Oh, that would be fine. The checklist system isn't perfect, though I don't really know if there's a better way... anyway, is this PR ready for merge?

// { variables to be captured by the closure
let mut target = None;
// { variables to be captured by the closures
let target = Arc::new(Mutex::new(None));
Copy link
Member

Choose a reason for hiding this comment

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

Default::default() should be equivalent to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let target: Arc<Mutex<Option<_>>> = Default::default();

or

let target = Arc::new(Mutex::new(Default::default()));

Copy link
Member

Choose a reason for hiding this comment

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

Oh, since the type isn't inferred in this case, then it's preferable the way it already is. Sorry!

@trimental
Copy link
Contributor Author

trimental commented Aug 19, 2018

I believe it should be in a state to merge after implementing your review point. I just want to be sure though that my understanding is correct that there is no difference between key press and key repeat events in winit and that they should be indistinguishable?

@francesca64
Copy link
Member

francesca64 commented Aug 19, 2018

That's correct; our API presently has no indication of a key press being a repeat.

Anyway, thanks for the PR!

@francesca64 francesca64 merged commit e4e53fe into rust-windowing:master Aug 19, 2018
@ddevault
Copy link

Thanks! I'm sure you guys have your own release cycle but I wanted to mention that I'm looking forward to seeing this tagged for alacritty's sake.

@francesca64
Copy link
Member

This is going to be included in a new release as part of #630, which will merge as soon as the CI passes!

@ddevault
Copy link

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants