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

Update motion events generation logic #210

Merged
merged 4 commits into from
Oct 30, 2018

Conversation

mykola-mokhnach
Copy link
Contributor

It is the same content as in #209, which has been non intentionally corrupted %)

@Test(expected = ActionsParseException.class)
public void verifyChainPreprocessingFailsIfMultiplePointerTypesArePresent() throws JSONException {
final JSONArray actionJson = new JSONArray("[ {" +
"\"type\": \"pointer\"," +
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh. Does Java have nothing like a here-doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately not. They've just added something like that into java10 spec

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

the code is a bit too much for me to comment on right now, but the behavior isn't really working in my test case. what i'm seeing is weird in 2 ways:

  1. a move of 1500ms is taking more like 10 seconds
  2. every "synthesized motionevent" says "success" except for the very first move event, which says "fail". the drag actually looks like it works, but then the server responds with an error.

@jlipps
Copy link
Member

jlipps commented Oct 30, 2018

@jlipps
Copy link
Member

jlipps commented Oct 30, 2018

FWIW i went back to the 1.9.1 official release and it is working, however the slowness issue is still present. i think the time conversion for the pointer move is problematic somehow.

@mykola-mokhnach
Copy link
Contributor Author

Thanks a lot @jlipps. I'll add the necessary fixes

@KazuCocoa
Copy link
Member

When I ran double_click, I got an error. => https://gist.github.com/KazuCocoa/c59caeb832fd709fc429d534a42f1176
command: https://github.com/appium/ruby_lib_core/pull/157/files#diff-1419dcc55a00830b8adc2b23544e6702R88
Is this issue? (I haven't tested with previous version yet, though)

A double tap action combined with pause and pointer_down by own worked.
https://github.com/appium/ruby_lib_core/pull/157/files#diff-1419dcc55a00830b8adc2b23544e6702R75

Except for it, it worked in some test cases.

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Oct 30, 2018

@KazuCocoa I'd say such behaviour is expected, since there must be a pause between pointer down and pointer up. For a single click it is about 100-125 ms. Also Android defines minimum 40 ms delay between clicks to be recognised as double click.

How do you think, would it be more logical to throw ActionParseException if such chain is parsed or leave it as is and let it fail on execution stage?

@KazuCocoa
Copy link
Member

1.9.1 didn't work for some actions...

It's a helpful hint for users to understand why it doesn't work, I think.
(We can imagine it needs some delay because of OS side restrictions/stabilities, but many users probably don't)

@mykola-mokhnach
Copy link
Contributor Author

@KazuCocoa Added the case above into the set of unit tests

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Oct 30, 2018

1.9.1 didn't work for some actions...

Of course it did not. This feature has never been tested properly. This is the idea to have this PR and fix as much problems as possible and also improve the quality of the code.

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

Thanks! I've added some built-in actions in the test suite and they also worked.

@mykola-mokhnach mykola-mokhnach merged commit f53c830 into appium:master Oct 30, 2018
@mykola-mokhnach mykola-mokhnach deleted the actions_fix branch October 30, 2018 18:54
rajdeepv pushed a commit to rajdeepv/appium-uiautomator2-server that referenced this pull request Jan 13, 2019
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

4 participants