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

fix touch handling (pragmatic approach) #349

Closed
wants to merge 2 commits into from

Conversation

michaelknoch
Copy link
Member

@michaelknoch michaelknoch commented Nov 29, 2021

This reverts commit 9a698b9.

Type of change: fix broken loop handles

Motivation (current vs expected behavior)

Reverting 9a698b9 leads to the exact same behaviour as we have on the latest commit of the fix-touch branch.

This commit introduces the behaviour that touchesEnded gets (maybe correctly?) cancelled, but touchesBegan does not. Thats why in our player we see that the all controls get hidden when dragging a loop handle, but they never come back on touchesEnded.

https://record.wtf/play?id=1638181427736_46106

reproduce:

  • opens song
  • open loop
  • drag any of the handles to adjust the loop

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)

@michaelknoch
Copy link
Member Author

I think reverting the commit and then iterating on touch handling again makes more sense instead of relying on the fix-touch branch for our app releases. @ephemer @rikner

@rikner
Copy link
Contributor

rikner commented Nov 29, 2021

Do I understandly correctly, that 9a698b9 was intended to be a refactor without any behaviour change, but it indeed changed the behaviour? Would totally make sense then to revert it and re-iterating again on the touch handling.

@michaelknoch
Copy link
Member Author

hard to say @rikner . I can't see a pr for this, seems like it was commited to master directly without description or motivation. I think we tried to align the cancels behaviour with iOS, because this is slightly different. On android the draghandles disappear while dragging, on iOS they don't. And this is probably what he intended to fix, but now only touchesBegan and moved gets called and touchesEnded gets cancelled which leads to the weird UX where the main controls remain hidden after dragging.

Copy link
Member

@ephemer ephemer left a comment

Choose a reason for hiding this comment

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

If we're currently releasing from a branch then we should merge this to avoid that. I fear we may break others' code doing this but so far no one has made us aware that they're using this in production so let's go for it for now

@michaelknoch
Copy link
Member Author

i think having a consistent behaviour between touchesBegan and touchesEnded is probably more beneficial for every project using UIKit. I'm happy to work on a fix asap as soon as someone opens an issue ;)

@michaelknoch
Copy link
Member Author

michaelknoch commented Nov 29, 2021

ok never mind, merging this pr breaks scrolling through the exercise parts menu in our courses. It's new to me that this commit behaves differently than the last commit of fix-touch #333

I am closing this now and will re-iterate on the fix-touch branch to get it in as soon as possible. sorry for the noise @rikner @ephemer

@michaelknoch michaelknoch deleted the fix-touch-pragmatic branch November 29, 2021 17:43
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.

3 participants