GNOME Bugzilla – Bug 688457
clutter_get_current_event() returns NULL on TOUCH_BEGIN
Last modified: 2012-12-18 02:09:38 UTC
Calling clutter_get_current_event() always yields NULL when called during the dispatch of TOUCH_BEGIN. Given that _clutter_process_event() is not really reentrant as it inconditionally sets context->current_event=NULL after calling _clutter_process_event_details(), this causes the current event to get NULLified when _clutter_process_event() is called from _clutter_input_device_set_actor(..., emit_crossing=TRUE) to emit an ENTER event while dispatching TOUCH_BEGIN. Does it make sense to convert current_event to a stack (using a GSList)?
Created attachment 229141 [details] [review] events: Make _clutter_process_event() reentrant The _clutter_process_event() function may get called while already servicing a _clutter_process_event() invocation (eg. when generating ENTER events before emitting TOUCH_BEGIN). In these cases clutter_get_current_event() would return NULL after the inner call to _clutter_process_event() has finished, thus making the current event inaccessible during the outer event emission. By stacking the current events in ClutterMainContext instead of simply replacing them we do not lose track of the real current event. I'm attaching this for reference, even if it's not the approach we will end to use.
Created attachment 231707 [details] [review] events: Make _clutter_process_event() reentrant Fix segfault if no current event is set.
Review of attachment 231707 [details] [review]: I'm not overly fond of re-entrant functions: they tend to mess up stuff. sadly, get_current_event() is already a leaking abstraction. ::: clutter/clutter-event.c @@ +1400,3 @@ g_return_val_if_fail (context != NULL, NULL); + return context->current_event ? context->current_event->data : NULL; coding style: needs NULL comparison. ::: clutter/clutter-main.c @@ +2797,3 @@ * an event chain */ context->last_event_time = clutter_event_get_time (event); this will obviously break if you try to query it from a re-entrant perspective; clutter_get_current_event_time() would have to be reimplemented on top of the current event stack. @@ +2798,3 @@ */ context->last_event_time = clutter_event_get_time (event); + context->current_event = g_slist_prepend(context->current_event, event); coding style: needs space between the function and the arguments list. @@ +2802,3 @@ _clutter_process_event_details (stage, context, event); + context->current_event = g_slist_delete_link(context->current_event, context->current_event); coding style: missing space.
Created attachment 231717 [details] [review] events: Make _clutter_process_event() reentrant Updated clutter_get_current_event_time() and my sloppy coding style.
Review of attachment 231717 [details] [review]: sorry to be anal retentive on this stuff... ::: clutter/clutter-private.h @@ +165,3 @@ + /* stack of #ClutterEvent */ + GSList *current_event; instead of using a GSList can you not use a GList, and just walk backwards to pop the stack, instead of using g_slist_delete_link(), which is O(n)? granted: the list won't ever be long, but I'd rather not have a function with linear side effects to save the size of a pointer.
(In reply to comment #5) > sorry to be anal retentive on this stuff... No need to worry. Having strict reviews makes me more confortable when pushing changes. :D > ::: clutter/clutter-private.h > @@ +165,3 @@ > > + /* stack of #ClutterEvent */ > + GSList *current_event; > > instead of using a GSList can you not use a GList, and just walk backwards to > pop the stack, instead of using g_slist_delete_link(), which is O(n)? granted: > the list won't ever be long, but I'd rather not have a function with linear > side effects to save the size of a pointer. The related code is: // push context->current_event = g_slist_prepend (context->current_event, event); // peek context->current_event->data // pop context->current_event = g_slist_delete_link (context->current_event, context->current_event); Shouldn't all of them be O(1) given that we always operate only on the list head?
(In reply to comment #6) > The related code is: > > // push > context->current_event = g_slist_prepend (context->current_event, event); > // peek > context->current_event->data > // pop > context->current_event = g_slist_delete_link (context->current_event, > context->current_event); > > Shouldn't all of them be O(1) given that we always operate only on the list > head? true, though I still cringe a bit whenever I see a GSList used as a stack. oh, well.
pushed to master