Merge lp:~elopio/autopilot/fix1257055-slow_drag into lp:autopilot

Proposed by Leo Arias
Status: Merged
Approved by: Thomi Richards
Approved revision: 435
Merged at revision: 440
Proposed branch: lp:~elopio/autopilot/fix1257055-slow_drag
Merge into: lp:autopilot
Prerequisite: lp:~elopio/autopilot/no_uinput_side-effects
Diff against target: 418 lines (+280/-28)
5 files modified
autopilot/input/_X11.py (+19/-4)
autopilot/input/__init__.py (+63/-7)
autopilot/input/_uinput.py (+39/-14)
autopilot/tests/functional/test_input_stack.py (+2/-1)
autopilot/tests/unit/test_input.py (+157/-2)
To merge this branch: bzr merge lp:~elopio/autopilot/fix1257055-slow_drag
Reviewer Review Type Date Requested Status
Thomi Richards (community) Approve
PS Jenkins bot continuous-integration Approve
Leo Arias Pending
Review via email: mp+205702@code.launchpad.net

This proposal supersedes a proposal from 2014-01-19.

Commit message

Added Mouse, Touch and Pointer drags with rate.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal

Hi,

I'm not sure if this MP is still relevant. If it's not, please delete the proposal to merge. If it is, then:

39 - def drag(self, x1, y1, x2, y2):
40 + def drag(self, x1, y1, x2, y2, rate=10):

If you're adding (or removing, or changing) a parameter to a public method, please document it in the docstrings. What exactly is 'rate' measured in anyway?

82 - def drag(self, x1, y1, x2, y2):
83 + def drag(self, x1, y1, x2, y2, rate=10):

You introduce a new parameter, and then don't use it at all in the method.

In general this MP makes me feel rather uneasy. You're introducing a new parameter, and some code changes, and I can't see any justification for it.

If this is really needed, I think it might be worth moving the algorithm to a common location, and making both the X11 and uinput backends be able to use it. That way, you can test it without needing to mock out anything.

Anyway, please talk to me on IRC about this. We need to either clean it up and merge it, or remove it from the review queue.

review: Needs Fixing
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal

Hi,

In order to make our review queue a little more sane, I'm setting this to WIP. If/when you need a new review, please set it back to 'Needs Review', and (optionally) ping someone on the AP team.

Thanks.

Revision history for this message
Leo Arias (elopio) wrote :

What exactly is 'rate' measured in anyway?

It's the number of pixels you advance per iteration. This is really confusing for me, I spent a couple of hours deciphering the algorithm; but unfortunately, that's what you chose for Mouse API. I would prefer something like the number of seconds that the drag will take, but at this point we can't change the Mouse API and the best we can do is to make the Touch API similar.

Revision history for this message
Leo Arias (elopio) wrote :

> If you're adding (or removing, or changing) a parameter to a public method, please document it in the docstrings.

I did my best. Please check it.

> In general this MP makes me feel rather uneasy. You're introducing a new parameter, and some code changes, and I can't see any justification for it.

The justification is on the linked bug. We need a way to slow down the drags. If we do them with the current hard-coded rate, when we try to make a new item visible on a list, we end up on the bottom of the list instead.

> If this is really needed, I think it might be worth moving the algorithm to a common location, and making both the X11 and uinput backends be able to use it. That way, you can test it without needing to mock out anything.

That's not actually possible at the moment, because the X11 relies on the move method, and so, it uses the current position of the pointer. On Touch, we don't have a current position, so there's no clean way to do a slow drag. Of course, I plan to keep cleaning things up, and that's something I will try in the following weeks.

But for now, I am losing my head with the number of workarounds I have to do on every place where the drag fails. This is an improvement from the current status, that has a hard-coded value of 100 steps per drag on the touch and 10 pixels per iteration on the mouse, which is inconsistent and comes without tests.

Lets talk tomorrow on IRC.

For now, I'll update the tests to use the new testability that comes from the branch I've added as a prerequisite.

Revision history for this message
Leo Arias (elopio) wrote :

Sorry, where it says: so there's no clean way to do a slow drag
It should say: so there's no clean way to do a slow move

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
434. By Leo Arias

We can't use the real Mouse, so switch to touch backend for now.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
435. By Leo Arias

Updated the fake.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Hi,

What's confusing me is that the new parameter 'rate' isn't used in this code:

193 + current_x, current_y = x1, y1
194 + while current_x != x2 or current_y != y2:
195 + dx = abs(x2 - current_x)
196 + dy = abs(y2 - current_y)
197 +
198 + intx = float(dx) / max(dx, dy)
199 + inty = float(dy) / max(dx, dy)
200 +
201 + step_x = min(rate * intx, dx)
202 + step_y = min(rate * inty, dy)
203 +
204 + if x2 < current_x:
205 + step_x *= -1
206 + if y2 < current_y:
207 + step_y *= -1
208 +
209 + current_x += step_x
210 + current_y += step_y
211 + self._device.finger_move(current_x, current_y)
212 +
213 + sleep(time_between_events)

So I don't see the point in adding the parameter if we're not going to use it.

review: Needs Information
Revision history for this message
Leo Arias (elopio) wrote :

201 + step_x = min(rate * intx, dx)
202 + step_y = min(rate * inty, dy)

I'm using it!

Actually, you are using it, as most of this code comes from X11 move. Here we are making sure that if the distance is smaller than the rate (which happens on the last iteration), we will make a move that will get us to the destination point and not farther. I added tests for this condition.

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

huh, sorry, I must be blind.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'autopilot/input/_X11.py'
--- autopilot/input/_X11.py 2013-11-07 05:53:36 +0000
+++ autopilot/input/_X11.py 2014-02-11 12:23:36 +0000
@@ -1,7 +1,7 @@
1# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-1# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
2#2#
3# Autopilot Functional Test Tool3# Autopilot Functional Test Tool
4# Copyright (C) 2012-2013 Canonical4# Copyright (C) 2012, 2013, 2014 Canonical
5#5#
6# This program is free software: you can redistribute it and/or modify6# This program is free software: you can redistribute it and/or modify
7# it under the terms of the GNU General Public License as published by7# it under the terms of the GNU General Public License as published by
@@ -455,16 +455,31 @@
455 x, y = coord["root_x"], coord["root_y"]455 x, y = coord["root_x"], coord["root_y"]
456 return x, y456 return x, y
457457
458 def drag(self, x1, y1, x2, y2):458 def drag(self, x1, y1, x2, y2, rate=10, time_between_events=0.01):
459 """Performs a press, move and release.459 """Perform a press, move and release.
460460
461 This is to keep a common API between Mouse and Finger as long as461 This is to keep a common API between Mouse and Finger as long as
462 possible.462 possible.
463463
464 The pointer will be dragged from the starting point to the ending point
465 with multiple moves. The number of moves, and thus the time that it
466 will take to complete the drag can be altered with the `rate`
467 parameter.
468
469 :param x1: The point on the x axis where the drag will start from.
470 :param y1: The point on the y axis where the drag will starts from.
471 :param x2: The point on the x axis where the drag will end at.
472 :param y2: The point on the y axis where the drag will end at.
473 :param rate: The number of pixels the mouse will be moved per
474 iteration. Default is 10 pixels. A higher rate will make the drag
475 faster, and lower rate will make it slower.
476 :param time_between_events: The number of seconds that the drag will
477 wait between iterations.
478
464 """479 """
465 self.move(x1, y1)480 self.move(x1, y1)
466 self.press()481 self.press()
467 self.move(x2, y2)482 self.move(x2, y2, rate=rate, time_between_events=time_between_events)
468 self.release()483 self.release()
469484
470 @classmethod485 @classmethod
471486
=== modified file 'autopilot/input/__init__.py'
--- autopilot/input/__init__.py 2013-09-20 19:01:27 +0000
+++ autopilot/input/__init__.py 2014-02-11 12:23:36 +0000
@@ -369,12 +369,27 @@
369 """369 """
370 raise NotImplementedError("You cannot use this class directly.")370 raise NotImplementedError("You cannot use this class directly.")
371371
372 def drag(self, x1, y1, x2, y2):372 def drag(self, x1, y1, x2, y2, rate=10, time_between_events=0.01):
373 """Performs a press, move and release.373 """Perform a press, move and release.
374374
375 This is to keep a common API between Mouse and Finger as long as375 This is to keep a common API between Mouse and Finger as long as
376 possible.376 possible.
377377
378 The pointer will be dragged from the starting point to the ending point
379 with multiple moves. The number of moves, and thus the time that it
380 will take to complete the drag can be altered with the `rate`
381 parameter.
382
383 :param x1: The point on the x axis where the drag will start from.
384 :param y1: The point on the y axis where the drag will starts from.
385 :param x2: The point on the x axis where the drag will end at.
386 :param y2: The point on the y axis where the drag will end at.
387 :param rate: The number of pixels the mouse will be moved per
388 iteration. Default is 10 pixels. A higher rate will make the drag
389 faster, and lower rate will make it slower.
390 :param time_between_events: The number of seconds that the drag will
391 wait between iterations.
392
378 """393 """
379 raise NotImplementedError("You cannot use this class directly.")394 raise NotImplementedError("You cannot use this class directly.")
380395
@@ -466,8 +481,28 @@
466 """Release a previously pressed finger"""481 """Release a previously pressed finger"""
467 raise NotImplementedError("You cannot use this class directly.")482 raise NotImplementedError("You cannot use this class directly.")
468483
469 def drag(self, x1, y1, x2, y2):484 def drag(self, x1, y1, x2, y2, rate=10, time_between_events=0.01):
470 """Perform a drag gesture from (x1,y1) to (x2,y2)"""485 """Perform a drag gesture.
486
487 The finger will be dragged from the starting point to the ending point
488 with multiple moves. The number of moves, and thus the time that it
489 will take to complete the drag can be altered with the `rate`
490 parameter.
491
492 :param x1: The point on the x axis where the drag will start from.
493 :param y1: The point on the y axis where the drag will starts from.
494 :param x2: The point on the x axis where the drag will end at.
495 :param y2: The point on the y axis where the drag will end at.
496 :param rate: The number of pixels the finger will be moved per
497 iteration. Default is 10 pixels. A higher rate will make the drag
498 faster, and lower rate will make it slower.
499 :param time_between_events: The number of seconds that the drag will
500 wait between iterations.
501
502 :raises RuntimeError: if the finger is already pressed.
503 :raises RuntimeError: if no more finger slots are available.
504
505 """
471 raise NotImplementedError("You cannot use this class directly.")506 raise NotImplementedError("You cannot use this class directly.")
472507
473508
@@ -641,9 +676,30 @@
641 else:676 else:
642 return (self._x, self._y)677 return (self._x, self._y)
643678
644 def drag(self, x1, y1, x2, y2):679 def drag(self, x1, y1, x2, y2, rate=10, time_between_events=0.01):
645 """Performs a press, move and release."""680 """Perform a press, move and release.
646 self._device.drag(x1, y1, x2, y2)681
682 This is to keep a common API between Mouse and Finger as long as
683 possible.
684
685 The pointer will be dragged from the starting point to the ending point
686 with multiple moves. The number of moves, and thus the time that it
687 will take to complete the drag can be altered with the `rate`
688 parameter.
689
690 :param x1: The point on the x axis where the drag will start from.
691 :param y1: The point on the y axis where the drag will starts from.
692 :param x2: The point on the x axis where the drag will end at.
693 :param y2: The point on the y axis where the drag will end at.
694 :param rate: The number of pixels the mouse will be moved per
695 iteration. Default is 10 pixels. A higher rate will make the drag
696 faster, and lower rate will make it slower.
697 :param time_between_events: The number of seconds that the drag will
698 wait between iterations.
699
700 """
701 self._device.drag(
702 x1, y1, x2, y2, rate=rate, time_between_events=time_between_events)
647 if isinstance(self._device, Touch):703 if isinstance(self._device, Touch):
648 self._x = x2704 self._x = x2
649 self._y = y2705 self._y = y2
650706
=== modified file 'autopilot/input/_uinput.py'
--- autopilot/input/_uinput.py 2014-02-11 12:23:36 +0000
+++ autopilot/input/_uinput.py 2014-02-11 12:23:36 +0000
@@ -521,8 +521,23 @@
521 """521 """
522 self._device.finger_move(x, y)522 self._device.finger_move(x, y)
523523
524 def drag(self, x1, y1, x2, y2):524 def drag(self, x1, y1, x2, y2, rate=10, time_between_events=0.01):
525 """Perform a drag gesture from (x1,y1) to (x2,y2).525 """Perform a drag gesture.
526
527 The finger will be dragged from the starting point to the ending point
528 with multiple moves. The number of moves, and thus the time that it
529 will take to complete the drag can be altered with the `rate`
530 parameter.
531
532 :param x1: The point on the x axis where the drag will start from.
533 :param y1: The point on the y axis where the drag will starts from.
534 :param x2: The point on the x axis where the drag will end at.
535 :param y2: The point on the y axis where the drag will end at.
536 :param rate: The number of pixels the finger will be moved per
537 iteration. Default is 10 pixels. A higher rate will make the drag
538 faster, and lower rate will make it slower.
539 :param time_between_events: The number of seconds that the drag will
540 wait between iterations.
526541
527 :raises RuntimeError: if the finger is already pressed.542 :raises RuntimeError: if the finger is already pressed.
528 :raises RuntimeError: if no more finger slots are available.543 :raises RuntimeError: if no more finger slots are available.
@@ -531,18 +546,28 @@
531 logger.debug("Dragging from %d,%d to %d,%d", x1, y1, x2, y2)546 logger.debug("Dragging from %d,%d to %d,%d", x1, y1, x2, y2)
532 self._device.finger_down(x1, y1)547 self._device.finger_down(x1, y1)
533548
534 # Let's drag in 100 steps for now...549 current_x, current_y = x1, y1
535 dx = 1.0 * (x2 - x1) / 100550 while current_x != x2 or current_y != y2:
536 dy = 1.0 * (y2 - y1) / 100551 dx = abs(x2 - current_x)
537 cur_x = x1 + dx552 dy = abs(y2 - current_y)
538 cur_y = y1 + dy553
539 for i in range(0, 100):554 intx = float(dx) / max(dx, dy)
540 self._device.finger_move(int(cur_x), int(cur_y))555 inty = float(dy) / max(dx, dy)
541 sleep(0.002)556
542 cur_x += dx557 step_x = min(rate * intx, dx)
543 cur_y += dy558 step_y = min(rate * inty, dy)
544 # Make sure we actually end up at target559
545 self._device.finger_move(x2, y2)560 if x2 < current_x:
561 step_x *= -1
562 if y2 < current_y:
563 step_y *= -1
564
565 current_x += step_x
566 current_y += step_y
567 self._device.finger_move(current_x, current_y)
568
569 sleep(time_between_events)
570
546 self._device.finger_up()571 self._device.finger_up()
547572
548573
549574
=== modified file 'autopilot/tests/functional/test_input_stack.py'
--- autopilot/tests/functional/test_input_stack.py 2014-02-11 12:23:36 +0000
+++ autopilot/tests/functional/test_input_stack.py 2014-02-11 12:23:36 +0000
@@ -487,7 +487,8 @@
487 def __init__(self):487 def __init__(self):
488 pass488 pass
489489
490 def drag(self, x1, y1, x2, y2):490 def drag(self, x1, y1, x2, y2, rate='dummy',
491 time_between_events='dummy'):
491 pass492 pass
492493
493 p = Pointer(FakeTouch())494 p = Pointer(FakeTouch())
494495
=== modified file 'autopilot/tests/unit/test_input.py'
--- autopilot/tests/unit/test_input.py 2014-02-11 12:23:36 +0000
+++ autopilot/tests/unit/test_input.py 2014-02-11 12:23:36 +0000
@@ -19,14 +19,15 @@
1919
20import testscenarios20import testscenarios
21from evdev import ecodes, uinput21from evdev import ecodes, uinput
22from mock import ANY, call, patch, Mock22from mock import ANY, call, Mock, patch
23from six import StringIO23from six import StringIO
24from testtools import TestCase24from testtools import TestCase
25from testtools.matchers import Contains, raises25from testtools.matchers import Contains, raises
2626
27import autopilot.input
28from autopilot import utilities
27from autopilot.input import _uinput29from autopilot.input import _uinput
28from autopilot.input._common import get_center_point30from autopilot.input._common import get_center_point
29from autopilot import utilities
3031
3132
32class Empty(object):33class Empty(object):
@@ -698,6 +699,54 @@
698 touch.move(10, 10)699 touch.move(10, 10)
699 self.assertEqual(expected_calls, touch._device.mock_calls)700 self.assertEqual(expected_calls, touch._device.mock_calls)
700701
702 def test_drag_must_call_finger_down_move_and_up(self):
703 expected_calls = [
704 call.finger_down(0, 0),
705 call.finger_move(10, 10),
706 call.finger_up()
707 ]
708
709 touch = self.get_touch_with_mocked_backend()
710 touch.drag(0, 0, 10, 10)
711 self.assertEqual(expected_calls, touch._device.mock_calls)
712
713 def test_drag_must_move_with_specified_rate(self):
714 expected_calls = [
715 call.finger_down(0, 0),
716 call.finger_move(5, 5),
717 call.finger_move(10, 10),
718 call.finger_move(15, 15),
719 call.finger_up()]
720
721 touch = self.get_touch_with_mocked_backend()
722 touch.drag(0, 0, 15, 15, rate=5)
723
724 self.assertEqual(
725 expected_calls, touch._device.mock_calls)
726
727 def test_drag_without_rate_must_use_default(self):
728 expected_calls = [
729 call.finger_down(0, 0),
730 call.finger_move(10, 10),
731 call.finger_move(20, 20),
732 call.finger_up()]
733
734 touch = self.get_touch_with_mocked_backend()
735 touch.drag(0, 0, 20, 20)
736
737 self.assertEqual(
738 expected_calls, touch._device.mock_calls)
739
740 def test_drag_to_same_place_must_not_move(self):
741 expected_calls = [
742 call.finger_down(0, 0),
743 call.finger_up()
744 ]
745
746 touch = self.get_touch_with_mocked_backend()
747 touch.drag(0, 0, 0, 0)
748 self.assertEqual(expected_calls, touch._device.mock_calls)
749
701750
702class MultipleUInputTouchBackend(_uinput._UInputTouchDevice):751class MultipleUInputTouchBackend(_uinput._UInputTouchDevice):
703752
@@ -731,3 +780,109 @@
731 self.addCleanup(finger1.release)780 self.addCleanup(finger1.release)
732781
733 self.assertFalse(finger2.pressed)782 self.assertFalse(finger2.pressed)
783
784
785class DragUInputTouchTestCase(testscenarios.TestWithScenarios, TestCase):
786
787 scenarios = [
788 ('drag to top', dict(
789 start_x=50, start_y=50, stop_x=50, stop_y=30,
790 expected_moves=[call.finger_move(50, 40),
791 call.finger_move(50, 30)])),
792 ('drag to bottom', dict(
793 start_x=50, start_y=50, stop_x=50, stop_y=70,
794 expected_moves=[call.finger_move(50, 60),
795 call.finger_move(50, 70)])),
796 ('drag to left', dict(
797 start_x=50, start_y=50, stop_x=30, stop_y=50,
798 expected_moves=[call.finger_move(40, 50),
799 call.finger_move(30, 50)])),
800 ('drag to right', dict(
801 start_x=50, start_y=50, stop_x=70, stop_y=50,
802 expected_moves=[call.finger_move(60, 50),
803 call.finger_move(70, 50)])),
804
805 ('drag to top-left', dict(
806 start_x=50, start_y=50, stop_x=30, stop_y=30,
807 expected_moves=[call.finger_move(40, 40),
808 call.finger_move(30, 30)])),
809 ('drag to top-right', dict(
810 start_x=50, start_y=50, stop_x=70, stop_y=30,
811 expected_moves=[call.finger_move(60, 40),
812 call.finger_move(70, 30)])),
813 ('drag to bottom-left', dict(
814 start_x=50, start_y=50, stop_x=30, stop_y=70,
815 expected_moves=[call.finger_move(40, 60),
816 call.finger_move(30, 70)])),
817 ('drag to bottom-right', dict(
818 start_x=50, start_y=50, stop_x=70, stop_y=70,
819 expected_moves=[call.finger_move(60, 60),
820 call.finger_move(70, 70)])),
821
822 ('drag less than rate', dict(
823 start_x=50, start_y=50, stop_x=55, stop_y=55,
824 expected_moves=[call.finger_move(55, 55)])),
825
826 ('drag with last move less than rate', dict(
827 start_x=50, start_y=50, stop_x=65, stop_y=65,
828 expected_moves=[call.finger_move(60, 60),
829 call.finger_move(65, 65)])),
830 ]
831
832 def setUp(self):
833 super(DragUInputTouchTestCase, self).setUp()
834 # Mock the sleeps so we don't have to spend time actually sleeping.
835 self.addCleanup(utilities.sleep.disable_mock)
836 utilities.sleep.enable_mock()
837
838 def get_touch_with_mocked_backend(self):
839 touch = _uinput.Touch(device_class=Mock)
840 touch._device.mock_add_spec(
841 _uinput._UInputTouchDevice, spec_set=True)
842 return touch
843
844 def test_drag_moves(self):
845 touch = self.get_touch_with_mocked_backend()
846
847 touch.drag(
848 self.start_x, self.start_y, self.stop_x, self.stop_y)
849
850 # We don't check the finger down and finger up. They are already
851 # tested.
852 expected_calls = [ANY] + self.expected_moves + [ANY]
853 self.assertEqual(
854 expected_calls, touch._device.mock_calls)
855
856
857class PointerWithTouchBackendTestCase(TestCase):
858
859 def get_pointer_with_touch_backend_with_mock_device(self):
860 touch = _uinput.Touch(device_class=Mock)
861 touch._device.mock_add_spec(
862 _uinput._UInputTouchDevice, spec_set=True)
863 pointer = autopilot.input.Pointer(touch)
864 return pointer
865
866 def test_drag_with_rate(self):
867 pointer = self.get_pointer_with_touch_backend_with_mock_device()
868 with patch.object(pointer._device, 'drag') as mock_drag:
869 pointer.drag(0, 0, 20, 20, rate='test')
870
871 mock_drag.assert_called_once_with(
872 0, 0, 20, 20, rate='test', time_between_events=0.01)
873
874 def test_drag_with_time_between_events(self):
875 pointer = self.get_pointer_with_touch_backend_with_mock_device()
876 with patch.object(pointer._device, 'drag') as mock_drag:
877 pointer.drag(0, 0, 20, 20, time_between_events='test')
878
879 mock_drag.assert_called_once_with(
880 0, 0, 20, 20, rate=10, time_between_events='test')
881
882 def test_drag_with_default_parameters(self):
883 pointer = self.get_pointer_with_touch_backend_with_mock_device()
884 with patch.object(pointer._device, 'drag') as mock_drag:
885 pointer.drag(0, 0, 20, 20)
886
887 mock_drag.assert_called_once_with(
888 0, 0, 20, 20, rate=10, time_between_events=0.01)

Subscribers

People subscribed via source and target branches