Re: Bad portability by foreign_new_for_display

[Hans Breuer <hans breuer org>]

At 26.01.2011 17:33, Benjamin Otte wrote:
> Hans Breuer<hans<at>  breuer.org>  writes:
>
>>
>> Current master has three occurences of
>>
>> #ifdef GDK_WINDOWING_X11
>>     if (GDK_IS_X11_DISPLAY (display))
>>
>> followed by some
>>       window = gdk_x11_window_lookup_for_display (display, ...);
>>
>> Only one was ported to win32 and currently none for OSX, leading
>> to e.g. a crash during clipboard copy.
>>
>> Wouldn't it be better to add foreign_new_for_display() to
>> the display vtable, rather than adding all these ifdefs?
>>
> I don't think that'd work very well, for 2 reasons:
> 1) If it was a vtable entry, what argument would it take? gpointer? We 
can't use
> GdkNativeWindow like GTK2 does, because it's tyepdeffed based on backend 
and we
> can't do that in GTK3 with multiple backends.
Correct, I've overlooked the suggested loss of GdkNativeWindow during the 
creation of backend specific foreign_new_for_display(), but I think using a 
gpointer and e.g. have the backend specifc delegates done _once_ in say

 gdk_foreign_new_for_display(GdkDisplay *, gpointer native_window)

would be better than doing this #ifdef stuff three times in Gtk alone. And 
also putting the burden on every other user of foreign_new_for_display()

> 2) You need backend-specific code to extract the native window anyway, so
> special casing will be necessary.
>
But aren't vtables for e.g. GdkDisplay exactly used for this purpose?

> But you point out one important thing: GtkSocket/GtkPlug are not yet API 
ready
> for multi-backend GTK. They need to be changed before we can ship GTK 3.0.
>
The point I was trying to make was a different one: making it easier to 
implement a Gdk backend should not make it harder to use them.

And if this is still not convincing enough to adapt the API, I'd like to 
get a proposal for a better fix than:

 gtk/gtkselection.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gtk/gtkselection.c b/gtk/gtkselection.c
index 105acb8..0221953 100644
--- a/gtk/gtkselection.c
+++ b/gtk/gtkselection.c
@@ -2331,6 +2331,10 @@ _gtk_selection_request (GtkWidget *widget,
   if (GDK_IS_X11_DISPLAY (display))
     info->requestor = gdk_x11_window_foreign_new_for_display (display, 
event->requestor);
   else
+#elif defined GDK_WINDOWING_WIN32
+  if (GDK_IS_WIN32_DISPLAY (display))
+    info->requestor = gdk_win32_window_foreign_new_for_display (display, 
event->requestor);
+  else
 #endif
     info->requestor = NULL;


BTW: the definition of GdkNativeWindow is still part of the Gdk API, see: 
gdk/gtktypes.h and GdkEventSelection::requestor in gdk/gdkevents.h

Thanks,
    Hans

-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to
get along without it.                -- Dilbert