Skip to content

Less strict verification#795

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.

Sign up for GitHub

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

Merged
TikhomirovSergey merged 3 commits into from
Dec 22, 2017
Merged

Less strict verification #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da8d7f8
Remove isPositive verification for point coordinates
Dec 19, 2017
e853e24
Update tests
Dec 19, 2017
a2d8410
Add an explanation to the documentation
Dec 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/io/appium/java_client/TouchAction.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -125,6 +125,11 @@ public T release() {
* Moves current touch to a new position.
*
* @param moveToOptions see {@link PointOption} and {@link ElementOption}
* Important: some older Appium drivers releases have a bug when moveTo
* coordinates are calculated as relative to the recent pointer position
* in the chain instead of being absolute.
* @see <a href="https://github.com/appium/appium/issues/7486">Appium Issue #7486</a>
* for more details.
* @return this TouchAction, for chaining.
*/
public T moveTo(PointOption moveToOptions) {
Expand Down
8 changes: 2 additions & 6 deletions src/main/java/io/appium/java_client/touch/offset/PointOption.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
package io.appium.java_client.touch.offset;

import static com.google.common.base.Preconditions.checkArgument;
import static java.lang.String.format;
import static java.util.Optional.ofNullable;

import io.appium.java_client.touch.ActionOptions;
Expand All@@ -13,8 +11,6 @@ public class PointOption<T extends PointOption<T>> extends ActionOptions<T> {

protected Point coordinates;

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

/**
* It creates a built instance of {@link PointOption} which takes x and y coordinates.
* This is offset from the upper left corner of the screen.
Expand All@@ -36,14 +32,14 @@ public static PointOption point(int xOffset, int yOffset) {
* @return self-reference
*/
public T withCoordinates(int xOffset, int yOffset) {
checkArgument(xOffset >= 0, format(ERROR_MESSAGE_TEMPLATE, "X"));

@TikhomirovSergeyTikhomirovSergeyDec 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it temporary change for backward compatibility with older servers versions?
I think it is not necessary to remove it. Just comment it, maybe with the TODO mark.
And before the publishing of the 6.0.0 we can remove //

@mykola-mokhnachmykola-mokhnachDec 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

There might be a situation when it is necessary to provide negative absolute coordinates. For example, if the device has two screens and the second screen is located on the left side of the main one. That is why I'd consider this restriction as not very useful.

@mykola-mokhnachmykola-mokhnachDec 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Also, it is possible that one wants to drag an element to an off-screen area and negative coordinates might also have sense for such case.

@valfirstvalfirstDec 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree with @mykola-mokhnach. I tried using java-client 6.0.0-BETA1, but I was needed to rollback due to the new limitation: we have cases when actions start at the screen, but end out off screen.

checkArgument(yOffset >= 0, format(ERROR_MESSAGE_TEMPLATE, "Y"));
coordinates = new Point(xOffset, yOffset);
//noinspection unchecked
return (T) this;
}

@Override
protected void verify() {
//noinspection ResultOfMethodCallIgnored
ofNullable(coordinates).orElseThrow(() -> new IllegalArgumentException(
"Coordinate values must be defined"));
}
Expand Down
2 changes: 0 additions & 2 deletions src/test/java/io/appium/java_client/touch/TouchOptionsTests.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -44,8 +44,6 @@ public void invalidOptionsArgumentsShouldFailOnAltering() throws Exception {
final List<Runnable> invalidOptions = new ArrayList<>();
invalidOptions.add(() -> waitOptions(ofMillis(-1)));
invalidOptions.add(() -> new ElementOption().withCoordinates(0, 0).withElement(null));
invalidOptions.add(() -> new PointOption().withCoordinates(0, -1));

@TikhomirovSergeyTikhomirovSergeyDec 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just comment I think

invalidOptions.add(() -> new PointOption().withCoordinates(-1, 0));
invalidOptions.add(() -> new WaitOptions().withDuration(null));
invalidOptions.add(() -> tapOptions().withTapsCount(-1));
invalidOptions.add(() -> longPressOptions().withDuration(null));
Expand Down