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 655122 - Crash when resizing window on MacOS Lion
Crash when resizing window on MacOS Lion
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Quartz
2.24.x
Other Mac OS
: Normal major
: ---
Assigned To: gtk-quartz maintainers
gtk-bugs
: 656493 662732 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-07-22 13:17 UTC by Mikayla Hutchinson
Modified: 2012-01-09 15:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix the crash and allow resizing on all window edges in Lion (2.56 KB, patch)
2011-07-24 00:41 UTC, John Ralls
none Details | Review
Don't allow pointer_window to ever be NULL (448 bytes, patch)
2011-07-24 20:00 UTC, Jeffrey Stedfast
none Details | Review
Patch for Gtk2 (3.26 KB, patch)
2011-07-24 21:30 UTC, John Ralls
none Details | Review
Revised Gtk2 Patch (4.65 KB, patch)
2011-07-25 01:40 UTC, John Ralls
none Details | Review
Updated patch to fix bug mentioned in comment #15 (4.70 KB, patch)
2011-07-25 21:53 UTC, Jeffrey Stedfast
none Details | Review
different approach to fixing this bug (6.65 KB, patch)
2011-07-26 19:36 UTC, Jeffrey Stedfast
none Details | Review
Test for resizing in a separate function (4.34 KB, patch)
2011-07-28 23:17 UTC, John Ralls
committed Details | Review
Functions for setting and retrieving the running version of OSX (2.96 KB, patch)
2011-07-29 00:13 UTC, John Ralls
none Details | Review
New version patch using Gestalt() instead of uname() (1.70 KB, patch)
2011-07-29 16:27 UTC, John Ralls
committed Details | Review

Description Mikayla Hutchinson 2011-07-22 13:17:38 UTC
When resizing a window on macOS Lion, GTK crashes hard.

  • #0 find_window_for_ns_event
    at gdkevents-quartz.c line 648
  • #1 gdk_event_translate
    at gdkevents-quartz.c line 1148
  • #2 _gdk_events_queue
    at gdkevents-quartz.c line 1316
  • #3 gdk_event_dispatch
    at gdkeventloop-quartz.c line 667
  • #4 g_main_dispatch
    at gmain.c line 2441
  • #5 g_main_context_dispatch
    at gmain.c line 3014
  • #6 g_main_context_iterate
    at gmain.c line 3092
  • #7 g_main_loop_run
    at gmain.c line 3300
  • #8 gtk_main
    at gtkmain.c line 1256

Comment 1 John Ralls 2011-07-22 21:10:47 UTC
I experimented with it a bit this afternoon: It does re-size, but from the top edge and top right corner instead of the bottom right corner. It's supposed to re-size from any edge or corner on Lion. Unfortunately, after a varying number of resizes, it crashes, as Michael noted.
Comment 2 John Ralls 2011-07-23 14:50:18 UTC
Some additional debug info:

rogram received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c
0x00053c46 in find_window_for_ns_event (nsevent=0xeb8910, x=0xbffff750, y=0xbffff74c, x_root=0xbffff748, y_root=0xbffff744) at gdkevents-quartz.c:650
650                     grab_nswindow = ((GdkWindowImplQuartz *)grab_private->impl)->toplevel;
(gdb) bt
  • #0 find_window_for_ns_event
    at gdkevents-quartz.c line 650
  • #1 gdk_event_translate
    at gdkevents-quartz.c line 1150
  • #2 _gdk_events_queue
    at gdkevents-quartz.c line 1318
  • #3 gdk_event_dispatch
    at gdkeventloop-quartz.c line 666
  • #4 g_main_context_dispatch
  • #5 g_main_context_iterate
  • #6 g_main_loop_run
  • #7 gtk_main
    at gtkmain.c line 1256
  • #8 main
    at testimage.c line 209
Cannot access memory at address 0xc
(gdb) p grab_private
$1 = (GdkWindowObject *) 0x0
(gdb) p grab_toplevel
$2 = (GdkWindow *) 0x0
(gdb) p display
$3 = (GdkDisplay *) 0x10d9000
(gdb) p view
$4 = (GdkQuartzView *) 0x217ae70
(gdb) p toplevel
$5 = (GdkWindow *) 0x10e5170
(gdb) p grab
$6 = (GdkPointerGrabInfo *) 0x217da40
(gdb) p toplevel_under_pointer
No symbol "toplevel_under_pointer" in current context.
(gdb) p *grab
$7 = {
  window = 0x0, 
  native_window = 0x10e5170, 
  serial_start = 0, 
  serial_end = 4294967295, 
  owner_events = 0, 
  event_mask = 0, 
  implicit = 1, 
  time = 683779, 
  activated = 1, 
  implicit_ungrab = 0
}
(gdb) p grab_event_mask_from_ns_event(nsevent)
No symbol "grab_event_mask_from_ns_event" in current context.
(gdb) p get_event_mask_from_ns_event(nsevent)
$8 = 316
(gdb) p /x get_event_mask_from_ns_event(nsevent)
$9 = 0x13c
(gdb) 

This explains the crash: Since grab->window is NULL, the following tries to dereference a NULL pointer:
	grab_toplevel = gdk_window_get_effective_toplevel (grab->window);
        grab_private = (GdkWindowObject *)grab_toplevel;
        grab_nswindow = ((GdkWindowImplQuartz *)grab_private->impl)->toplevel;

So, a potential fix is:

GdkWindow* grab_win = grab->window ? grab->window : grab->native_wndow;
if (!grab_win) return NULL;

That will at least prevent the crash, though it won't do anything for the goofy resize behavior.

Kris, you have a much better handle on how grabs work than I do. Will this work?
Comment 3 John Ralls 2011-07-23 15:13:30 UTC
Here's the backtrace for gtk+-3. Same crash, the code has just been extracted to a new function:
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c
0x00665c3b in find_toplevel_for_mouse_event (nsevent=0x56abec0, x=0xbffff6c0, y=0xbffff6bc) at gdkevents-quartz.c:610
610               grab_nswindow = ((GdkWindowImplQuartz *)grab_toplevel->impl)->toplevel;
(gdb) bt
  • #0 find_toplevel_for_mouse_event
    at gdkevents-quartz.c line 610
  • #1 find_window_for_ns_event
    at gdkevents-quartz.c line 728
  • #2 gdk_event_translate
    at gdkevents-quartz.c line 1149
  • #3 _gdk_quartz_display_queue_events
    at gdkevents-quartz.c line 1316
  • #4 gdk_event_dispatch
    at gdkeventloop-quartz.c line 666
  • #5 g_main_dispatch
    at gmain.c line 2500
  • #6 g_main_context_dispatch
    at gmain.c line 3083
  • #7 g_main_context_iterate
    at gmain.c line 3161
  • #8 g_main_loop_run
    at gmain.c line 3369
  • #9 gtk_main
    at gtkmain.c line 1362
  • #10 main
    at testimage.c line 209

Comment 4 John Ralls 2011-07-23 20:49:33 UTC
Using grab->native_window when the grab-window == NULL works to prevent the crash.

Digging a bit into the resizing, I find that the reason that resizing works along the top edge is that first, there's no grab on a titlebar (so the grab bug is avoided) and second, a mouse-move event on the titlebar is ignored, leaving quartz free to resize the window.

So I added a line to return NULL just before the one that was causing the crashes, and presto!, resizing now works on all edges and corners. That's not the fix, obviously, but it does point the way.
Comment 5 John Ralls 2011-07-24 00:41:26 UTC
Created attachment 192547 [details] [review]
Fix the crash and allow resizing on all window edges in Lion

This works. It's not perfect, because it doesn't get to the root cause of the grab having native window set but not window, but it seems to be otherwise correct.
Comment 6 Mikayla Hutchinson 2011-07-24 01:15:16 UTC
Fantastic!

I ported the patch to vanilla gtk-2-24 (it didn't apply as-is), and I had to change a couple of grab_window references to grab_private, which I *hope* is correct: https://github.com/mhutch/bockbuild/blob/master/packages/patches/gtk/gtklionresizecrash.patch

It mostly works for me (on Lion). Often GTK doesn't seem to track the grab or the grab release correctly and strange things happen, but it's not clear to me how to reproduce this reliably.
Comment 7 Mikayla Hutchinson 2011-07-24 01:23:18 UTC
Ah, I meant change grab_toplevel references to grab_private.

I figured out one of the issues. You can drag faster than the window resizes, and therefore get out of the 5px border, at which point GTK interrupts the resize.
Comment 8 John Ralls 2011-07-24 11:59:19 UTC
Yes, the patch is against Gtk3. I ran out of time yesterday before I got to backporting it.

Makes sense. We really want the grab to register the border region and set a resizing flag. I'll have a look at that. Maybe in the process I'll see why the grab loses (or doesn't get) the window pointer.
Comment 9 Jeffrey Stedfast 2011-07-24 19:46:45 UTC
gdkwindow.c:proxy_button_event() is sometimes failing to get the pointer_window from the toplevel_window using _gdk_window_find_descendant_at().

A simple fix seems to be:

if (pointer_window == NULL) pointer_window = toplevel_window;

I'll attach a patch soon.
Comment 10 Jeffrey Stedfast 2011-07-24 20:00:40 UTC
Created attachment 192576 [details] [review]
Don't allow pointer_window to ever be NULL
Comment 11 Jeffrey Stedfast 2011-07-24 20:02:38 UTC
Er, I probably shouldn't have marked John's patch as obsoleted by mine, since my patch really only obsoletes the grab->window ? grab->window : grab->native_window bit of his patch.
Comment 12 John Ralls 2011-07-24 20:09:17 UTC
Easy enough to fix: Just select "details" of the obsoleted patch and uncheck the box.
Comment 13 John Ralls 2011-07-24 21:30:29 UTC
Created attachment 192587 [details] [review]
Patch for Gtk2

This patch goes on Gtk2. It uses Jeff's fix in gdkwindow.c instead of selecting window or native_window as the other patch does. It still checks grab->window to protect against a possible crash. (I changed Jeff's fix a bit, moving the setting of pointer_window to after the parent check.)

It also addresses Michael's complaint that the resize can get stopped by overrunning the edge detection by adding a new gboolean to GdkPointerGrabInfo and setting it if the first post-grab event is inside the 5-pixel border, then returning NULL if we find it set. I haven't been able to reproduce the problem, so Michael, please test it.

I'm not sure if this affects ABI... Kris, is it acceptable?
Comment 14 John Ralls 2011-07-25 01:40:32 UTC
Created attachment 192592 [details] [review]
Revised Gtk2 Patch

Ah, now I see a problem with the patch. This might be what Michael noticed earlier and diagnosed as running out of the border:

The first time after restoring focus to the window, it takes a moment for the grab to be recognized. If you click and immediately start to drag, nothing happens; if you click and wait, then drag, resizing works as expected. After the first attempt, subsequent attempts work instantly until the window loses focus.

So this patch adds another border handler for the ungrabbed case. It also fixes an omission of checking the new grabinfo->resize parameter and returning NULL if it's set, and replaces the "magic number" 5 with a const gint GDK_LION_RESIZE.
Comment 15 Mikayla Hutchinson 2011-07-25 07:33:52 UTC
Works well, and fixes all the GTK type assertions that happened when resizing too. Thanks!

There's one remaining issue I can find - clicking once in the resize border (i.e.. not dragging) seems to take a grab which is not released. It can only be reset by clicking on the window titlebar.
Comment 16 Ed 2011-07-25 13:21:48 UTC
I hesitate to display my ignorance, but does anyone have an idea on how long it may take for these patches to be compiled into the SourceForge DMG package? Thanks!

-Ed
Comment 17 John Ralls 2011-07-25 13:30:05 UTC
What SF DMG package?
Comment 18 Ed 2011-07-25 13:33:42 UTC
Here is the link I'm referring to, which is also in the purple "Downloads" box at the top-right of www.gnucash.org (for "Mac OSX Intel"):
http://downloads.sourceforge.net/sourceforge/gnucash/Gnucash-Intel-2.4.7.dmg

Thanks!
Comment 19 John Ralls 2011-07-25 13:42:13 UTC
Ah. This is a Gtk+ bug, please don't post user questions about applications here. 
In the case of Gnucash, you should ask questions on gnucash-user@gnucash.org.

I've already answered your question there for the bugs which we have solutions to. This isn't one of them yet.
Comment 20 Kristian Rietveld 2011-07-25 18:42:06 UTC
(In reply to comment #7)
> I figured out one of the issues. You can drag faster than the window resizes,
> and therefore get out of the 5px border, at which point GTK interrupts the
> resize.

Is this really happening?  When I test with the old corner-only resize grip on Tiger, Cocoa will swallow all events once a resize has started until the mouse button has been released.  Therefore, we did not have a need for any code tracking the resize before.  I would really expect Lion to behave the same when a resize has been started on the border -- otherwise I would almost assume this a bug in Lion's resize logic.  I will check the AppKit release notes to see what's mentioned about this.
Comment 21 Kristian Rietveld 2011-07-25 19:04:30 UTC
This article has some interesting details on window edge resizing:

http://arstechnica.com/apple/reviews/2011/07/mac-os-x-10-7.ars/4
Comment 22 Mikayla Hutchinson 2011-07-25 19:44:29 UTC
I *might* have been wrong, but it looked that way to me. Something certainly wasn't right, and it seems better with the new patch.

One subtlety of the new resizing is that Cocoa does pass through click events in the resize area - this patch isn't doing that.
Comment 23 Jeffrey Stedfast 2011-07-25 21:53:03 UTC
Created attachment 192641 [details] [review]
Updated patch to fix bug mentioned in comment #15

This patch is basically just an update to John Ralls' patch but fixes the bug Michael Hutchinson mentions in comment #15 about how clicking in the resize area and then releasing the mouse button keeps the pointer grab active thus disallowing further clicks in the window until you click the title bar.

Basically the fix is to not set grab->resizing to TRUE if the event type is NSLeftMouseUp.
Comment 24 John Ralls 2011-07-26 03:41:36 UTC
(In reply to comment #22)
> I *might* have been wrong, but it looked that way to me. Something certainly
> wasn't right, and it seems better with the new patch.
> 
> One subtlety of the new resizing is that Cocoa does pass through click events
> in the resize area - this patch isn't doing that.

I think what's really going on is that Gdk takes a moment to establish the grab, and if you start moving before then the event handler goes to the non-grab part of the logic. I fixed that with a second set of edge detection, but I think the final patch should move that to an edge (for Lion) and lower-right-corner (everything before) resize handler for NSLeftDrag that falls through to the other handlers.

If I'm right, the grab->resizing thing can go away.
Comment 25 John Ralls 2011-07-26 04:10:59 UTC
(In reply to comment #21)
> This article has some interesting details on window edge resizing:
> 
> http://arstechnica.com/apple/reviews/2011/07/mac-os-x-10-7.ars/4

Well, I've got that mostly covered. I need to figure out how to see if the scrollbars are visible and enable that part when appropriate.

The extra border region outside of the window might be the reason we were getting a grab->native_window and no grab->window, too... if he's right about that. It's not obvious to me where the active point on those resize cursors is, but it appears to me that the resize works for every starting point that the resize cursor is set.
Comment 26 Mikayla Hutchinson 2011-07-26 05:56:20 UTC
(In reply to comment #25)
> The extra border region outside of the window might be the reason we were
> getting a grab->native_window and no grab->window, too... if he's right about
> that. It's not obvious to me where the active point on those resize cursors is,
> but it appears to me that the resize works for every starting point that the
> resize cursor is set.

Yes, if I start the resize outside the window I still get the unreleased grab, even with Jeff's patch. When the unreleased grab is in place, resizing from outside the window works.
Comment 27 Jeffrey Stedfast 2011-07-26 11:12:26 UTC
Michael: ugh, really? My patch for the grab->resize thing seemed to fix it for me :-(
Comment 28 Kristian Rietveld 2011-07-26 11:23:06 UTC
(In reply to comment #22)
> I *might* have been wrong, but it looked that way to me. Something certainly
> wasn't right, and it seems better with the new patch.

Ah, so the "GTK+ interrupts the resize" is without a patched GTK+ on Lion.  That likely makes sense, because then both the window manager and GTK+ are processing the event.

> One subtlety of the new resizing is that Cocoa does pass through click events
> in the resize area - this patch isn't doing that.

What I need to know is whether Cocoa also passes through any other/subsequent events.  How the resizing logic for GTK+ is working for older OS X releases is:

  1. GTK+ gets a button down event.  If we see that this is in the lower-right resize area, we will ignore this event.  There is no need to create an artificial grab.
  2. Cocoa will restart the resizing and no mouse events are received by GTK+.
  3. When the resizing ends, everything is back to normal.

So in 2, Cocoa essentially grabs all events.  If the Lion resizing behaves similar, but only with all of the window border as starting point, then we simply need to replace/extend the lower-right resize area with the entire window border and we're done.

If the application continues to receive mouse dragged events while the window is in resize, how can the application distinguish between mouse dragged events handled by the window manager and other events?
Comment 29 John Ralls 2011-07-26 12:47:23 UTC
Hmm. I'm not sure I agree that under previous versions CF isn't sending Gtk mouse events; the code appears to handle any mouse events in that corner as resize events regardless of what kind of events they are. I am sure that in Lion, CF does send NSLeftDrag events while resizing. That's not really a problem, we just need to recognize that an NSLeftDrag in the border region is a resize and let Cocoa do its thing, while handling all other mouse events in the border region normally. I suspect that we can do the same with the lower-right-corner on earlier versions and when scrollbars are enabled on Lion.

find_window_for_ns_event() is perhaps not the best place to be doing this. It rather makes the function name misleading, and I don't really like idea of renaming it to "find_window_for_ns_event_and_oh_by_the_way_handle_window_resizing_too()". ;-)
Comment 30 Jeffrey Stedfast 2011-07-26 16:21:08 UTC
Gtk+ is definitely getting NSLeftMouseDragged events on Lion when resizing a window.
Comment 31 Jeffrey Stedfast 2011-07-26 19:36:44 UTC
Created attachment 192697 [details] [review]
different approach to fixing this bug

Here's a different approach which does not need the grab->resizing hack and also doesn't seem to suffer from any of the pointer grab bugs noticed above.

We might even be able to trim out at least 1 more of the lion checks:


                /* If we're on Lion and within 5 pixels of an edge, then                                                                                                         
                 * assume that the user wants to resize, and return NULL to                                                                                                      
                 * let Quartz get on with it. We check the selector                                                                                                              
                 * isRestorable to see if we're on 10.7.                                                                                                                         
                 */
                if (([grab_nswindow respondsToSelector:@selector(isRestorable)]) &&
                    (*x < GDK_LION_RESIZE ||
                     *x > grab_private->width - GDK_LION_RESIZE ||
                     *y > grab_private->height - GDK_LION_RESIZE))
                  {
                    return NULL;
                  }

I can't seem to get this one to hit anymore, so we can probably just drop it.
Comment 32 Mikayla Hutchinson 2011-07-26 20:03:50 UTC
This seems to work well, I can't find any problems with it.

It still doesn't allow clicking on widgets within the 5px border but that's very minor and probably not worth worrying about.
Comment 33 Kristian Rietveld 2011-07-28 15:29:31 UTC
(In reply to comment #31)
> Here's a different approach which does not need the grab->resizing hack and
> also doesn't seem to suffer from any of the pointer grab bugs noticed above.

Great!  I was not very happy with the resizing hack ...


> We might even be able to trim out at least 1 more of the lion checks:
> 
> 
>                 /* If we're on Lion and within 5 pixels of an edge, then        
>                  * assume that the user wants to resize, and return NULL to     
>                  * let Quartz get on with it. We check the selector             
>                  * isRestorable to see if we're on 10.7.                        
>                  */
>                 if (([grab_nswindow
> respondsToSelector:@selector(isRestorable)]) &&
>                     (*x < GDK_LION_RESIZE ||
>                      *x > grab_private->width - GDK_LION_RESIZE ||
>                      *y > grab_private->height - GDK_LION_RESIZE))
>                   {
>                     return NULL;
>                   }
> 
> I can't seem to get this one to hit anymore, so we can probably just drop it.

This is the check in the grabbed case, right?  It is a good sign that you cannot hit this one, because I think it should go.  Once GTK+ is in a grab, it should actually not be interrupted by a resize (if that's possible to achieve).

My other two questions at the moment are:

 1) Is the patch:

+      if (!pointer_window)
+	  pointer_window = toplevel_window;

still needed?  I rather not add that to gdkwindow.c

 2) In another bug, John has indicated that respondsToSelector: can be quite expensive.  I think we should do this check once and then cache the value in the Window or Display object.
Comment 34 John Ralls 2011-07-28 15:43:27 UTC
(In reply to comment #33)
> (In reply to comment #31)
> > Here's a different approach which does not need the grab->resizing hack and
> > also doesn't seem to suffer from any of the pointer grab bugs noticed above.
> 
> Great!  I was not very happy with the resizing hack ...
> 
> 
> > We might even be able to trim out at least 1 more of the lion checks:
> > 
> > 
> >                 /* If we're on Lion and within 5 pixels of an edge, then        
> >                  * assume that the user wants to resize, and return NULL to     
> >                  * let Quartz get on with it. We check the selector             
> >                  * isRestorable to see if we're on 10.7.                        
> >                  */
> >                 if (([grab_nswindow
> > respondsToSelector:@selector(isRestorable)]) &&
> >                     (*x < GDK_LION_RESIZE ||
> >                      *x > grab_private->width - GDK_LION_RESIZE ||
> >                      *y > grab_private->height - GDK_LION_RESIZE))
> >                   {
> >                     return NULL;
> >                   }
> > 
> > I can't seem to get this one to hit anymore, so we can probably just drop it.
> 
> This is the check in the grabbed case, right?  It is a good sign that you
> cannot hit this one, because I think it should go.  Once GTK+ is in a grab, it
> should actually not be interrupted by a resize (if that's possible to achieve).
> 
> My other two questions at the moment are:
> 
>  1) Is the patch:
> 
> +      if (!pointer_window)
> +      pointer_window = toplevel_window;
> 
> still needed?  I rather not add that to gdkwindow.c
> 
>  2) In another bug, John has indicated that respondsToSelector: can be quite
> expensive.  I think we should do this check once and then cache the value in
> the Window or Display object.

Yes, it's the instance in the grabbed case. If we can keep a resize from creating the grab, that would indeed be best.

For safety we need to either prevent pointer_window from being NULL or make sure that we gracefully handle when it is -- or better yet, both. Segfaults are never acceptable.

Detecting and caching the OSX version somewhere would be helpful in a lot of places. It would be nice if there was a cleaner way of doing it than running respondsToSelector on a series of methods introduced in each version, but I haven't been able to find one. Ideally done while initializing the backend, it needs to be stored somewhere that's easy to retrieve without passing around an extra parameter.
Comment 35 Mikayla Hutchinson 2011-07-28 15:46:06 UTC
(In reply to comment #33)
>  2) In another bug, John has indicated that respondsToSelector: can be quite
> expensive.  I think we should do this check once and then cache the value in
> the Window or Display object.

Since we're only really doing it for version detection, presumably a "static gboolean macos_at_least_lion" somewhere would do the trick?
Comment 36 Kristian Rietveld 2011-07-28 15:49:22 UTC
(In reply to comment #35)
> (In reply to comment #33)
> >  2) In another bug, John has indicated that respondsToSelector: can be quite
> > expensive.  I think we should do this check once and then cache the value in
> > the Window or Display object.
> 
> Since we're only really doing it for version detection, presumably a "static
> gboolean macos_at_least_lion" somewhere would do the trick?

Either that prefixed with _gdk_ or putting it in Display should work.  Currently there is only one display to deal with in the quartz backend (it is even defined globally as _gdk_display...), however, if you compile for both quartz and x11 at the same time you might get into trouble.  So a static gboolean might be safest.
Comment 37 John Ralls 2011-07-28 18:07:47 UTC
Head-slap time.
We should just parse the release element from uname (3).

I'd rather have a guint with the actual version than a gboolean. Might be useful in other places (ATSUI vs. CoreText comes to mind), and it's more future-proof.

A global function gdk_quartz_getversion() to retrieve it would be great.
Comment 38 John Ralls 2011-07-28 23:17:57 UTC
Created attachment 192841 [details] [review]
Test for resizing in a separate function

I did a little experimentation, printing out the mouse event types in the resize area. It turns out that Kris is right: Cocoa emits an NSLeftMouseDown at the beginning of the resize and then eats all of the NSLeftMouseDragged events so that we never see them.

As I've said, I don't really like overloading the "find-the-window" function with the resize detection, so this patch pulls it out into its own function. Since there's no grab it should also avoid the segfault from grab->window being NULL, and it doesn't do anything about that (though I think that we should).

It's also using a respondsToSelector to detect Lion. I'll write up a separate patch for using uname for that.

This is against Gtk2 so that Michael and Jeffrey can test it; it should port over to master pretty easily.
Comment 39 John Ralls 2011-07-29 00:13:48 UTC
Created attachment 192844 [details] [review]
Functions for setting and retrieving the running version of OSX

Creates an enum type for the version and a function which runs from gdk_display_quartz_init to set a static variable with the appropriate value of GdkOSXVersion and a getter function, gdk_quartz_osx_version() to retrieve it. The test in test_resize() becomes:
  if (gdk_quartz_osx_version () >= GDK_OSX_LION
      && (x < GDK_LION_RESIZE
	  || x > toplevel_private->width - GDK_LION_RESIZE
	  || y > toplevel_private->height - GDK_LION_RESIZE))

and we can use the same in gdk_window_set_decorations. It will be faster, but it will still emit the compiler warning when built with older SDKs.
Comment 40 Kristian Rietveld 2011-07-29 11:35:49 UTC
(In reply to comment #39)
> Created an attachment (id=192844) [details] [review]
> Functions for setting and retrieving the running version of OSX
> 

I like using an enumeration and function call.  However, perhaps we can better use Gestalt instead of uname?  This is suggested in:

  http://www.cocoadev.com/index.pl?DeterminingOSVersion
Comment 41 Kristian Rietveld 2011-07-29 11:39:31 UTC
(In reply to comment #38)
> Created an attachment (id=192841) [details] [review]
> Test for resizing in a separate function
> 
> I did a little experimentation, printing out the mouse event types in the
> resize area. It turns out that Kris is right: Cocoa emits an NSLeftMouseDown at
> the beginning of the resize and then eats all of the NSLeftMouseDragged events
> so that we never see them.

Great!  I knew it would have to work like this, otherwise it becomes pretty problematic.  Right now the patch pretty much looks like what I would expect.

> As I've said, I don't really like overloading the "find-the-window" function
> with the resize detection, so this patch pulls it out into its own function.
> Since there's no grab it should also avoid the segfault from grab->window being
> NULL, and it doesn't do anything about that (though I think that we should).

I agree.

> This is against Gtk2 so that Michael and Jeffrey can test it; it should port
> over to master pretty easily.

I am eager to hear how this works for Michael and Jeffrey.
Comment 42 John Ralls 2011-07-29 16:27:26 UTC
Created attachment 192882 [details] [review]
New version patch using Gestalt() instead of uname()

Nice find. In addition to being the Apple-sanctioned way, it saves the sscanf. Since it's now basically a lookup-and-translate function, there's no need to cache the result, so I've collapsed it to a single function and no static variable.
Comment 43 Jeffrey Stedfast 2011-08-01 21:17:28 UTC
I'll try to test these latest updates soon. Just swamped in other bugs atm.
Comment 44 Mikayla Hutchinson 2011-08-01 22:55:05 UTC
On my queue too. I've been stuck bootcamped into Windows handing some other urgent stuff.
Comment 45 Kristian Rietveld 2011-08-13 19:32:37 UTC
Comment on attachment 192841 [details] [review]
Test for resizing in a separate function

For the lion check, I gather there is no y < check because we don't handle the title bar?  Should this be commented?

If the lion check is replaced with the code from comment 42, I am fine with accepting this for 2-24 and master.
Comment 46 Kristian Rietveld 2011-08-13 19:34:14 UTC
Comment on attachment 192882 [details] [review]
New version patch using Gestalt() instead of uname()

My only worry here was whether calling Gestalt() on each event would introduce a performance problem.  I would propose we commit it like this and if Gestalt() calls show up in profiles at some point, we can always add a cache for it.
Comment 48 Garrett Regier 2011-08-14 02:50:12 UTC
*** Bug 656493 has been marked as a duplicate of this bug. ***
Comment 49 John Ralls 2012-01-09 15:25:06 UTC
*** Bug 662732 has been marked as a duplicate of this bug. ***