Re: Pointer/screen problems, part two

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj gtk org> writes:

> > > > Then we can have a:
> > > > 
> > > >  void gdk_event_set_screen (GdkEvent *event);
> > > 
> > > if this is only going to work on events that are allocated
> > > via _gdk_event_new() and thusly freed with gdk_event_free(),
> > > i don't see why you couldn't simply use g_datalist_id_set_data_full().
> 
> sorry, s/g_datalist_id_set_data_full/g_dataset_id_set_data_full/
> 
> > A pretty big percentage of events will actually have this extra 
> > information (all motion notifies, and motion notifies are a big
> > percentage of events).
> > 
> > So, if I need to keep track of "is this an allocated event" using 
> > a hash table, it probably make sense to just to use a separate
> > hash table rather than incurring the overhead of separate
> > blocks to hold the extra information and GData blocks. 
> 
> owen, this kind of hackery (attaching extra data to "foreign"
> structures) is exactly why datasets where introduced. and the
> current implementation is also pretty fast if you're not using
> too long data lists (which prolly isn't the case for events).
> in light of the long term plan to simply move screen into the
> event structure for 2.4, datasets are good and easy enough
> to use for migration (in the end, that's what they exist
> for and provide a hash table for you already),

You mean, for 3.0? 3.0 is a long time off...

The reason why I don't think using dataset is appropriate here 
is that dataset is for extending structures that can't be 
extended in memory. But heap allocated events already have 
a private structure with extra fields in it (GdkEventPrivate)
        
Plus, and this is an important factor for me, using the dataset
in the obvious (and efficient) way, you cannot catch when
someone calls gdk_event_set_screen() on an stack-allocated event.

It will just leak memory and silently cause weird behavior the next 
time an event happens to appear in the same place.

In the end, this is just an implementation detail. I have it
coded now with the separate hash table (see attached patch
which implements gdk_event_set/get_screen() and also sets it
properly for X11). 

I could switch it over to a dataset in a few minutes, and it
would be probably pretty much the same amount of code, and
unlikely to be noticeably different in performance...


Question on gdk_event_new()
 
 GdkEvent *gdk_event_new (void);

or:

 GdkEvent *gdk_event_new (GdkEventType type);

? Advantages of the second:

 - We could theoretically allocate the event short
   for smaller events.
 - We can guarantee that the event is initialized
   properly for the particular event type.
 - It corresponds better to the idea that events
   "derive" from GdkEvent. Changing the type of a
   GdkEvent is conceptually like turning a GtkButton
   into an a GtkEntry.

Still, it would cause problems for fill-in interfaces
that work like gdk_event_translate() (inside GDK we
could clearly use an internal interface). So on the 
balance, preference is the simple first form.

Regards,
                                        Owen

Index: gdk/gdkevents.c
===================================================================
RCS file: /cvs/gnome/gtk+/gdk/gdkevents.c,v
retrieving revision 1.56
diff -u -p -r1.56 gdkevents.c
--- gdk/gdkevents.c	25 Sep 2002 07:23:52 -0000	1.56
+++ gdk/gdkevents.c	2 Oct 2002 19:45:59 -0000
@@ -256,6 +256,7 @@ gdk_event_put (GdkEvent *event)
 }
 
 static GMemChunk *event_chunk = NULL;
+static GHashTable *event_hash = NULL;
 
 GdkEvent*
 _gdk_event_new (void)
@@ -263,17 +264,30 @@ _gdk_event_new (void)
   GdkEventPrivate *new_event;
   
   if (event_chunk == NULL)
-    event_chunk = g_mem_chunk_new ("events",
-				   sizeof (GdkEventPrivate),
-				   4096,
-				   G_ALLOC_AND_FREE);
+    {
+      event_chunk = g_mem_chunk_new ("events",
+				     sizeof (GdkEventPrivate),
+				     4096,
+				     G_ALLOC_AND_FREE);
+      event_hash = g_hash_table_new (g_direct_hash, NULL);
+    }
   
   new_event = g_chunk_new (GdkEventPrivate, event_chunk);
   new_event->flags = 0;
+  new_event->screen = NULL;
+
+  g_hash_table_insert (event_hash, new_event, GUINT_TO_POINTER (1));
   
   return (GdkEvent*) new_event;
 }
 
+static gboolean
+gdk_event_is_allocated (GdkEvent *event)
+{
+  if (event_hash)
+    return g_hash_table_lookup (event_hash, event) != NULL;
+}
+ 
 /**
  * gdk_event_copy:
  * @event: a #GdkEvent
@@ -399,7 +413,8 @@ gdk_event_free (GdkEvent *event)
     default:
       break;
     }
-  
+
+  g_hash_table_remove (event_chunk, event);
   g_mem_chunk_free (event_chunk, event);
 }
 
@@ -736,6 +751,59 @@ gdk_event_get_axis (GdkEvent   *event,
     return FALSE;
 
   return gdk_device_get_axis (device, axes, axis_use, value);
+}
+
+/**
+ * gdk_event_set_screen:
+ * @event: a #GdkEvent
+ * @screen: a #GdkScreen
+ * 
+ * Sets the screen for @event to @screen. The event must
+ * have been allocated by GTK+, for instance, by
+ * gdk_event_copy().
+ **/
+void
+gdk_event_set_screen (GdkEvent  *event,
+		      GdkScreen *screen)
+{
+  GdkEventPrivate *private;
+  
+  g_return_if_fail (gdk_event_is_allocated (event));
+
+  private = (GdkEventPrivate *)event;
+  
+  private->screen = screen;
+}
+
+/**
+ * gdk_event_get_screen:
+ * @event: a #GdkEvent
+ * 
+ * Returns the screen for the event. The screen is
+ * typically the screen for event->any.window, but
+ * for events such as mouse events, it is the screen
+ * where the the pointer was when the event occurs -
+ * that is, the screen which has the root window 
+ * to which event->motion.x_root and
+ * event->motion.y_root are relative.
+ * 
+ * Return value: the screen for the event
+ **/
+GdkScreen *
+gdk_event_get_screen (GdkEvent *event)
+{
+  if (gdk_event_is_allocated (event))
+    {
+      GdkEventPrivate *private = (GdkEventPrivate *)event;
+
+      if (private->screen)
+	return private->screen;
+    }
+
+  if (event->any.window)
+    return gdk_drawable_get_screen (event->any.window);
+
+  return NULL;
 }
 
 /**
Index: gdk/gdkevents.h
===================================================================
RCS file: /cvs/gnome/gtk+/gdk/gdkevents.h,v
retrieving revision 1.17
diff -u -p -r1.17 gdkevents.h
--- gdk/gdkevents.h	25 Sep 2002 19:16:39 -0000	1.17
+++ gdk/gdkevents.h	2 Oct 2002 19:45:59 -0000
@@ -470,6 +470,10 @@ void	  gdk_event_handler_set 	(GdkEventF
 					 gpointer        data,
 					 GDestroyNotify  notify);
 
+void       gdk_event_set_screen (GdkEvent  *event,
+				 GdkScreen *screen);
+GdkScreen *gdk_event_get_screen (GdkEvent  *event);
+
 void	  gdk_set_show_events		(gboolean	 show_events);
 gboolean  gdk_get_show_events		(void);
 
Index: gdk/x11/gdkevents-x11.c
===================================================================
RCS file: /cvs/gnome/gtk+/gdk/x11/gdkevents-x11.c,v
retrieving revision 1.94
diff -u -p -r1.94 gdkevents-x11.c
--- gdk/x11/gdkevents-x11.c	25 Sep 2002 19:16:40 -0000	1.94
+++ gdk/x11/gdkevents-x11.c	2 Oct 2002 19:46:03 -0000
@@ -499,6 +499,19 @@ generate_focus_event (GdkWindow *window,
   gdk_event_put (&event);
 }
 
+static void
+set_screen_from_root (GdkDisplay *display,
+		      GdkEvent   *event,
+		      Window      xrootwin)
+{
+  GdkScreen *screen;
+
+  screen = _gdk_x11_display_screen_for_xrootwin (display, xrootwin);
+  g_assert (screen);
+
+  gdk_event_set_screen (event, screen);
+}
+
 static gboolean
 gdk_event_translate (GdkDisplay *display,
 		     GdkEvent   *event,
@@ -817,6 +830,9 @@ gdk_event_translate (GdkDisplay *display
 	  event->scroll.y_root = (gfloat)xevent->xbutton.y_root;
 	  event->scroll.state = (GdkModifierType) xevent->xbutton.state;
 	  event->scroll.device = display->core_pointer;
+
+	  set_screen_from_root (display, event, xevent->xbutton.root);
+	  
           break;
           
         default:
@@ -832,6 +848,8 @@ gdk_event_translate (GdkDisplay *display
 	  event->button.button = xevent->xbutton.button;
 	  event->button.device = display->core_pointer;
 	  
+	  set_screen_from_root (display, event, xevent->xbutton.root);
+
 	  _gdk_event_button_generate (display, event);
           break;
 	}
@@ -872,6 +890,8 @@ gdk_event_translate (GdkDisplay *display
       event->button.state = (GdkModifierType) xevent->xbutton.state;
       event->button.button = xevent->xbutton.button;
       event->button.device = display->core_pointer;
+
+      set_screen_from_root (display, event, xevent->xbutton.root);
       
       break;
       
@@ -902,6 +922,8 @@ gdk_event_translate (GdkDisplay *display
       event->motion.is_hint = xevent->xmotion.is_hint;
       event->motion.device = display->core_pointer;
       
+      set_screen_from_root (display, event, xevent->xmotion.root);
+      
       break;
       
     case EnterNotify:
@@ -954,6 +976,8 @@ gdk_event_translate (GdkDisplay *display
       event->crossing.x_root = xevent->xcrossing.x_root;
       event->crossing.y_root = xevent->xcrossing.y_root;
       
+      set_screen_from_root (display, event, xevent->xcrossing.root);
+      
       /* Translate the crossing mode into Gdk terms.
        */
       switch (xevent->xcrossing.mode)
@@ -1040,6 +1064,8 @@ gdk_event_translate (GdkDisplay *display
       event->crossing.y = xevent->xcrossing.y + yoffset;
       event->crossing.x_root = xevent->xcrossing.x_root;
       event->crossing.y_root = xevent->xcrossing.y_root;
+      
+      set_screen_from_root (display, event, xevent->xcrossing.root);
       
       /* Translate the crossing mode into Gdk terms.
        */