Created attachment 145858 [details] [review]
Current snapshot of XI2 patch, up-to-date with master

I think the current status is good enough for an initial review, I'm attaching the full patch here. Being a complete redesign of some quite delicate internals, I find the patch quite hard to split, so I'm not sure what would be the best approach for merging, I've tried to keep things fully working along the log history, but was unable to at times.

There is up-to-date info about the implementation details in:

http://live.gnome.org/GTK+/MPX

Ok, this will take a while to get through...

I'll start with some notes about the events-refactor branch. 


In general, the refactoring into GdkEventSource and GdkEventTranslator looks like a good idea to me, even though I think I will miss gdkevents-x11.c and it will take me a long time to stop looking for gdk_event_translate as the central place for event munging.


in gdkdevicemanager.h:

#if defined(GTK_DISABLE_SINGLE_INCLUDES) && !defined (__GDK_H_INSIDE__) && !defined (GDK_COMPILATION)

I believe single-includes are only supposed to be disable-able for old headers, not for new stuff.

in gdkdevicemanager.c:

                                                        G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));

Should use the G_PARAM_STATIC_BLA flags as well.


    g_signal_new (g_intern_static_string ("device-added"),

Don't we have the I() macro I used for this elsewhere defined in gdk ?


     device_manager = _gdk_device_manager_new (display);
     g_hash_table_insert (device_managers, display, device_manager);

I think this approach is going to lead to lifecycle pain wrt to closing displays. A better approach might be to tack the device manager as object data on the display, and rely on gdk_display_manager_list_displays. In practise, the list will ever only of length 1, so that should be plenty fast enough.


 * Returns: a list of #GdkDevice<!-- -->s. The returned list must be
 *          freed with g_list_free ().

Best to say explicitly what to do with the individual devices in the list - unref, or not ? and what about the lifecycle of the devices returned by this call - can I hold onto them, or do I have to ref them, or am I not allowed to ?


in gdkeventsource.c:

some minor style issues:

gdk_event_apply_filters (XEvent *xevent,
                         GdkEvent *event,
                         GList *filters)

arg alignment

      if (result !=  GDK_FILTER_CONTINUE)

extra whitespace


  while (!_gdk_event_queue_find_first(display) && XPending (xdisplay))

missing whitespace


          event_mask &= ~(mask);

redundant parens


void
gdk_event_source_add_translator (GdkEventSource     *source,
                                 GdkEventTranslator *translator)

{
  g_return_if_fail (GDK_IS_EVENT_TRANSLATOR (translator));

  source->translators = g_list_prepend (source->translators, translator);
}

I would have expected a function named _add_ to append, not prepend...

I've read through the xi2 branch as well now. Impressive work; I don't have any detailed comments as of yet; http://live.gnome.org/GTK+/MPX seems to have a good list of leftover work...

Do we need to do something about the confine_to window for pointer grabs that XI2 seems to have lost ?

googling found at least one or two users of gdk_pointer_grab with a non-NULL confine-to window...

(In reply to comment #2)
> 
> in gdkdevicemanager.h:
> 
> #if defined(GTK_DISABLE_SINGLE_INCLUDES) && !defined (__GDK_H_INSIDE__) &&
> !defined (GDK_COMPILATION)
> 
> I believe single-includes are only supposed to be disable-able for old headers,
> not for new stuff.

Right :), fixed.

> 
> in gdkdevicemanager.c:
> 
>                                                         G_PARAM_READWRITE |
> G_PARAM_CONSTRUCT_ONLY));
> 
> Should use the G_PARAM_STATIC_BLA flags as well.

Fixed too, GdkDevice also lacked these, so I've added them there as well

> 
> 
>     g_signal_new (g_intern_static_string ("device-added"),
> 
> Don't we have the I() macro I used for this elsewhere defined in gdk ?

No, we don't, it's only defined in gtk/gtkintl.h. Should I commit one in gdk/gdkintl.h and fix all usages in master? Makes quite sense to me

> 
> 
>      device_manager = _gdk_device_manager_new (display);
>      g_hash_table_insert (device_managers, display, device_manager);
> 
> I think this approach is going to lead to lifecycle pain wrt to closing
> displays. A better approach might be to tack the device manager as object data
> on the display, and rely on gdk_display_manager_list_displays. In practise, the
> list will ever only of length 1, so that should be plenty fast enough.

That code actually changed in the xi2 branch (should have gone into events-refactor indeed), there is now a gdk_display_get_device_manager () method.

> 
> 
>  * Returns: a list of #GdkDevice<!-- -->s. The returned list must be
>  *          freed with g_list_free ().
> 
> Best to say explicitly what to do with the individual devices in the list -
> unref, or not ? and what about the lifecycle of the devices returned by this
> call - can I hold onto them, or do I have to ref them, or am I not allowed to ?

Right, I added a blurb about list items memory handling, also renamed that function to gdk_device_manager_list_devices(), Mitch pointed out that this seems to be the convention for methods that return an allocated list.

> 
> 
> in gdkeventsource.c:
> 
> some minor style issues:
> arg alignment
> extra whitespace
> missing whitespace
> redundant parens

These are fixed as well, I've been fixing quite a few of these in the old code as I refactored, thanks for spotting these :)

> 
> 
> void
> gdk_event_source_add_translator (GdkEventSource     *source,
>                                  GdkEventTranslator *translator)
> 
> {
>   g_return_if_fail (GDK_IS_EVENT_TRANSLATOR (translator));
> 
>   source->translators = g_list_prepend (source->translators, translator);
> }
> 
> I would have expected a function named _add_ to append, not prepend...

Right, and the order should not be significant, I've committed all these fixes in the xi2 branch, although perhaps I should cherry-pick to events-refactor?


(In reply to comment #4)
> Do we need to do something about the confine_to window for pointer grabs that
> XI2 seems to have lost ?

Not sure if we can do much... that FIXME in gdk/x11/gdkdevice-xi2.c could go I guess. As far as I talked with Peter Hutterer, he took the oportunity to remove some dubious features in the XI redesign, this was one.

> 
> googling found at least one or two users of gdk_pointer_grab with a non-NULL
> confine-to window...

Hmm, that unfortunate, perhaps we should check whether it's use was reasonable, do you remember which apps were these?

(In reply to comment #5)

> >     g_signal_new (g_intern_static_string ("device-added"),
> > 
> > Don't we have the I() macro I used for this elsewhere defined in gdk ?
> 
> No, we don't, it's only defined in gtk/gtkintl.h. Should I commit one in
> gdk/gdkintl.h and fix all usages in master? Makes quite sense to me
> 

I'd just file a bug about it for now and hope for some helping hand to fix it :-)

> > 
> >      device_manager = _gdk_device_manager_new (display);
> >      g_hash_table_insert (device_managers, display, device_manager);
> > 
> > I think this approach is going to lead to lifecycle pain wrt to closing
> > displays. A better approach might be to tack the device manager as object data
> > on the display, and rely on gdk_display_manager_list_displays. In practise, the
> > list will ever only of length 1, so that should be plenty fast enough.
> 
> That code actually changed in the xi2 branch (should have gone into
> events-refactor indeed), there is now a gdk_display_get_device_manager ()
> method.

Yeah, noticed in later review. Looks good to me now.

> > 
> > I would have expected a function named _add_ to append, not prepend...
> 
> Right, and the order should not be significant, I've committed all these fixes
> in the xi2 branch, although perhaps I should cherry-pick to events-refactor?

Yeah, might make sense.
 

> (In reply to comment #4)
> > Do we need to do something about the confine_to window for pointer grabs that
> > XI2 seems to have lost ?
> 
> Not sure if we can do much... that FIXME in gdk/x11/gdkdevice-xi2.c could go I
> guess. As far as I talked with Peter Hutterer, he took the oportunity to remove
> some dubious features in the XI redesign, this was one.

Yeah, thats what he told me as well. I guess together with the GdkEvent abi break this makes the patch a 3.0 candidate.

> > 
> > googling found at least one or two users of gdk_pointer_grab with a non-NULL
> > confine-to window...
> 
> Hmm, that unfortunate, perhaps we should check whether it's use was reasonable,
> do you remember which apps were these?

The one occurrence I remember was the Gimp, but it must have been some old version that google found, since I couldn't find it by grepping through the current gimp sources. So I would not worry too much about just declaring confined grabs a no-longer-supported feature.

Carlos, one thing we should look at wrt to the abi break is whether we can get away with doing the same thing we do with screens for events; see gdk_event_get_screen and gdk_event_set_screen

Yes, that could make sense, although some stack allocated GdkEvents throughout GTK+ should be changed in order to use gdk_event_new(). Could be I'm lucky, but I still haven't seen any misbehavior in any app I have installed related to this (running the xi2 branch for my whole desktop).

Besides this, If this finally isn't going to be a 3.x item, I still think we'd want apps to explicitly enable xi2 awareness at least for the GTK+ 2.x series, perhaps by adding a new gtk_init() variant or something... The GdkDeviceManager is created at gdk_display_open() time, and doing late changes to it could bring up some undesired side effects. 

My main concern here is applications that take X.h events and pointer/keyboard calls (grabs, etc) as granted, such as emacs and metacity. I've also discovered recently that the XEmbed implementation uses XSendEvent() in order to forward events to the GtkPlug. Until I find a way to send the same event types than the GdkDeviceManager available (I'm not even sure we can send xi2 events :/), applications such as gnome-screensaver will ignore keyboard events.

During the last weeks, I've been slowly working on fixing the remaining TODO items. I've also added a gdk_event_set_device() function, so now GdkDevice info goes to private event data, so the ABI break is no longer an issue.

I've also added a global switch to use XI2, if gdk_enable_multidevice() is called before gdk_display_open() and there is support, the XI2 implementation will be used, else either the core or XI1 implementations will be used.

There have been also sort of tricky changes in XEmbed and XDnD implementations, they're mostly about behaving nicely in a backwards compatible way with multiple input devices, but in the future the specs for these might require changes to get multiple device awareness.

So now I really think this is ready for prime time. I've been lately working on API docs for GdkDeviceManager and all new API, if there's still time for this to be in GTK+ 2.20, I'll be adding the Since: and Deprecated: details as well.

I've also updated http://live.gnome.org/GTK+/MPX to reflect this progress.

punting to 3.0

Merged