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 precise swipe actions for swipes from a start to an end point #675

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

steviki
Copy link

@steviki steviki commented Jan 23, 2018

This PR implements and fixes #674.

Introduces new swipe actions, that take a starPoint and an endPoint and perform a swipe along those points via two new public functions:

grey_swipeFastFromStartToEndPoint(CGPoint startPoint, CGPoint endPoint);
grey_swipeSlowFromStartToEndPoint(CGPoint startPoint, CGPoint endPoint);


This is done via the new GREYPreciseSwipeAction class, that is implemented similar to GREYSwipeAction, but instead of taking a direction, duration and startPercents, the precise swipe action takes a startPoint, endPoint and duration.

When performing this action, a swipe with a single touch is executed that follows the path from the startPoint to the endPoint in the time of the given duration.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@steviki
Copy link
Author

steviki commented Jan 23, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

* Returns an action that swipes through the view from a given @c startPoint to a given
* @c endPoint.
*
* @param startPoint The point where the swipe should begin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the startpoint based on a point in the whole screen or within the matched view?

Copy link
Author

Choose a reason for hiding this comment

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

It is based on the whole screen currently.
But it might make more sense to base this on the origin of the matched view.
I will change that and clarify in the documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@steviki any updates on this?

Copy link
Author

Choose a reason for hiding this comment

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

@tirodkar Yes, the change to use points based on the origin of the matched view landed in 0380dbb.
So this PR is good to go from my side.

Please let me know if there are any questions left.

Copy link
Collaborator

Choose a reason for hiding this comment

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

QQ - isn't this the same as scroll amount? Ir are you trying to do from any point to another?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't seem to be the same as scrolling in a direction with a given amount, as the scroll amount action only allows scrolling in a GREYDirection (which is restricted to left, right, up, and down) and not any arbitrary direction. While scrolling from a specific point to another point allows to scroll in any direction without restriction.

@steviki
Copy link
Author

steviki commented Apr 27, 2018

Just wanted to check in on the status here.
I'd love to get this PR merged, so let me know if there is anything left for me to take care of.
Thank you!

@khandpur
Copy link
Collaborator

khandpur commented May 1, 2018

It looks like you want precision here. Is there a reason for not using scroll API which is much more precise than swipe?

@steviki
Copy link
Author

steviki commented May 4, 2018

Is there a reason for not using scroll API which is much more precise than swipe?

@khandpur I'm not quite sure if I understand your question correctly, so please correct me if I'm wrong.
If you are referring to GREYScrollAction, then the reason is that it doesn't allow scrolling from one point to another in any arbitrary directions, but only in predefined GREYDirections.

Copy link
Collaborator

@khandpur khandpur left a comment

Choose a reason for hiding this comment

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

Could you add some tests for the new action?

EarlGrey/Action/GREYPreciseSwipeAction.m Outdated Show resolved Hide resolved
@steviki
Copy link
Author

steviki commented Aug 16, 2018

@khandpur Sorry for the long delay, but I've now added unit and functional tests for the precise swipe actions.
Looking forward to your feedback!

@steviki
Copy link
Author

steviki commented Nov 14, 2018

@khandpur Just wanted to ping to check on the status of this PR. I would really appreciated if you could have another look at this PR and consider merging it. Let me know if there are any questions!

Copy link
Collaborator

@khandpur khandpur left a comment

Choose a reason for hiding this comment

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

Mostly minor nits. Would be great if you could wrap header files with Nullability macros (NS_ASSUME_NONNULL_BEGIN / END) as well. Once done, I'll merge the changes. Thanks!

EarlGrey/Action/GREYActions.m Outdated Show resolved Hide resolved
EarlGrey/Action/GREYActions.m Outdated Show resolved Hide resolved
EarlGrey/Action/GREYPreciseSwipeAction.m Outdated Show resolved Hide resolved
EarlGrey/Action/GREYPreciseSwipeAction.m Outdated Show resolved Hide resolved
Tests/UnitTests/Sources/GREYActionsTest.m Show resolved Hide resolved
Tests/UnitTests/Sources/GREYActionsTest.m Outdated Show resolved Hide resolved
@steviki steviki force-pushed the precise-swipe-action branch from bdc102f to edd869e Compare November 15, 2018 10:19
@steviki
Copy link
Author

steviki commented Nov 15, 2018

Thanks for the review @khandpur.
I addressed all your feedback, and fixed the code style nits, adjusted the wrong indentations, added nullability macros to GREYPreciseSwipeAction.h, and also rebased this branch to the latest master.

@khandpur khandpur added the lgtm label Nov 16, 2018
@khandpur
Copy link
Collaborator

@tirodkar PTAL and merge if it looks good.

@khandpur
Copy link
Collaborator

And thank you Stefan for adding this feature request!

EarlGrey/Action/GREYActions.h Show resolved Hide resolved
EarlGrey/Action/GREYActions.h Outdated Show resolved Hide resolved
@steviki steviki force-pushed the precise-swipe-action branch from 50b518c to 51ae4fc Compare November 16, 2018 07:00
@steviki
Copy link
Author

steviki commented Nov 16, 2018

Thank you for reviewing this PR and getting it merged! I really appreciate it.

@tirodkar I also addressed your review feedback now and fixed the typo.

@khandpur
Copy link
Collaborator

@tirodkar I've restarted the CI. Could we merge this and make it available in earlgrey 2?

@steipete
Copy link

Hi! Are there any blockers left for merging this?

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

Successfully merging this pull request may close these issues.

Support swipe actions from a given start point to an end point
5 participants