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

Refactor setting of touch actions options #756

Closed
wants to merge 3 commits into from

Conversation

mykola-mokhnach
Copy link
Contributor

Change list

The idea of this PR is to unify touch action options, so it is easier to extend them.
The main reason for this change was my intension to add pressure argument to press/longPress actions for iOS. With the current implementation the count of overriden methods in TouchAction class would have grown up exponentially.

Types of changes

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@mykola-mokhnach mykola-mokhnach changed the title Refactor setting touch actions options Refactor setting of touch actions options Oct 16, 2017
@appium appium deleted a comment Oct 16, 2017
@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Oct 16, 2017

@KazuCocoa I would be happy to see your feedback on this change. Would it be useful to set action options this way? If yes, then would it be necessary to have something like this implemented for Ruby/Python libs?

@appium appium deleted a comment Oct 16, 2017
@KazuCocoa
Copy link
Member

looks good for me. 🆗
For Ruby/Python client, it isn't probably necessary for them, but I'd like to consider a bit.

@appium appium deleted a comment Oct 17, 2017
public T moveTo(int x, int y) {
ActionParameter action = new ActionParameter("moveTo",
new MoveToOptions()
.withRelativeOffset(x, y));
Copy link
Member

Choose a reason for hiding this comment

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

@mykola-mokhnach Isn't it absoluteOffset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • @param x change in x coordinate to move through.
  • @param y change in y coordinate to move through.

That is why I find the current approach useful, because it helps to avoid such misunderstandings while settings options ;)

Copy link
Member

Choose a reason for hiding this comment

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

@mykola-mokhnach haha makes sense.thanks :)

@mykola-mokhnach
Copy link
Contributor Author

@TikhomirovSergey when do you have time to take a look at the PR?



public class IOSTouchAction extends TouchAction {
public class IOSTouchAction extends TouchAction<IOSTouchAction> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mykola-mokhnach Why this class is not fully deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class will be needed to add further TouchAction extensions

import io.appium.java_client.TouchAction;


public class AndroidTouchAction extends TouchAction<AndroidTouchAction> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What the purpose of this class?

Copy link
Contributor Author

@mykola-mokhnach mykola-mokhnach Oct 28, 2017

Choose a reason for hiding this comment

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

This class will be needed in the future to add Android-specific touch actions (as well as the IOS one).

Also, there is second very Java-specific reason. This class is a specific implementation of Builder pattern with inheritance. If you check the ancestor classes they all have quite specific generic signatures and this all is done to make builder methods return appropriate class without casting, for example:

new IOSTouchAction(driver).press(...).doubleTap(...)

won't work in the previous implementation, because press returns the instance of TouchAction class instead of IOSTouchAction, where this method is implemented, so it is necessary to perform class cast:

((IOSTouchAction)new IOSTouchAction(driver).press(...)).doubleTap(...)

This approach fixes it, but now Java prevents one from passing TouchAction as a generic parameter, so Supplier<TouchAction> won't work, but Supplier<IOSTouchAction> works as expected. https://stackoverflow.com/questions/21086417/builder-pattern-and-inheritance has more details.

ActionParameter action = new ActionParameter("doubleTap", (HasIdentity) el);
action.addParameter("x", x);
action.addParameter("y", y);
ActionParameter action = new ActionParameter("doubleTap",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also
Won't the doubleTap be supported anymore?

Copy link
Contributor Author

@mykola-mokhnach mykola-mokhnach Oct 28, 2017

Choose a reason for hiding this comment

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

It looks redundant to me. doubleTap can be easily replaced with tap(count:2) or double tap-wait-tap chains.
Would you prefer to keep it?


package io.appium.java_client.ios.touch;

import io.appium.java_client.touch.OptionsWithAbsolutePositioning;
Copy link
Contributor

@TikhomirovSergey TikhomirovSergey Oct 27, 2017

Choose a reason for hiding this comment

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

If this class was added only for backward compatibility so I think it might be better to make it nested in IOSTouchAction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this should be visible for TouchAction options as well.

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey left a comment

Choose a reason for hiding this comment

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

@mykola-mokhnach
I generally like this PR and the idea behind it. But I have some doubds:

  • why there some empty classes? May some things be generilized?
  • is it safe to declare something like public T waitAction(ActionOptions waitOptions)? What is user will try something like to.wait(optionsWithAbsolutePositioning)? Should methods have more strict signature?

@mykola-mokhnach
Copy link
Contributor Author

why there some empty classes? May some things be generilized?

See my comment above about builder generics. Also, it will be possible to add more stuff into these classes later, for example some special platform-specific gestures, which we don't support yet.

is it safe to declare something like public T waitAction(ActionOptions waitOptions)? What is user will try something like to.wait(optionsWithAbsolutePositioning)? Should methods have more strict signature?

I thought about it... From code creation perspective the current approach should not be a problem, since it's pretty obvious to see an error in the method like tap(new LongPressOptions()), which is the case for dynamically typed non-Java world. And this gives us more freedom if we want to change these options in the future without affecting the client code much. However, its good as an additional protection level from dumb users. So my point is we can always make these types more strict later if we observe complains about it.

@mykola-mokhnach
Copy link
Contributor Author

Closed because of #760

TikhomirovSergey added a commit that referenced this pull request Nov 12, 2017
…_params

Mykola mokhnach's actions params:  The addition to the #756
@mykola-mokhnach mykola-mokhnach deleted the actions_params branch December 19, 2017 19: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.

4 participants