#4515 HIGH Update.: Palette interaction improvements

[Zarro Boogs per Child]

#4515: Palette interaction improvements
---------------------+------------------------------------------------------
  Reporter:  marco   |       Owner:  rwh              
      Type:  defect  |      Status:  new              
  Priority:  high    |   Milestone:  Update.1         
 Component:  sugar   |     Version:                   
Resolution:          |    Keywords:  update.1? review?
  Verified:  0       |  
---------------------+------------------------------------------------------

Comment(by marco):

 I agree we should poll cursor position, it makes the logic simpler and we
 don't gain anything with the other approach.

 {{{
 +        'mouse-slow': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,
 ([])),
 +        'mouse-fast': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,
 ([])),
 }}}

 I'd use motion-slow/motion-fast.

 {{{
 +    _DISABLED = 1
 +    _ENABLED = 2
 }}}

 Looks like we can do without these two states and set self._state to None
 in __init__ and in stop().

 {{{
 +    _SIGNALLED_SLOW = 3
 +    _SIGNALLED_FAST = 4
 }}}

 I'd rename these to _MOTION_SLOW and _MOTION_FAST

 {{{
 +        self._thresh2 = thresh**2
 }}}

 Rename to _threshold and keep the math all inside _detect_motion. It will
 make the code more clear and will not affect perf in any way.

 {{{
 +        self._need_cal = True
 }}}

 Let's initialize self._mouse_pos to None in the constructor instead and
 check for None in _detect_motion.

 {{{
 +        self._timeout_sid = gobject.timeout_add(self._delay,
 self._timer_cb)
 }}}

 Use _timeout_hid there, code is currently mixed but we settled on hid.

 {{{
 +#        print('_timer_cb(): md: %r, state: %r') %
 (self._motion_detected, self._state)
 }}}

 Please remove this before checking it in. We might use logging.debug but
 it would probably clutter the logs too much. Maybe turn it into a
 logging.debug and comment it out. Also 80 cols.

 {{{
 +        self._motion_detected = False   # Reset
 }}}

 We are generally not using "horizontal" comments. Sorry, a nitpick really
 :)

 {{{
 +        self._detect_motion()
 }}}

 It's not immediately clear this changes self._motion_detected. And it's
 not clear that motion_detected means fast motion. Also it doesn't seem to
 be necessary to store it. Can we do something like

 {{{
 fast_motion = self._dectect_motion()
 if fast_motion...
 }}}

-- 
Ticket URL: <http://dev.laptop.org/ticket/4515#comment:13>
One Laptop Per Child <http://dev.laptop.org>
OLPC bug tracking system