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

Touch support #57

Merged
merged 8 commits into from
Dec 17, 2020
Merged

Touch support #57

merged 8 commits into from
Dec 17, 2020

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Nov 15, 2019

I've got mixed feelings about some of this - like that each touch enum has a position in it. I could have done it more similar to how the mouse events work.

I've tested the todo and tour examples on my iPhone 8 with iOS 13.2.2, the things that don't work are:

  • Text input (though, the cursor ends up in the text box) - I think we need to signal to winit to pop up the keyboard.
  • Touch scrolling on a scrollable widget. Given that the logic around cursor and touch movement not having the previous location, this didn't seem very easy to do. Fortunately, the side scrollbar works.

Here's what the tour example looks like (up to just before the rust image):

output

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you 🎉

I think we should wait until rust-windowing/winit#1082 is correctly implemented for iOS before merging. Currently, the PR is rolling back winit to use the old RedrawRequested API, which causes smooth resizes to be somewhat broken on desktop. I can try to get the web backend version of winit there, but some help for iOS would be appreciated as I do not own an iOS device.

Could the positions be removed from Touch::Ended and Touch::Cancelled or can they be different from the last Touch::Moved? I am also guessing we will need to account for multiple fingers at some point, so we may need more data to identify the events in the future.

wgpu/Cargo.toml Outdated
@@ -15,5 +15,5 @@ wgpu_glyph = { version = "0.5", git = "https://github.com/hecrj/wgpu_glyph", bra
raw-window-handle = "0.3"
image = "0.22"
glam = "0.8"
font-kit = "0.4"
font-kit = { git = "https://github.com/simlay/font-kit", branch = "add-ios-support" }
Copy link
Member

Choose a reason for hiding this comment

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

This is waiting for servo/font-kit#110 to get merged.


// TODO: Handle animations!
// Maybe we can use `ControlFlow::WaitUntil` for this.
}
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to revert this once rust-windowing/winit#1082 is resolved.

@hecrj hecrj added the feature New feature or request label Nov 15, 2019
@simlay simlay force-pushed the ios-support-wip branch 2 times, most recently from 1b484c3 to 4fba2f2 Compare November 19, 2019 18:18
@hecrj hecrj added this to the 0.1.0 milestone Nov 29, 2019
@cryptoquick
Copy link

It looks like a lot of progress has been made since last year, how's this looking?

@hecrj
Copy link
Member

hecrj commented Feb 8, 2020

Now that #181 landed, I think we can get this ready for merging.

@simlay We should rebase and remove all the merge commits. Let me know if you need any help. I could also take it to the finish line from here.

@simlay simlay force-pushed the ios-support-wip branch 2 times, most recently from 13ff494 to 971dbde Compare February 24, 2020 03:13
@simlay
Copy link
Contributor Author

simlay commented Feb 24, 2020

@hecrj Well, I squashed the commits (it was a pretty brutal rebase). but it seems that servo/font-kit#115 font-kit has moved to using pathfinder_geometry to draw stuff which breaks the build for aarch64-apple-ios (all modern iPhones).

Looks like there's a bunch of issues similar to:
no method named `min` found for type `pathfinder_simd::scalar::I32x2` in the current scope

I might submit a PR to the pathfinding repo if it's not too hard. I updated the font-kit dependency to 0.5 from crates.io. There looks to be a simd issue with some of the stuff on the github repo. This will be better anyway.

I haven't been keeping up with all the widgets this project has added since I opened the PR so I'm not sure which widgets don't have touch support.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of details.

I do not think there have been any new interactive widgets. However, there is #36 which will most likely need to be adapted to also work with touch events.

native/src/widget/text_input.rs Outdated Show resolved Hide resolved
winit/Cargo.toml Outdated Show resolved Hide resolved
@simlay simlay force-pushed the ios-support-wip branch 2 times, most recently from bcdc9e1 to eb0bbfb Compare March 18, 2020 18:24
@simlay
Copy link
Contributor Author

simlay commented Mar 18, 2020

@hecrj This good to merge?

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

About merging... I've given it some more thought, and I don't believe I can commit to maintaining this myself on master for the time being. Targetting the desktop and the web is already tricky enough and bringing touch events into the equation seems to complicate event processing a bit.

I could create an (even more) experimental mobile branch and we could merge this there (and maybe mention it in the README). This should allow us to explore completely different strategies for mobile (like IMEs) and I'd gladly accept PRs of anyone who wants to keep it up-to-date with master. Would you be okay with this?

Of course, there is also the possibility of you maintaining your own fork, but that may be less welcoming for new contributors!

In any case, I made some changes and turned the Touch type into a struct to unify the position and add finger data. I think it's an improvement. Let me know what you think!

@valpackett
Copy link
Contributor

Touch is not exclusively mobile, it's definitely a thing on desktop too! (oh and IMEs are too!)

For my project (desktop dock), I can get away with pointer emulation — actually I've implemented a very particular behavior where it's clicking on release, so I can glide up from the bottom of the screen to pick an item. The usual proper touch support would be worse for this unusual application :) but customization of input handling is a separate issue, for most apps we need something like this PR.

@hecrj
Copy link
Member

hecrj commented Aug 23, 2020

@myfreeweb You are completely right.

Looking back at this now, I am not sure I was being reasonable. I do not see a lot of complexity in these changes. Maybe the "mobile" part of the title scared me a bit at the time 😅

Thanks for bringing this back to my attention! I will work on getting this merged soon.

@hecrj hecrj reopened this Aug 23, 2020
@hecrj hecrj changed the title Initial mobile touch support for iOS. Touch support Aug 23, 2020
@hecrj hecrj added this to the 0.2.0 milestone Aug 23, 2020
@hecrj hecrj modified the milestones: 0.2.0, 0.3.0 Nov 26, 2020
@matthieugayon
Copy link

Does that PR also add IOS support for iced ?
Might be totally unrelated, but is that project relevant for that PR ? And for IOS support in general ?
https://github.com/BrainiumLLC/cargo-mobile

@hecrj hecrj changed the base branch from master to 0.2 December 15, 2020 05:18
@hecrj hecrj changed the base branch from 0.2 to master December 15, 2020 05:18
@hecrj
Copy link
Member

hecrj commented Dec 15, 2020

I think I've brought this up to speed.

I have ended up turning the Touch struct back into an enum in 3bdf931, given that this is how we handle mouse events now.

Proper support for the latest widgets is missing, but I imagine we can add that in further PRs. For instance, #650 seems to be adding touch support to the PaneGrid and the PickList. It may be interesting to rebase that PR on top of this one, once we get this merged.

I also haven't tested the changes, as I don't have a touch device at hand. I'd appreciate it if someone could try them out and report back.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

I think we can let this ship sail! We can iron out any potential issues later.

Thank you everyone! 🎉

@hecrj hecrj merged commit 89e604d into iced-rs:master Dec 17, 2020
@mtsr
Copy link
Contributor

mtsr commented Dec 21, 2020

Great news!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants