GNOME Bugzilla – Bug 550942
[patch] Rework of gdkeventloop-quartz.c
Last modified: 2008-11-12 13:32:01 UTC
The patch that I'll attach here is a major rework of gdkeventloop-quartz.c. The biggest change in the patch is that it makes use of CFRunLoopObserver to iterate the GLib main loop when a native OS X run loop is running, as during a modal operation like window resizing or drag and drop. This means that windows redraw during resize and DND "drag under" highlighting works. Another major change is in the communication with the select thread. Rather than doing synchronous communication where it asks the select thread to start polling, waits for it to start polling, then after waiting for events, wakes up the select thread, now it just submits new sets of poll descriptors to the select thread asynchronously. Doing the thread polling asynchronously is enabled by always doing a timeout=0 poll in the main thread first. The timeout 0 poll is needed in any case to get the behavior of idle functions correct: a low priority idle function should not fire if there is data waiting on a file descriptor. The asynchronous polling is already a win in reducing context switches, but the technique becomes even more powerful because we can simply leave the existing poll going if the new set of file descriptors is the same as the old one; there are some subtle aspects to this documented in the comments. Finally, I added a lot of comments to the existing code as well as the new code and reordered things a bit, putting the final nail in the coffin of the usefulness of reviewing this as a diff. (I'll attach the entire file as well.) There are also a couple of small changes worth noting: * I removed the call to nextEventMatchingMask in gdk_event_prepare(); I think it was only there by false analogy to the GDK code where you have to call XPending() before polling the X file descriptor to avoid blocking if there are events in the queue. * I removed the code to try and reuse the slot with the dummy file descriptor for our wakeup pipe; it was complexity for no performance gain. (The GLib poll function used on OS X handles -1 fd's fine. This might need rethinking of OS X ever gains a native poll() system call.) * I made the code always try to pull a Cocoa event, even if the dummy poll FD wasn't in the FD set. The dummy poll FD not in the set only occurs in the rare case where a higher than default sources *has already* said that is ready to dispatch. It should be fine to queue up another event in that case too. One thing to note is that while the Quartz run loop is running, GLib timeouts, idles, and IO watches fire and synthetic events like CONFIGURE events are delivered, but mouse and keyboard events are not delivered to GTK+ apps... nothing is pulling them off the event queue. For window resizing and other "grab" type applications of manual polling of the event loop this is right; there may be other situations where we want to deliver all sorts of events. Investigating run loop modes in detail is likely needed to figure out exactly what to do. I'm pretty happy with how this all works... window resizing works and is pretty smooth, DND is a lot better. It seems robust with a test program I wrote (will attach), gtk-demo, and my own small application. I would not be surprised if there are lurking bugs revealed by other larger apps with more Quartz/Cocoa integration. I've left in the debug prints guarded by a new GDK debug facility GDK_DEBUG=eventloop. That all being said, while I think the patch is a good thing and useful for various reasons, I'm not sure it's the best way to handle window resizing. On X, doing layout and repaint during idle during window is important because otherwise the window manager can get arbitrarily far ahead of the application. With Quartz, the frame is being drawn *by* the application, so doing layout and resize synchronously in windowDidResize may be best - that will result in the frame and contents being redrawn as a single unit with no lag between them. This would require some window system #ifdefs in gtkwindow.c.
Created attachment 118071 [details] [review] Patch reworking event loop handling
Created attachment 118072 [details] Resulting gdkeventloop-quartz.c for convenient reading
Created attachment 118073 [details] Test program
This looks very interesting. Do you think this will fix the issues of qtk-quartz losing keypress events when under load? Here is a description of that problem I submitted to the Imendio mailing list: KeyPressEvents seem to be handled differently using the X11 backend and the Mac backend. Using X11 ALL keypress events are queued and handled by the main loop when it gets around to them. So if I have a big computation going and hit two key presses, the main loop will see them when the big computation stops and the main loop runs. But with the Mac backend key presses are not be seen by the main loop if they are pressed while a big computation is running. They just disappear. This greatly changes the behavior of my application (making it very hard to use reliable). Richard Hult was able to confirm this but AFAIK has not been able to fix it. (I stopped using gtk-quartz because of this issue.) It sounds like you know a lot about main loop issues and could tackle it.
Trunk should not lose any events, it was fixed a while ago.
(In reply to comment #5) > Trunk should not lose any events, it was fixed a while ago. > OK. I'll give that try. Thanks and sorry for the noise...
(In reply to comment #5) > Trunk should not lose any events, it was fixed a while ago. > OK. I've tried the recently released Mono Framework 2.0_3 release candidate and the KeyPressEvent problem described above is still present. The framework lists its gtk+2 version as 2.15.0-0, so I don't know if you would expect it to be fixed in that version (if not then I will request that the mono folks use a newer version). Also, the issue with the menubar not being redrawn when returning from fullscreen is also present. Again thanks and sorry about the noise on this bug report (let me know if it should go off list).
If they call their GTK+ version 2.15 then it's probably pretty old, since the unstable branch was numbered that way for a short while before it was changed to 2.13. So my guess is that the fix is not in their version.
I've tested this patch for a while now and it works fine for all my test apps (and fixes an annoying deadlock in events with gimp too). I plan to commit this to trunk to get some more widespread testing, but it seems no matter what it's better than the current version. Very nicely commented too, thanks a lot!
Committed to trunk to get this more widely used and tested. So far I have not seen any issues.