Skip to content

Improve Touch (Part 1 - Timestamps) - #282

Merged
rikner merged 26 commits into
masterfrom
improve-touch-part1
Feb 20, 2020
Merged

Improve Touch (Part 1 - Timestamps)#282
rikner merged 26 commits into
masterfrom
improve-touch-part1

Conversation

@rikner

@rikner rikner commented Mar 4, 2019

Copy link
Copy Markdown
Contributor

Fixes #217

Type of change:

Motivation (current vs expected behavior)

SDL's touch handling makes it impossible to get accurate timestamps on events, which breaks our velocity scrolling and causes other weird behaviour.

Also manually tested on my Android phone 👍

Please check if the PR fulfills these requirements

  • Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
  • There are no dependencies on other PRs or I have linked dependencies through Zenhub
  • The commit messages are clean and understandable
  • Tests for the changes have been added (for bug fixes / features)

@rikner
rikner requested review from ephemer and janek March 4, 2019 14:40
@rikner rikner changed the title Improve Touch (Part 1) Improve Touch (Part 1 - Timestamps) Mar 4, 2019
return firstExistingEvent
} else {
// No matching event found, creating a new one
return UIEvent(touch: newTouch)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

something about this implementation broke last I looked at it - which is why I started implementing proper multitouch.

this is half implementing multitouch because it pays attention to touchId - for this to work in a minimal implementation we need to remove that and always assume touchId == 0

@rikner rikner mentioned this pull request May 2, 2019
5 tasks
@ephemer

ephemer commented Feb 18, 2020

Copy link
Copy Markdown
Member

@rikner it would be awesome if you could provide an update whether this is still relevant, still builds, and still works as expected. From looking quickly at the code it seems like we should be able to merge this 👍

@rikner

rikner commented Feb 19, 2020

Copy link
Copy Markdown
Contributor Author

@ephemer Yeah, I will take care of this very very soon.

@rikner

rikner commented Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

@ephemer merged master and tested this on my phone. velocity scrolling now reliably works and I also compared the CPU usage with the master branch, which was exactly the same 👍

@ephemer ephemer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM overall, I would like to see this on a device before merging though, and there are a couple of comments that are effectively TODOs which we need to clear up before merging. Thanks @rikner!

Comment thread Sources/UIApplication+handleSDLEvents.swift Outdated
Comment thread Sources/UIApplication+handleSDLEvents.swift
Comment thread Sources/UIApplication+handleSDLEvents.swift Outdated
Comment thread Sources/UIApplication+handleSDLEvents.swift Outdated
Comment thread Sources/UIApplication+handleSDLEvents.swift Outdated
@codecov

codecov Bot commented Feb 20, 2020

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@0ce2db4). Click here to learn what that means.
The diff coverage is 45.58%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #282   +/-   ##
=========================================
  Coverage          ?   51.16%           
=========================================
  Files             ?       87           
  Lines             ?     3223           
  Branches          ?        0           
=========================================
  Hits              ?     1649           
  Misses            ?     1574           
  Partials          ?        0
Impacted Files Coverage Δ
Sources/UIScreen.swift 9.09% <ø> (ø)
Sources/UIView+animate.swift 78.37% <ø> (ø)
Sources/UIApplicationDelegate.swift 0% <ø> (ø)
Sources/UINavigationBarAndroid.swift 57.14% <ø> (ø)
Sources/AVPlayerItem+Mac.swift 0% <ø> (ø)
Sources/UIWindow.swift 71.05% <ø> (ø)
Sources/UIAlertAction.swift 0% <ø> (ø)
Sources/SDL2-Shims.swift 0% <ø> (ø)
Sources/DisplayLink.swift 0% <0%> (ø)
Sources/UIViewAnimationGroup.swift 63.63% <0%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ce2db4...327a804. Read the comment docs.

@rikner

rikner commented Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

@janek 💯

@rikner
rikner merged commit 638ec93 into master Feb 20, 2020
@rikner
rikner deleted the improve-touch-part1 branch February 20, 2020 17:49
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.

Android: Improve touch handling

4 participants