Mykola mokhnach's actions params: The addition to the #756 #760

TikhomirovSergey · 2017-11-03T23:01:16Z

Change list

The addition to the #756

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)

…ava-client into mykola-mokhnach-actions_params

- the draft of architectural improvements

- reversion of accidentally changed strings/javadocs. - javadocs were added to new-designed static methods - ability to take x&y offsets by RelativeOffsetOption

- old integration tests were updated

TikhomirovSergey · 2017-11-03T23:02:35Z

@mykola-mokhnach @SrinivasanTarget
I finished. Could you take a look at this one?
@SrinivasanTarget could you run iOS tests?

mykola-mokhnach · 2017-11-04T07:10:18Z

src/main/java/io/appium/java_client/TouchAction.java

     * @return this TouchAction, for chaining.
     */
-    public TouchAction moveTo(WebElement el) {
-        ActionParameter action = new ActionParameter("moveTo", (HasIdentity) el);
+    public T moveTo(AbsoluteOffsetOption moveToOptions) {


moveTo does not support absolute offset.

mykola-mokhnach · 2017-11-04T07:10:49Z

src/main/java/io/appium/java_client/TouchAction.java

-        return this;
+    @Deprecated
+    public T moveTo(int x, int y) {
+        return moveTo(useAbsolute(x, y));


@param x change in x coordinate to move through.

@param y change in y coordinate to move through.

mykola-mokhnach · 2017-11-04T07:13:53Z

src/main/java/io/appium/java_client/touch/AbsoluteOffsetOption.java

+    @Override
+    protected void verify() {
+        ofNullable(absoluteOffset).orElseThrow(() -> new IllegalArgumentException(
+                "Absolute offset must not be defined"));


why must not?

mykola-mokhnach · 2017-11-04T07:15:21Z

src/main/java/io/appium/java_client/touch/OptionsCombinedWithOffset.java

+
+    protected void verify() {
+        ofNullable(offsetOption).orElseThrow(() ->
+                new IllegalArgumentException("Some relative or absolute offset should "


->Either relative or absolute

mykola-mokhnach · 2017-11-04T07:17:38Z

src/main/java/io/appium/java_client/touch/OptionsCombinedWithOffset.java

+    }
+
+    protected void verify() {
+        ofNullable(offsetOption).orElseThrow(() ->


we should consider here, that useRelative(x,y) won't work as expected for press/tap/longTap. I think the best way to avoid it would be to extract element parameter from RelativeOffset into withElement like we discussed before

the main reason of this is that we anyway translate both relative and absolute offset into x,y JSON keys, so there is no way for the server do distinguish them (unless we start using W3C JSON format to represent the stuff, which is already awaiting in the queue).

mykola-mokhnach · 2017-11-04T07:19:48Z

src/main/java/io/appium/java_client/touch/RelativeOffsetOption.java

+     * @return a built option
+     */
+    public static RelativeOffsetOption useRelative(WebElement element) {
+        return useRelative(element, 0, 0);


0,0 should not be the case, but separate withElement should address this problem.

mykola-mokhnach · 2017-11-04T07:22:56Z

src/main/java/io/appium/java_client/touch/RelativeOffsetOption.java

+        checkNotNull(element);
+        this.elementId = ((HasIdentity) element).getId();
+        this.relativeOffset = new Point(xOffset, yOffset);
+        //noinspection unchecked


I assume this comment is not needed here

mykola-mokhnach · 2017-11-04T07:23:38Z

src/main/java/io/appium/java_client/touch/RelativeOffsetOption.java

+            result.put("x", point.x);
+            result.put("y", point.y);
+        });
+


the empty line is not needed

mykola-mokhnach · 2017-11-04T07:26:33Z

src/main/java/io/appium/java_client/touch/WaitOptions.java

+
+    @Override
+    protected void verify() {
+        ofNullable(duration).orElseThrow(() ->


this verification is redundant, since we already have checkNotNull verification inside withDuration method

mykola-mokhnach · 2017-11-04T07:32:02Z

src/test/java/io/appium/java_client/touch/TouchOptionsTests.java

+    public void invalidAbsolutePositionOptionsShouldFailOnBuild() throws Exception {
+        final List<ActionOptions> invalidOptions = new ArrayList<>();
+        invalidOptions.add(new AbsoluteOffsetOption());
+        for (ActionOptions opts : invalidOptions) {


does it make sense to create an array with a single item?

mykola-mokhnach · 2017-11-04T07:32:15Z

src/test/java/io/appium/java_client/touch/TouchOptionsTests.java

+    public void invalidRelativePositionOptionsShouldFailOnBuild() throws Exception {
+        final List<ActionOptions> invalidOptions = new ArrayList<>();
+        invalidOptions.add(new RelativeOffsetOption());
+        for (ActionOptions opts : invalidOptions) {


- removal of redundant code from the WaitOptions. - code of test was optimized.

mykola-mokhnach · 2017-11-05T19:02:30Z

src/test/java/io/appium/java_client/touch/TouchOptionsTests.java

@@ -24,34 +25,28 @@
 public class TouchOptionsTests {
    private static final WebElement DUMMY_ELEMENT = new DummyElement();

-    @Test
+    @Test(expected = IllegalArgumentException.class)


I didn't know about such possibility %)

- PositionOffsetOption and WebElementOption instead of AbsoluteOffsetOption and RelativeOffsetOption.

mykola-mokhnach · 2017-11-06T06:29:42Z

src/main/java/io/appium/java_client/TouchAction.java

+     * @param longPressOptions see {@link LongPressOptions}.
+     * @return this TouchAction, for chaining.
+     */
+    public T longPress(LongPressOptions longPressOptions) {


do we still need this method?

@mykola-mokhnach
Yes because it has the additional

withDuration

mykola-mokhnach · 2017-11-06T06:35:11Z

src/main/java/io/appium/java_client/touch/PositionOffsetOption.java

+    }
+
+    /**
+     * Sets the offset from the upper left corner of the screen/element or


I'd rather have two separate methods to set relative and absolute offset, so then we can distinguish these in verify and show an error if the client wants, for example, to use absolute coordinates with moveTo call.

mykola-mokhnach · 2017-11-06T06:35:40Z

src/main/java/io/appium/java_client/touch/PositionOffsetOption.java

+     * @param yOffset is y-axis offset.
+     * @return this instance for chaining.
+     */
+    public T withOffet(int xOffset, int yOffset) {


mykola-mokhnach · 2017-11-06T06:38:11Z

src/main/java/io/appium/java_client/touch/PositionOffsetOption.java

+import java.util.Map;
+
+public class PositionOffsetOption<T extends PositionOffsetOption<T>> extends ActionOptions<T> {
+    private Point offset = new Point(0, 0);


like I mentioned before, the default value should not be set to 0,0, but rather to null. When only element is set the the server code selects appropriate coordinates for touch and this is not always the top left corner of the element.

mykola-mokhnach · 2017-11-06T06:40:47Z

src/main/java/io/appium/java_client/touch/WebElementOption.java

+     * @return the built option
+     */
+    public static WebElementOption elementOption(WebElement element) {
+        return elementOption(element, 0, 0);


should be no zeroes

mykola-mokhnach · 2017-11-06T06:41:30Z

src/main/java/io/appium/java_client/touch/WebElementOption.java

+        checkNotNull(element);
+        checkArgument(true, "Element should be an instance of the class which "
+                + "implements org.openqa.selenium.internal.HasIdentity",
+                (HasIdentity.class.isAssignableFrom(element.getClass())));


element instanceof HasIdentity is enough

mykola-mokhnach · 2017-11-06T06:42:39Z

src/main/java/io/appium/java_client/touch/WebElementOption.java

+
+public class WebElementOption extends PositionOffsetOption<WebElementOption> {
+
+    private HasIdentity elementId;


We could save only String id value here instead of the whole element

mykola-mokhnach · 2017-11-06T06:46:09Z

src/test/java/io/appium/java_client/android/AndroidTouchTest.java

-        TouchAction dragNDrop =
-                new TouchAction(driver).longPress(dragDot1).moveTo(dragDot3).release();
+        TouchAction dragNDrop = new TouchAction(driver)
+                        .longPress(elementOption(dragDot1))


perhaps withOffset(elementOption(...))

mykola-mokhnach · 2017-11-06T06:50:12Z

src/main/java/io/appium/java_client/touch/WebElementOption.java

+     * @param y is the y-offset from the upper left corner of the given element.
+     * @return the built option
+     */
+    public static WebElementOption elementOption(WebElement element, int x, int y) {


I'd rather rename these to simply element, relative and absolute, so we can pass it like

withOffset(element(el, x, y)), withOffset(element(el)), withOffset(relative(x,y)), withOffset(absolute(x,y))

@mykola-mokhnach
I think it would be better to have two static methods

element() offset() //to not confuse user when he/she waits for an action with absolute offset but it acts with //relative or does the opposite.

If there was a flag that could make action use offset as some relative/absolute value forcefully I think that it would have sense.

I think it's easier to perform the verification on the client side and throw an exception then if we explicitly define which type of offset we have - relative or absolute.

@mykola-mokhnach

Also issues whick were pointed out by @mykola-mokhnach were fixed out

TikhomirovSergey · 2017-11-06T21:35:19Z

@mykola-mokhnach
It seems I still don't understand something.
First of all it would be good to understand the action flow and how do actions use offsets accordingly their number in the action sequence.
Also which actions may use only absolute/relative offsets.

mykola-mokhnach · 2017-11-06T21:39:24Z

The point is to make this implementation backward compatible to the existing one. And the existing one defines:

tap, longPress, press: absolute coordinates (x, y; element; element, x, y)
moveTo: relative coordinates (x, y) and partially absolute coordinates (element; element, x ,y)

the meaning of "relative coordinates" in this context is that these coordinates are calculated as the offset from the most recent item in the chain, which precedes the current one

- offsets were divided to absolute(PointOption) relative(OffsetOption) and relative to an element (ElementOption) - The PointOption which may take any position option - signature of TouchAction was changed

mykola-mokhnach · 2017-11-09T06:28:36Z

src/main/java/io/appium/java_client/touch/offset/AbstractPositionOption.java

+import org.openqa.selenium.Point;
+
+public abstract class AbstractPositionOption<T extends AbstractPositionOption<T>> extends ActionOptions<T> {
+    protected Point offset;


mykola-mokhnach · 2017-11-09T06:30:54Z

src/main/java/io/appium/java_client/touch/offset/PointOption.java

+
+public class PointOption extends AbstractPositionOption<PointOption> {
+
+    private static final String ERROR_MESSAGE_TEMPLATE = "%s coordinate value should be equal or greater than zero";


I'd rather have it of type Function<String, String>

mykola-mokhnach · 2017-11-09T06:36:53Z

src/main/java/io/appium/java_client/touch/offset/PointOption.java

+     * @return self-reference
+     */
+    @Override
+    public PointOption position(int xOffset, int yOffset) {


why not AbsoluteOffsetOption or just AbsoluteOffset?

I think it is not completely correct. When we say absolute offset it means coordinates or 'point' on the screen.

mykola-mokhnach · 2017-11-09T06:39:11Z

src/test/java/io/appium/java_client/android/AndroidTouchTest.java

                        .withDuration(ofSeconds(2)))
-                .moveTo(offset(center2.x, center2.y))
+                .moveTo(position()
+                        .withPosition(coordinates(center2.x, center2.y)))


how this expression visually helps to understand, that this position is calculated relatively to the previous element and the previous one is absolute?

@mykola-mokhnach
The signature of methods prevents the setting of the wrong offset. Like

public T tap(Position<PointOption> tapOptions)

It means that user may set only absolute offset (PointOption, coordinates(x, y)) or element-option there.

You said before

moveTo: relative coordinates (x, y) and partially absolute coordinates (element; element, x ,y)

It means that moveTo is not bounded by absolute/relative/element offsets formally. So I desided to

public <S extends AbstractPositionOption> T moveTo(Position<S> moveToOptions) {

and it is possible to

moveTo(position().withOffset(coordinates())) moveTo(position().withOffset(offset())) moveTo(position().withOffset(element()))

I thought that one of ideas behind these changes was to minimize method count and exclude unnecessary overloading/code repeating. But, yes, it should be safe and it should exclude accidental user's actions.

If not everything is ok with this approach then I think that we could only split moveTo to

moveTo(absolute) moveTo(relative)

but it is still not clear when each method should be invoked.

May be there is some problem with the method naming

I like the idea in general. What would you say if we don't split parameter signatures between move/non-moveTo, but just add a possibility to throw a runtime verification exception if withOffset(coordinates()) is provided to moveTo and/or withOffset(offset())) is provided to non-moveTo method?

@mykola-mokhnach I think that the good way is to inform users by method signature and compilation errors. I suppose users won't have fun if compiled code will throw lots of runtime verification errors.

The Position class has at least two purposes:

unify signature of methods. Most of touch actions (present and further) can use either coordinate position/offset or offset from some element. So the class has 2 methods withOffset and withElement which invocation replaces previously stored value.

bound used coordinate strategy (absolute or relative) by generic parameter.

Currently most of actions use only points/elements except moveTo which may consume offsets as well, but I think there will be new actions which will use only offsets/elements.

If something breaks the spec then it should throw an error. However I see there is a major issue in the server implementation. iOS threats moveTo arguments as relative and Android as absolute. That's why such expression

TouchAction dragNDrop = new TouchAction(driver) .longPress(longPressOptions() .withPosition(coordinates(center1.x, center1.y)) .withDuration(ofSeconds(2))) .moveTo(position() .withPosition(coordinates(center2.x, center2.y))) .release();

works perfectly for Android, but causes an unexpected behaviour for iOS

@mykola-mokhnach Yes i remember i had an issue open on this at server end reg. moveTo

Ok. I need to think a bit.

One of the possible ways could be the ovelloading of such methods by iOS/Androind/Windows (if it will be added), maybe the making TouchAction the abstract class with generic signature of such merhods, overloaded with bounds by sublasses .

appium/appium#7486

AbstractPositionOption, OffsetOption, Position were removed. The set of class was simplified. Signature of TouchAction methods was changed and simplified.

TikhomirovSergey · 2017-11-10T21:37:58Z

@mykola-mokhnach @SrinivasanTarget
There is one more change. It was done according our decision to use only absolute coordinates or an offset from an element. Please review the last change.

...Lets wait the new beta of appium with fixed appium/appium#7486 before the publishing of the 6.0.0-BETA.

mykola-mokhnach · 2017-11-10T22:36:03Z

src/main/java/io/appium/java_client/touch/offset/PointOption.java

-public class PointOption extends AbstractPositionOption<PointOption> {
+public class PointOption<T extends PointOption<T>> extends ActionOptions<T> {
+
+    protected Point coordinates;


mykola-mokhnach · 2017-11-10T22:38:48Z

src/main/java/io/appium/java_client/touch/offset/ElementOption.java

+     * @return self-reference
+     */
+    @Override
+    public ElementOption coordinates(int xOffset, int yOffset) {


should it be withCoordinates to follow the common style?

mykola-mokhnach

minor comments. I like the way it looks now.

SrinivasanTarget · 2017-11-11T14:27:11Z

src/main/java/io/appium/java_client/touch/offset/PointOption.java

+    public T coordinates(int xOffset, int yOffset) {
+        checkArgument(xOffset >= 0, format(ERROR_MESSAGE_TEMPLATE, "X"));
+        checkArgument(yOffset >= 0, format(ERROR_MESSAGE_TEMPLATE, "Y"));
+        coordinates = new Point(xOffset, yOffset);


withCoordinates as mykola suggested above.

SrinivasanTarget · 2017-11-11T14:45:51Z

src/test/java/io/appium/java_client/touch/FailsWithMatcher.java

+
+    public static <E extends Throwable> Matcher<Runnable> failsWith(
+            final Class<E> throwableType, final Matcher<? super E> throwableMatcher) {
+        return new FailsWithMatcher<>(allOf(instanceOf(throwableType), throwableMatcher));


Also compilation error here

@SrinivasanTarget
I can't reproduce it even after removal of the project from the disc and cloning it again.
Could you try again? If you are facing the same issue could you provide some text of the error.

@SrinivasanTarget even Travis could compile the project

Yes travis wasn't able to catch it. Only IDE throws it on file level but still project builds fine. no instance of type variable exists so that E conforms to capture of ? super.... is error.

looks fine in latest IDE

Do you use Eclipse?

Intellij IDEA 2017.1.5 throws this error but no errors in Intellij IDEA 2017.2

- the `coordinates` method was renamed to `withCoordinates` - removal of unused imports from ElementOption

TikhomirovSergey · 2017-11-11T19:44:18Z

I am waiting for @SrinivasanTarget 's approve.

Mykola Mokhnach and others added 8 commits October 16, 2017 10:43

Refactor setting of touch actions options

55039ab

Add unit tests

5fdf0e1

Fix checkstyle issues

95e4042

Merge branch 'actions_params' of https://github.com/mykola-mokhnach/j…

c817ac4

…ava-client into mykola-mokhnach-actions_params

The addition to the #756:

f525ae3

- the draft of architectural improvements

The addition to the #756:

42f4a96

- reversion of accidentally changed strings/javadocs. - javadocs were added to new-designed static methods - ability to take x&y offsets by RelativeOffsetOption

The addition to the #756:

cbd9f5d

- old integration tests were updated

The addition to the #756. The final commit.

0b185e0

TikhomirovSergey requested review from mykola-mokhnach and SrinivasanTarget November 3, 2017 23:01

mykola-mokhnach reviewed Nov 4, 2017

View reviewed changes

mykola-mokhnach mentioned this pull request Nov 4, 2017

Refactor setting of touch actions options #756

Closed

4 tasks

The addition to the #756. Code improvements:

c8185da

- removal of redundant code from the WaitOptions. - code of test was optimized.

mykola-mokhnach reviewed Nov 5, 2017

View reviewed changes

The addition to the #756. archetetural improvement:

c892098

- PositionOffsetOption and WebElementOption instead of AbsoluteOffsetOption and RelativeOffsetOption.

appium deleted a comment Nov 5, 2017

mykola-mokhnach reviewed Nov 6, 2017

View reviewed changes

TikhomirovSergey added 2 commits November 6, 2017 23:12

The addition to the #756. Method renaming

483ba5a

Also issues whick were pointed out by @mykola-mokhnach were fixed out

The addition to the #756. Bug fix

ee55d22

appium deleted a comment Nov 6, 2017

The addition to the #756. Archetectural improvement:

6ddbd2e

- offsets were divided to absolute(PointOption) relative(OffsetOption) and relative to an element (ElementOption) - The PointOption which may take any position option - signature of TouchAction was changed

appium deleted a comment Nov 8, 2017

mykola-mokhnach reviewed Nov 9, 2017

View reviewed changes

The addition to the #756. Archetectural improvement.

92ab4c6

AbstractPositionOption, OffsetOption, Position were removed. The set of class was simplified. Signature of TouchAction methods was changed and simplified.

appium deleted a comment Nov 10, 2017

mykola-mokhnach reviewed Nov 10, 2017

View reviewed changes

mykola-mokhnach approved these changes Nov 10, 2017

View reviewed changes

SrinivasanTarget reviewed Nov 11, 2017

View reviewed changes

The addition to the #756. The fixing out of minor issues:

52492ec

- the `coordinates` method was renamed to `withCoordinates` - removal of unused imports from ElementOption

appium deleted a comment Nov 11, 2017

SrinivasanTarget approved these changes Nov 12, 2017

View reviewed changes

TikhomirovSergey merged commit a433896 into appium:master Nov 12, 2017


		public class WebElementOption extends PositionOffsetOption<WebElementOption> {

		private HasIdentity elementId;


		public class PointOption extends AbstractPositionOption<PointOption> {

		private static final String ERROR_MESSAGE_TEMPLATE = "%s coordinate value should be equal or greater than zero";

Mykola mokhnach's actions params: The addition to the #756 #760

Mykola mokhnach's actions params: The addition to the #756 #760

Conversation

TikhomirovSergey commented Nov 3, 2017

Change list

Types of changes

TikhomirovSergey commented Nov 3, 2017

Choose a reason for hiding this comment

mykola-mokhnach Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Nov 6, 2017 • edited Loading

mykola-mokhnach commented Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

TikhomirovSergey Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

mykola-mokhnach Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Nov 11, 2017

mykola-mokhnach Nov 4, 2017 •

edited

Loading

mykola-mokhnach Nov 6, 2017 •

edited

Loading

TikhomirovSergey commented Nov 6, 2017 •

edited

Loading

mykola-mokhnach commented Nov 6, 2017 •

edited

Loading

mykola-mokhnach Nov 9, 2017 •

edited

Loading

TikhomirovSergey Nov 9, 2017 •

edited

Loading

mykola-mokhnach Nov 9, 2017 •

edited

Loading

TikhomirovSergey Nov 9, 2017 •

edited

Loading

mykola-mokhnach Nov 9, 2017 •

edited

Loading

TikhomirovSergey Nov 9, 2017 •

edited

Loading

TikhomirovSergey commented Nov 10, 2017 •

edited

Loading