Skip to content

fix: Backwards compatible behaviour of swipe and scroll in action_helpers#744

Merged
KazuCocoa merged 5 commits intoappium:masterfrom
jatalahd:master
Aug 4, 2022
Merged

fix: Backwards compatible behaviour of swipe and scroll in action_helpers#744
KazuCocoa merged 5 commits intoappium:masterfrom
jatalahd:master

Conversation

@jatalahd
Copy link
Contributor

@jatalahd jatalahd commented Jul 1, 2022

  • Fixed handling the duration argument in swipe() and scroll() helpers
  • Functionality is now the same as in older versions using TouchActions

Fixes #743

jatalahd added 3 commits July 1, 2022 15:51
- Fixed handling the duration argument in swipe() and scroll() helpers
- Functionality is now the same as in older versions using TouchActions

Fixes appium#743
- Fixed handling the duration argument in swipe() and scroll() helpers
- Functionality is now the same as in older versions using TouchActions

Fixes appium#743
- Fixed handling the duration argument in swipe() and scroll() helpers
- Functionality is now the same as in older versions using TouchActions

Fixes appium#743
@KazuCocoa KazuCocoa changed the title Backwards compatible behaviour of swipe and scroll in action_helpers fix: Backwards compatible behaviour of swipe and scroll in action_helpers Jul 1, 2022
@KazuCocoa
Copy link
Member

thank you. I will take a look.
Could you fix the lint error in the CI?

@mykola-mokhnach
Copy link
Contributor

we probably need to use timedelta type for duration, so then we never have to mix seconds and milliseconds

- Fixed handling the duration argument in swipe() and scroll() helpers
- Functionality is now the same as in older versions using TouchActions

Fixes appium#743
@jatalahd
Copy link
Contributor Author

jatalahd commented Jul 1, 2022

I did not find the actual reason for the lint error in the CI logs, but I made a new commit that hopefully goes through. If this also fails the lint, then you need to tell me what to fix.

Also, I made an important improvement to earlier version, where I did not realize that the duration given in the ActionBuilder affects all move-operations. So the first move to the point of pointer_down was also affected with the duration, which was of course wrong. It was not very intuitive to create a second ActionBuilder instance and assign the duration to only that one. But I tested it and the logs were indicating that it is the correct approach.

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.

Left a comment probably lint error, otherwise lgtm.
Actions are stored into actions in touch_input, so giving a new instance with the same touch_input helps keep actions.

- Fixed handling the duration argument in swipe() and scroll() helpers
- Functionality is now the same as in older versions using TouchActions

Fixes appium#743
@jatalahd
Copy link
Contributor Author

jatalahd commented Jul 3, 2022

Fixed the lint error in the latest commit. This is now ready from my point of view.

actions.w3c_actions.pointer_action.move_to(origin_el)
actions.w3c_actions.pointer_action.pointer_down()
# setup duration for second move only, assuming duration always has atleast default value
actions.w3c_actions = ActionBuilder(self, mouse=touch_input, duration=duration)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the logic here - do we just reassign actions.w3c_actions property? what is then the point of having previous calls?

Copy link
Member

Choose a reason for hiding this comment

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

Previous actions are in touch_input, so this one just updates duration.
actions.w3c_actions.pointer_action._duration = duration can be the alternative of the ActionBuilder(self, mouse=touch_input, duration=duration) to append actions in touch_input with the duration.

>>> touch_input
<selenium.webdriver.common.actions.pointer_input.PointerInput object at 0x108c750f0>
>>> touch_input.actions
[{'type': 'pointerDown', 'duration': 0, 'button': 0}, {'type': 'pointerDown', 'duration': 0, 'button': 0}, {'type': 'pointerUp', 'duration': 0, 'button': 0}]

Copy link
Member

Choose a reason for hiding this comment

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

i think you can also call the create_xxx methods on touch_input itself, like touch_input.create_pointer_move(), right?

actions.w3c_actions.pointer_action.move_to(origin_el)
actions.w3c_actions.pointer_action.pointer_down()
# setup duration for second move only, assuming duration always has atleast default value
actions.w3c_actions = ActionBuilder(self, mouse=touch_input, duration=duration)
Copy link
Member

Choose a reason for hiding this comment

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

Previous actions are in touch_input, so this one just updates duration.
actions.w3c_actions.pointer_action._duration = duration can be the alternative of the ActionBuilder(self, mouse=touch_input, duration=duration) to append actions in touch_input with the duration.

>>> touch_input
<selenium.webdriver.common.actions.pointer_input.PointerInput object at 0x108c750f0>
>>> touch_input.actions
[{'type': 'pointerDown', 'duration': 0, 'button': 0}, {'type': 'pointerDown', 'duration': 0, 'button': 0}, {'type': 'pointerUp', 'duration': 0, 'button': 0}]

@jlipps
Copy link
Member

jlipps commented Jul 5, 2022

for example this is how i draw a line using the python client:

def draw_line(driver, sx, sy, ex, ey, duration=250):
    line = ActionBuilder(driver)
    p = line.add_pointer_input(POINTER_TOUCH, "finger1")
    p.create_pointer_move(duration=0, x=sx, y=sy)
    p.create_pointer_down(button=MouseButton.LEFT)
    p.create_pointer_move(x=ex, y=ey, duration=duration)
    p.create_pointer_up(button=MouseButton.LEFT)
    line.perform()

seems a bit less confusing.

@KazuCocoa KazuCocoa merged commit 677c1a2 into appium:master Aug 4, 2022
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.

Suspicious functionality change in driver.swipe() and driver.scroll() duration argument

4 participants