[Sugar-devel] gestures + event controller patch series

[Carlos Garnacho]

Hey,

I'm attaching some new patches, they apply on top of the previous batch,
some comments below

On lun, 2012-09-10 at 13:39 +0200, Simon Schampijer wrote:
> On 09/05/2012 02:04 PM, Carlos Garnacho wrote:
> > Hey,
> >
> > I'm sending an updated patch set. There was a missing header in the
> > previous set, and now there's code to show UI feedback on long press
> >
> >    Carlos
> 
> Hi Carlos,
> 
> all in all a very nice work, I have a few comments on the API:
> 
> ==Long press==
> The property names could be a bit more descriptive. How about 
> "timeuntiltriggered" or "durationuntiltriggered" instead of "timeout" 
> and "allowableMovement" instead of "threshold" (shorter descriptive 
> ideas welcome :).

I've went for "trigger-delay", some middle ground in descriptiveness :P

> 
> For the signal name, I prefer 'ended' instead of 'finished', works 
> better with 'started'. Began/Ended is another option.

I've went for began/updated/ended in the end

> 
> Sidenote: The visual feedback for the press-hold is currently a black 
> rectangle on the XO.

I've done some small fixes there, let me know if those helped. wrt the
black rectangle, I'm suspicious on the XShape extension support not
being compiled in though, the window should have a shape even with no
compositor.

> 
> 
> ==Swipe==
> Maybe there are better options for the signal name than 
> "swipe-finished", how about "swipe-recognized' or if we consider the 
> signal name for LongPress how about just 'ended'?

As "ended" is now a signal of SugarEventController, it's been changed to
"swipe-ended" so the direction can still be a signal argument.

> 
> Maybe we can enhance the API to know the location where a swipe begun. 
> Could be an argument to the 'ended' callback maybe.

Hmm, That's more or less related to limiting gestures to a bounding box,
so you could set up different swipe controllers on different widget
regions. inversely, you know which event controller triggered a signal,
and you can map that back to your area of interest.

> 
> Should we allow to only listen for a specific swipe direction as a property?

Given the controller still needs to listen for events regardless of them
being in the right direction or not, IMHO it doesn't save much that you
set a direction since the beginning, processing-wise. Could be surely
added for easing the controller usage, although it just saves a couple
of lines in the callback :)

> 
> Not highest priority: I am not sure how hard it is to extend this, but 
> maybe we can allow to define swipes for multiple touch points (at least 
> two)? Could be another property.

That could require some code changes there, as you don't only have to
check the velocity of one touchpoint, but the overall direction/velocity
of all current touches.

> 
> 
> ==Rotate==
> We should note that the angle is in radians.

Right, that's better documented

> 
> 
> ==Zoom==
> Maybe the delta argument in the annotation for zoom-changed should be 
> called scale? And we can note in the description that it is the delta.

Sounds better indeed, I changed that too

> 
> 
> ==Tap==
> As an addition we can think about a gesture handler for taps, to know 
> when there have been multiple taps.

One could be implemented indeed, although double/triple click is already
hinted in the GdkEventType

> 
> 
> ==Namespace==
> The Namespace is quiet long (SugarEventController.LongPressController) 
> and it doubles "Controller". We could either move it under SugarExt 
> (SugarExt.LongPressController) or we could maybe use SugarGestures 
> (SugarGestures.LongPressController).

The namespace was admittedly better thought out for C :). I've renamed
the gir file to SugarGestures

Cheers,
  Carlos
-------------- next part --------------
A non-text attachment was scrubbed...
Name: event-controller-patches.tar.gz
Type: application/x-compressed-tar
Size: 11459 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120913/6f2bbede/attachment-0001.bin>