After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 473822 - Global menubar integration steals key events too eagerly
Global menubar integration steals key events too eagerly
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Quartz
2.11.x
Other Mac OS
: Normal normal
: ---
Assigned To: gtk-quartz maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-09-05 07:40 UTC by Richard Hult
Modified: 2008-05-12 11:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adds "windowing_data" member to GdkEventPrivate (2.94 KB, patch)
2007-10-04 17:12 UTC, Paul Davis
none Details | Review
2nd part of a 4 part fix for key stealing by OSX main menu (3.26 KB, patch)
2007-10-05 02:30 UTC, Paul Davis
none Details | Review
part 3 of a 4 part patch (1.68 KB, text/plain)
2007-10-05 02:36 UTC, Paul Davis
  Details
Fixes the issues pointed out by Tim (2.14 KB, patch)
2007-12-20 19:14 UTC, Richard Hult
none Details | Review
Include the header file changes (3.39 KB, patch)
2008-02-09 22:35 UTC, Richard Hult
none Details | Review
Real version (3.28 KB, patch)
2008-02-17 23:18 UTC, Richard Hult
none Details | Review
Updated version again (4.11 KB, patch)
2008-04-27 09:16 UTC, Richard Hult
rejected Details | Review
Implements Tim's latest suggestion (2.87 KB, patch)
2008-05-05 16:23 UTC, Richard Hult
rejected Details | Review
Owen's suggestion (4.17 KB, patch)
2008-05-05 18:40 UTC, Richard Hult
committed Details | Review

Description Richard Hult 2007-09-05 07:40:27 UTC
(I am not convinced that this should be fixed in GDK or in apps, but filing here so we don't forget it)

When using Gimp with the global mac menubar, and bringing up for example the filechooser and pressing some keys, they are stolen by the menu. This goes for shortcuts that consist of just one key without modifiers.

The reason this happens is because the gdk event handling first of all checks if an event is a shortcut and then passes it to the menubar instead of letting GTK+ handle it. It might be possible to move the "stealing" so that it is only done after GTK+ has checked it. Not sure though.

Mitch, I understand you had some other idea to fix it in Gimp instead?
Comment 1 William Pitcock 2007-09-13 03:57:41 UTC
I've got some ideas on how to fix this (I use the Carbon menubar in Audacious to allow for quick access to some of the features, like a typical MacOS application would.)

I'll try to come up with a patch over the next few days, as the current behaviour is broken for me.
Comment 2 Paul Davis 2007-10-04 17:12:30 UTC
Created attachment 96653 [details] [review]
adds "windowing_data" member to GdkEventPrivate

this adds data and functions for handling windowing backend data that needs to be attached to GdkEvents. idea from Owen Taylor, encouragement from Richard Hult.
Comment 3 Paul Davis 2007-10-04 17:15:45 UTC
Bill, I've implemented a fix for this issue. The rest of the patch (entirely at the GDK/Quartz layer) will show up once I've done some more testing on it. 

The basic idea is to delay forwarding key press events until much later, up in the GTK event handling code. At present, my fix requires application code to make it work, but I'll be looking for a way to automate it too (we need to hook into the key press handlers of GtkWindow key press events at just the right time).
Comment 4 Paul Davis 2007-10-05 02:30:56 UTC
Created attachment 96671 [details] [review]
2nd part of a 4 part fix for key stealing by OSX main menu

This is not necessarily a patch that will apply, since its an edited version of svn diff run against current SVN and all my GDK/Quartz changes. However, it shows the relevant changes reasonably clearly. 

We store the NSEvent as "windowing data" for a GdkEvent, and provide a function to do forwarding to the main menu from this stored event.

The next step is to see if we can find an automatic way to attach this to GtkWindow event handling, at the right spot. My tests with Ardour have involved explicit calls to the forwarding function to prove that the idea works.

This patch references a function found in the next patch, to sync-menu.c, but the function (_gdk_quartz_possibly_forward_accelerator()) should probably be moved into gdkevents-quartz.c if this approach gets accepted.
Comment 5 Paul Davis 2007-10-05 02:36:59 UTC
Created attachment 96672 [details]
part 3 of a 4 part patch

hmm, something went wrong with svn diff for that last patch. this attachment contains the 2 core new functions for gdkevents-quartz.c, one that is exposed to the world, one that is the core functionality for possibly forwarding key press events to the main menu.

sorry about the mess here, but if this gets accepted as a direction for this problem, i can figure out a way to provide a clean 1-shot patch.
Comment 6 Richard Hult 2007-10-10 16:07:59 UTC
It looks good from my perspective (quartz port) at least (and since Owen suggested the gdk part of it, I don't think there will be any problems getting that part in). Did you figure out how to make use of it on the app side?
Comment 7 Paul Davis 2007-10-10 16:13:34 UTC
not yet. 

the problem is that it really needs to be called in the middle of GtkWindow key event handling - before the default handlers, but after any application-installed handlers. most applications don't have a way to do this. so the jury is still out on this front. if emissions hooks could cause signal emission to stop, that would work, but they do not.

however, i am using it in ardour, because we do have our own way to inject the call at precisely the right moment.

it might need support from a higher level in GTK - this concept of "this key in an accelerator but for a menu that is not part of this window" is really alien to GTK right now.
Comment 8 Tim Janik 2007-12-12 09:34:10 UTC
Comment on attachment 96653 [details] [review]
adds "windowing_data" member to GdkEventPrivate

>Index: gdkinternals.h
>===================================================================
>--- gdkinternals.h	(revision 18876)
>+++ gdkinternals.h	(working copy)
>@@ -162,6 +162,8 @@
>   GdkEvent   event;
>   guint      flags;
>   GdkScreen *screen;
>+  gpointer   windowing_data;
>+  GDestroyNotify destroy_notify;
> };
> 
> extern GdkEventFunc   _gdk_event_func;    /* Callback for events */
>@@ -176,6 +178,11 @@
> void      _gdk_events_queue  (GdkDisplay *display);
> GdkEvent* _gdk_event_unqueue (GdkDisplay *display);
> 
>+void      _gdk_event_set_windowing_data (GdkEvent *event,
>+					 gpointer data,
>+					 GDestroyNotify notify);
>+gpointer  _gdk_event_get_windowing_data (GdkEvent *event);
>+
> GList* _gdk_event_queue_find_first  (GdkDisplay *display);
> void   _gdk_event_queue_remove_link (GdkDisplay *display,
> 				     GList      *node);
>Index: gdkevents.c
>===================================================================
>--- gdkevents.c	(revision 18876)
>+++ gdkevents.c	(working copy)
>@@ -281,6 +281,8 @@
>   
>   new_private->flags = 0;
>   new_private->screen = NULL;
>+  new_private->windowing_data = NULL;
>+  new_private->destroy_notify = NULL;
> 
>   g_hash_table_insert (event_hash, new_private, GUINT_TO_POINTER (1));
> 
>@@ -436,6 +438,7 @@
> void
> gdk_event_free (GdkEvent *event)
> {
>+  GdkEventPrivate* private;
>   g_return_if_fail (event != NULL);
> 
>   if (event->any.window)
>@@ -486,10 +489,60 @@

please use diff -up when providing patches to glib/gdk/gtk.

>     }
> 
>   g_hash_table_remove (event_hash, event);
>-  g_slice_free (GdkEventPrivate, (GdkEventPrivate*) event);
>+  private = (GdkEventPrivate*) event;
>+  if (private->windowing_data && private->destroy_notify) 
>+     private->destroy_notify (private->windowing_data);
>+  g_slice_free (GdkEventPrivate, private);

NULL is a valid (data) argument to functions, so the condition here needs to be:

  if (private->destroy_notify) 
    private->destroy_notify (private->windowing_data);

> }
> 
> /**
>+ * _gdk_event_set_windowing_data
>+ * @event:  a #GdkEvent.
>+ * @data:   arbitrary data
>+ * @notify: a #GNotifyDestroy callback 
>+ * 
>+ * Sets backend "windowing" data assocaited with the event.
>+ * Intended for use by backends that need to keep implementation-specific
>+ * data associated with a GdkEvent. @notify can be NULL, otherwise
>+ * it will invoked with @data as its argument whenever @data should
>+ * be freed.
>+ **/
>+void
>+_gdk_event_set_windowing_data (GdkEvent* event,
>+			       gpointer data,
>+			       GDestroyNotify notify)
>+{
>+  GdkEventPrivate* private;
>+  g_return_if_fail (event != NULL);	
>+
>+  private = (GdkEventPrivate*) event;
>+
>+  if (private->windowing_data && private->destroy_notify)
>+     private->destroy_notify (private->windowing_data);

this condition also needs fixing.

>+
>+  private->windowing_data = data;
>+  private->destroy_notify = notify;
>+}
>+
>+/**
>+ * _gdk_event_set_windowing_data
>+ * @event:  a #GdkEvent.
>+ *
>+ * Return value: any "windowing" data set for this event, or NULL 
>+ * if none has been set.
>+ **/
>+gpointer
>+_gdk_event_get_windowing_data (GdkEvent* event)
>+{
>+  GdkEventPrivate* private;
>+  g_return_if_fail (event != NULL);	
>+
>+  private = (GdkEventPrivate*) event;
>+  return private->windowing_data;
>+}
>+	
>+
>+/**
>  * gdk_event_get_time:
>  * @event: a #GdkEvent
>  * 

apart from the above comments, the patch looks basically ok to me, so should go in once the above issues are fixed.
Comment 9 Richard Hult 2007-12-20 19:14:08 UTC
Created attachment 101340 [details] [review]
Fixes the issues pointed out by Tim

Fixes the issues + some whitespace and indentation issues.
Comment 10 Richard Hult 2008-02-09 22:35:15 UTC
Created attachment 104801 [details] [review]
Include the header file changes

Just noticed that the previous patch didn't have the header file changes. I also changed the new functions to be exported, because I am experimenting with this in the external Mac menubar syncing code.
Comment 11 Richard Hult 2008-02-17 23:18:31 UTC
Created attachment 105467 [details] [review]
Real version

Ignore the previous version, this is the real one, includes the fixes to the issues pointed out by Tim as before, just adds the header file change. Sorry about the noise.
Comment 12 Tim Janik 2008-04-25 13:15:23 UTC
Comment on attachment 105467 [details] [review]
Real version

Thanks, the patch looks good and can go in.

>--- gdk/gdkevents.c	(revision 19606)
>+++ gdk/gdkevents.c	(working copy)
>+void
>+_gdk_event_set_windowing_data (GdkEvent       *event,
>+                               gpointer        data,
>+                               GDestroyNotify  notify)
>+{
>+  GdkEventPrivate *private;
>+
>+  g_return_if_fail (event != NULL);	
>+
>+  private = (GdkEventPrivate*) event;
>+
>+  if (private->destroy_notify)
>+    private->destroy_notify (private->windowing_data);
>+
>+  private->windowing_data = data;
>+  private->destroy_notify = notify;
>+}

Note that it is in general better to make destroy-notify-setters reentrant, because destroy handlers are user provided and can execute arbitrary code, e.g. code that re-enters the destroy-notify-setter. This may be neccessary, depending on the complexity of custom destroy-notify handlers (i.e. probably not for windowing data). A reentrant version here looks like:

_gdk_event_set_windowing_data (GdkEvent       *event,
                               gpointer        data,
                               GDestroyNotify  notify)
{
  GdkEventPrivate *private;
  GDestroyNotify last_notify;
  gpointer last_data;

  g_return_if_fail (event != NULL);	

  private = (GdkEventPrivate*) event;
  last_notify = private->destroy_notify;
  last_data = private->windowing_data;

  private->windowing_data = data;
  private->destroy_notify = notify;

  if (last_notify)
    last_notify (last_data);
}

Getting the _free case right is more tricky:
while (private->destroy_notify)
  {
    GDestroyNotify last_notify = private->destroy_notify;
    gpointer last_data = private->windowing_data;
    private->destroy_notify = private->windowing_data = NULL;
    last_notify (last_data);
  }

Sadly, we're not implementing all callback setters in a reenatrant manner all throughout gdk/gtk.
Comment 13 Richard Hult 2008-04-27 09:16:53 UTC
Created attachment 109976 [details] [review]
Updated version again

Tim, thanks for the review. However, I spotted a problem, shouldn't we also handle the windowing data in gdk_event_copy somehow? Attached a new patch which only adds an optional copy_func that the backend supplies (in quartz this will ref the NSEvent). This is used in gdk_event_copy.
Comment 14 Tim Janik 2008-05-02 12:41:26 UTC
(In reply to comment #13)
> Created an attachment (id=109976) [edit]
> Updated version again
> 
> Tim, thanks for the review. However, I spotted a problem, shouldn't we also
> handle the windowing data in gdk_event_copy somehow? Attached a new patch which
> only adds an optional copy_func that the backend supplies (in quartz this will
> ref the NSEvent). This is used in gdk_event_copy.

Urm, having a (data, copy, destroy) triple starts to look a lot like a boxed type. However, adding windowing data to events is something very Gdk backend specific anyway. Considering special treatment needs in gdk_event_copy(), i guess it'd be better to simply add a backend specific pointer to GdkEvent as needed then. I.e. pseudo patching/coding:

 struct _GdkEventPrivate
 {
   [...]
+#ifdef GDK_WINDOWING_QUARTZ
+  gpointer quartz_data;
+#endif
  };
+ void _gdk_quartz_event_data_init (GdkEventPrivate *ep);
+ void _gdk_quartz_event_data_copy (GdkEventPrivate *dst, GdkEventPrivate *src);
+ void _gdk_quartz_event_data_free (GdkEventPrivate *ep);
And then call these in event's new/copy/free ifdef GDK_WINDOWING_QUARTZ.
Comment 15 Richard Hult 2008-05-05 16:23:39 UTC
Created attachment 110414 [details] [review]
Implements Tim's latest suggestion

New patch attached, it adds _gdk_windowing_event_copy/free and implements in quartz. I didn't use the names you suggested as all the recent additions of that kind uses the _gdk_windowing_ naming as far as I can see. It's only called for quartz now, since there is no point in adding empty stubs for the other three backends if they don't need this.
Comment 16 Tim Janik 2008-05-05 16:50:33 UTC
(In reply to comment #15)
> Created an attachment (id=110414) [edit]
> Implements Tim's latest suggestion
> 
> New patch attached, it adds _gdk_windowing_event_copy/free and implements in
> quartz. I didn't use the names you suggested as all the recent additions of
> that kind uses the _gdk_windowing_ naming as far as I can see. It's only called
> for quartz now, since there is no point in adding empty stubs for the other
> three backends if they don't need this.

Well, the exact names don't matter that much. But _gdk_windowing_event_copy/free do need to contain "quartz" somewhere (e.g. _gdk_windowing_quartz_event_copy), since they are used for the quartz_data pointer only.
That is, if we add directfb_data to the event structure tomorrow, it's functions would clash with the names of the quartz backend if the names miss "quartz" / "directfb" respectively.
Other than that, the patch looks fine to me and can go in.
Comment 17 Richard Hult 2008-05-05 17:02:50 UTC
But all the other _gdk_*windowing_* functions are the same as in my patch, i.e. they are declared in gdk/*.h and implemented in gdk/<backend>/*.c. There is no clash because only one backend is built at a time, the directfb function would use the same name. Or am I missing something?

Comment 18 Tim Janik 2008-05-05 17:34:15 UTC
(In reply to comment #17)
> But all the other _gdk_*windowing_* functions are the same as in my patch,
> i.e. they are declared in gdk/*.h and implemented in gdk/<backend>/*.c. There
> is no clash because only one backend is built at a time, the directfb function
> would use the same name. Or am I missing something?

We either introduce a single event data extension mechanism for the
"one and only" compiled backend:
  @@ struct _GdkEventPrivate
  +  gpointer   windowing_data;
  @@
  +void _gdk_windowing_event_data_copy (GdkEvent   *dst,
  +                                     GdkEvent   *src);
  +void _gdk_windowing_event_data_free (GdkEvent   *event);

Or an event extension mechanism *per* backend, which will play along nicely when we compile multiple backends: 
  @@ struct _GdkEventPrivate
  +  gpointer   quartz_data;
  +  gpointer   directfb_data;      
  @@
  +void _gdk_windowing_quartz_event_data_copy   (GdkEvent   *dst,
  +                                              GdkEvent   *src);
  +void _gdk_windowing_quartz_event_data_free   (GdkEvent   *event);
  +void _gdk_windowing_directfb_event_data_copy (GdkEvent   *dst,      
  +                                              GdkEvent   *src);
  +void _gdk_windowing_directfb_event_data_free (GdkEvent   *event);      

My suggestion was to do the latter, in case we have to deal with events being seen by multiple backends in the future.
Comment 19 Owen Taylor 2008-05-05 17:36:26 UTC
_gdk_windowing_<blah> is supposed to be *specifically* a function that
all backends must implement.

_gdk_windowing_quartz_* is (based on how I named things originally) a contradiction in terms.

Given the way the patch is done, it should be _gdk_quartz_..

It's distinctly bad form to have backend specific ifdefs in the
generic code, however. I'd much rather see empty stubs in all the
backends than the way the patch is done.
Comment 20 Richard Hult 2008-05-05 17:53:58 UTC
(In reply to comment #19)
> It's distinctly bad form to have backend specific ifdefs in the
> generic code, however. I'd much rather see empty stubs in all the
> backends than the way the patch is done.

OK, so the same patch but with all backends implementing the functions as stubs if they don't need them.

I assume you'd also have only one data pointer unconditionally added to the event private struct? (Not going with Tim's future-multibackend preparedness...)

Comment 21 Owen Taylor 2008-05-05 18:29:15 UTC
Yes, 4 bytes (or even 8 bytes) per event just doesn't matter... people don't
keep events around persistently.
Comment 22 Richard Hult 2008-05-05 18:40:35 UTC
Created attachment 110420 [details] [review]
Owen's suggestion

Something like this then... Tim, would you be fine with that too?
Comment 23 Tim Janik 2008-05-05 18:46:29 UTC
(In reply to comment #22)
> Created an attachment (id=110420) [edit]
> Owen's suggestion
> 
> Something like this then... Tim, would you be fine with that too?

Jup, looks good. We can still tackle events crossing backends if we actually support multiple backends at some point.
Comment 24 Richard Hult 2008-05-06 21:03:54 UTC
Thanks for the reviews! Committed to trunk, but added a check in _copy that the event is allocated, otherwise we don't have the private parts.

2008-05-06  Richard Hult  <richard@imendio.com>

	* gdk/gdkevents.c: (gdk_event_copy), (gdk_event_free):
	* gdk/gdkinternals.h: Add private backend data to events, and
	handle it when copying/freeing events. Currently only needed in
	the quartz backend.

	* gdk/directfb/gdkevents-directfb.c:
	* gdk/quartz/gdkevents-quartz.c:
	* gdk/win32/gdkevents-win32.c:
	* gdk/x11/gdkevents-x11.c: (_gdk_windowing_event_data_copy)
	(_gdk_windowing_event_data_free): Add stubs for X11, win32 and
	directfb. Implement for quartz. Part of fixing bug #473822.

Leaving the bug open until the remaining bits are taken care of (removing the old menu event special-casing and adding a function to check if an event is a menu accel event).
Comment 25 Richard Hult 2008-05-07 19:23:41 UTC
My plan for the remaining bits:

1. remove the special casing of menu events from gdkevents-quartz.c
2. and move it into the sync-menu code instead
3. add public API to sync-menu to handle a key event, which can be called from apps that deal with this themselves (like gimp and ardour)
4. provide default implementation that apps can enable (most apps will do that)

The reason I don't want to put this in gdk is that the code is quite hackish and also that is uses carbon which I am trying to get rid of, and if we add this as API, chances are we get tied to the carbon implementation.

I will finish and commit this in the next few days (gdk and sync menu).

Comment 26 Richard Hult 2008-05-12 11:24:54 UTC
Committed to trunk and sync-menu.