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

Avoid transitioning from pinch until all fingers have been released. #6479

Merged
merged 5 commits into from May 24, 2018
Merged

Avoid transitioning from pinch until all fingers have been released. #6479

merged 5 commits into from May 24, 2018

Conversation

puckey
Copy link
Contributor

@puckey puckey commented Apr 21, 2018

This fixes issue #6478:

  • pinch end is triggered only when both fingers are released
  • down trigger only happens if we aren't pinching

@cesium-concierge
Copy link

Signed CLA is on file.

@puckey, thanks for the pull request! Maintainers, we have a signed CLA from @puckey, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Apr 23, 2018

Thanks @puckey! @emackey can you review?

@pjcozzi
Copy link
Contributor

pjcozzi commented May 6, 2018

@puckey thanks again for the pull request!

@emackey is this something you can look at? It is only a few lines of code.

@emackey
Copy link
Contributor

emackey commented May 7, 2018

I do, sorry for the delay. I need to carve out a bit of time to try this on a touchscreen.

@emackey emackey self-assigned this May 7, 2018
Copy link
Contributor

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Thanks @puckey, this is certainly better behavior for the user. You've got one failing unit test, and I'm curious which strategy you'll take in fixing it. Also can you merge master into here when you get a chance? Thanks!

pinchEndAction.calls.reset();

// Putting another finger down should not trigger
// PINCH_START:
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails, and indeed PINCH_START is re-called when the user's touch count deviates from 2 (while remaining non-zero) and returns to 2, bringing pinch out of suspension. I'm not sure this is a bad thing, as the pinch is re-starting even though it never fully stopped.

Anyway the test should be fixed, not sure if by changing the test's expectation or by suppressing the pinch re-start event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks – I will look into this next week!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we aren't firing PINCH_END with a single touch up, it indeed doesn't make sense to fire PINCH_START when moving back to 2 fingers down.

I added a check to see if we are already pinching when touch count returns to 0.. All tests should be green and I merged in master.

@emackey
Copy link
Contributor

emackey commented May 22, 2018

Thanks @puckey. I will test on a touchscreen soon. With the latest changes I'd like to make sure that if I have two fingers down, and I lift one finger and place it down somewhere new, I don't get a sudden jump as if I had moved the finger from one place to the other. That my concern with skipping the intermediate PINCH_START event, but I'm not sure if it will happen.

@puckey
Copy link
Contributor Author

puckey commented May 23, 2018

Yes, I had the same concern.. I built the demo to check this on my iPhone and it seemed fine – but another check is always a good idea!

Copy link
Contributor

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Code looks good, tests pass, behavior seems nice on a touchscreen. Great work @puckey.

@emackey emackey merged commit 7a9184c into CesiumGS:master May 24, 2018
emackey added a commit that referenced this pull request May 24, 2018
@emackey
Copy link
Contributor

emackey commented May 24, 2018

Updated CHANGES.md in a32d774.

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.

None yet

5 participants