Created attachment 192421 [details] [review]
Fixes the crash

Review of attachment 192421 [details] [review]:

setStyleMask is (officially) introduced in 10.6. 
The fix is actually simpler; just
[impl->toplevel release];
impl->toplevel = [[GdkQuartzNSWindow alloc] initContentWithRect...
(See 655078, which I'll make this a duplicate of.)

Although 655078 is a little newer, it contains a more backward-compatible fix.

*** This bug has been marked as a duplicate of bug 655078 ***

Not sure what you mean, checking the object responds to the selector is a perfectly backwards-compatible fix, and more correct.

Also, my first patch implements the fix the way you other report describes.

Note: both patches work fine on Lion, but I haven't tested on older OSes.

(In reply to comment #4)
> Not sure what you mean, checking the object responds to the selector is a
> perfectly backwards-compatible fix, and more correct.

On the theory that if it doesn't respond to the selector the old way is OK? I guess, since Apple isn't going to go back and fix whatever they thought was broken.

(In reply to comment #5)
> Also, my first patch implements the fix the way you other report describes.

But you replaced that one with the new one, so I didn't look at it until now.

Are you worried about getting credit?

(In reply to comment #6)
> Note: both patches work fine on Lion, but I haven't tested on older OSes.

I've tested the first one on Lion and Snow Leopard. I'll test on Tiger and Leopard later, but there's no reason that it shouldn't work... and running init on an existing object is probably naughty anyway.

 (In reply to comment #7)
> > Not sure what you mean, checking the object responds to the selector is a
> > perfectly backwards-compatible fix, and more correct.
> 
> On the theory that if it doesn't respond to the selector the old way is OK? I
> guess, since Apple isn't going to go back and fix whatever they thought was
> broken.

Nope, the selector check is better because using setStyleMask is more correct on the OSes that support it - I'm not convinced we aren't missing copying some properties from the old window to the new window - and it's less wasteful.

> (In reply to comment #5)
> > Also, my first patch implements the fix the way you other report describes.
> 
> But you replaced that one with the new one, so I didn't look at it until now.
> 
> Are you worried about getting credit?

I'm usually not too bothered by these things but I did just spend about 8 hours getting a full GTK+ source build on Mac set up just so that I could test my fix :)

> (In reply to comment #6)
> > Note: both patches work fine on Lion, but I haven't tested on older OSes.
> 
> I've tested the first one on Lion and Snow Leopard. I'll test on Tiger and
> Leopard later, but there's no reason that it shouldn't work... and running init
> on an existing object is probably naughty anyway.

Yeah, the double-init is evil but since I couldn't test older OSes I didn't want to change the behaviour there.

Probably the "best" solution is to combine both patches :)

(In reply to comment #8)
>  (In reply to comment #7)
> > > Not sure what you mean, checking the object responds to the selector is a
> > > perfectly backwards-compatible fix, and more correct.
> > 
> > On the theory that if it doesn't respond to the selector the old way is OK? I
> > guess, since Apple isn't going to go back and fix whatever they thought was
> > broken.
> 
> Nope, the selector check is better because using setStyleMask is more correct
> on the OSes that support it - I'm not convinced we aren't missing copying some
> properties from the old window to the new window - and it's less wasteful.
> 
> > (In reply to comment #5)
> > > Also, my first patch implements the fix the way you other report describes.
> > 
> > But you replaced that one with the new one, so I didn't look at it until now.
> > 
> > Are you worried about getting credit?
> 
> I'm usually not too bothered by these things but I did just spend about 8 hours
> getting a full GTK+ source build on Mac set up just so that I could test my fix
> :)

You should have used http://gtk-osx.sourceforge.net . It would have taken you less than an hour, and with no pain. ;-)

> 
> > (In reply to comment #6)
> > > Note: both patches work fine on Lion, but I haven't tested on older OSes.
> > 
> > I've tested the first one on Lion and Snow Leopard. I'll test on Tiger and
> > Leopard later, but there's no reason that it shouldn't work... and running init
> > on an existing object is probably naughty anyway.
> 
> Yeah, the double-init is evil but since I couldn't test older OSes I didn't
> want to change the behaviour there.
> 
> Probably the "best" solution is to combine both patches :)

You're probably right. I'll get that worked up on 655078 for both Gtk-2.24 and master. They'll be checked in on gtk-2.24-quartz and quartz-integration soon (I'll have to knock off for a social engagement shortly, so perhaps not tonight); it will have to wait for Kris to decide what he wants to do for master.

Created attachment 192427 [details] [review]
Combined patch

I've done it already :)

(In reply to comment #9)
> You should have used http://gtk-osx.sourceforge.net . It would have taken you
> less than an hour, and with no pain. ;-)

Unfortunately I needed to build a GTK+ with Mono's patchset and gtk-sharp, so I ended up using Banshee's bockbuild.

Thanks.

*** Bug 655078 has been marked as a duplicate of this bug. ***

(In reply to comment #10)
> Created an attachment (id=192427) [details] [review]
> Combined patch
> 
> I've done it already :)

I will put this on my review list.  I don't really understand why a double init does not work on Lion, maybe because of the new ref counting thing?

(Note: I will not have access to Lion in the near future unfortunately).

(In reply to comment #14)
> I will put this on my review list.  I don't really understand why a double init
> does not work on Lion, maybe because of the new ref counting thing?

See the trace in bug 655078 - an internal method called by the initializer is asserting that an internal field is not null.

Kris,

If you're not able to test, perhaps you should just check it for style and I can push it when you're happy with that. This is a blocker for a lot of programs, so we need to get it out ASAP.

I'll have it in both the quartz branches tonight, but there are plenty of folk who don't use those.

FWIW, we'll be checking this out with Ardour/Mixbus today as well. Mike & John - thanks for the work on this.

No idea if that helps, but just wanted to report that I applied that patch and confirm it is working.
Thanks @Michael Hutchinson for this! And great to see that this community is reactive. :)

(In reply to comment #10)
> Created an attachment (id=192427) [details] [review]
> Combined patch
> 
> I've done it already :)

I've committed this to GTK+ 3 master (I will push shortly) with two changes:

 - Use GdkQuartzNSWindow.
 - Add guards so that the setStyleMask is not exposed to the compiler < 10.6 to avoid spewing warnings.


John: feel free to merge this into the gtk-2-24 branch, I don't have it checked out at the moment ...

Done.

(In reply to comment #19)

>  - Add guards so that the setStyleMask is not exposed to the compiler < 10.6 to
> avoid spewing warnings.
> 

Sorry I'm a bit late on this, I didn't really think it through until I had to reconcile it with quartz-integration.

The guards miss the whole point of the runtime check. Micheal's intent was that regardless of what SDK it's built on, and regardless of what mac_osx_version_min is, setStyleMask will be used if it's available rather than having to create a new object. 

Yes, it produces a compile-time warning because the compiler isn't smart enough to recognize the run-time check. If you really insist on squashing that, get rid of the (very expensive) ifRespondsToSelector call.

(In reply to comment #21)
> (In reply to comment #19)
> 
> >  - Add guards so that the setStyleMask is not exposed to the compiler < 10.6 to
> > avoid spewing warnings.
> > 
> 
> Sorry I'm a bit late on this, I didn't really think it through until I had to
> reconcile it with quartz-integration.
> 
> The guards miss the whole point of the runtime check. Micheal's intent was that
> regardless of what SDK it's built on, and regardless of what
> mac_osx_version_min is, setStyleMask will be used if it's available rather than
> having to create a new object. 

Hrm, my intent was to not have the compiler warning on older systems and if you compile on a newer system it would produce a backwards compatible binary.

But thinking of it more, if you compile on a newer system, you would likely compile it under the 10.4 SDK, right?  Does that set version_min to 10.4 so that the code breaks again?  (I guess so actually ...)

No, it's worse than that.
There are two environment variables at play here, MAC_OS_X_VERION_MIN_ALLOWED and MAC_OS_X_VERSION_MAX_AVAILABLE. Either can be explicitly set in the environment, but usually neither is explicitly. In that case, MAX_AVAILABLE is set to the SDK used for the build and MIN_ALLOWED is set to the higher of $MACOSX_DEPLOYMENT_TARGET or the -mmacosx_version_min compiler flag. If neither is set, it is also set to the SDK version.

http://developer.apple.com/library/mac/#technotes/tn2064/_index.html has the gory details.

What I've been doing for some time is building against the lowest-targeted SDK, which because WebKitGTK won't build against 10.4 has been 10.5. But Lion's XCode 4.1 doesn't support SDKs before 10.6, so I (and every other Mac Gtk maintainer who wants to move to Lion) will have to start doing what Apple says they want us to do in that technote: Use SDK 10.6 or 10.7 and set -mmacosx_version_min (or $MACOSX_DEPLOYMENT_TARGET) to 10.4 or 10.5. 

The thing about Objective-C is that it can link method calls at runtime (that's the weak linking they talk about in the tech note), so even if a method isn't in the header when you compile you can still use it if you check beforehand with respondsToSelector, which is what Michael does here, so even when you compile his version of the patch against SDK10.5, it will produce the compiler warning because the method isn't in the 10.5 header -- but it's weak linked anyway, so when it's run on 10.6 or 10.7, it will find the method in the framework in /System/Library/Frameworks and execute it.

(In reply to comment #23)
> No, it's worse than that.
> There are two environment variables at play here, MAC_OS_X_VERION_MIN_ALLOWED
> and MAC_OS_X_VERSION_MAX_AVAILABLE. Either can be explicitly set in the
> environment, but usually neither is explicitly. In that case, MAX_AVAILABLE is
> set to the SDK used for the build and MIN_ALLOWED is set to the higher of
> $MACOSX_DEPLOYMENT_TARGET or the -mmacosx_version_min compiler flag. If neither
> is set, it is also set to the SDK version.
> 
> http://developer.apple.com/library/mac/#technotes/tn2064/_index.html has the
> gory details.
> 
> What I've been doing for some time is building against the lowest-targeted SDK,
> which because WebKitGTK won't build against 10.4 has been 10.5. But Lion's
> XCode 4.1 doesn't support SDKs before 10.6, so I (and every other Mac Gtk
> maintainer who wants to move to Lion) will have to start doing what Apple says
> they want us to do in that technote: Use SDK 10.6 or 10.7 and set
> -mmacosx_version_min (or $MACOSX_DEPLOYMENT_TARGET) to 10.4 or 10.5. 

I see.


> The thing about Objective-C is that it can link method calls at runtime (that's
> the weak linking they talk about in the tech note), so even if a method isn't
> in the header when you compile you can still use it if you check beforehand
> with respondsToSelector, which is what Michael does here, so even when you
> compile his version of the patch against SDK10.5, it will produce the compiler
> warning because the method isn't in the 10.5 header -- but it's weak linked
> anyway, so when it's run on 10.6 or 10.7, it will find the method in the
> framework in /System/Library/Frameworks and execute it.

Yea, I understood that.   I only really tried to get rid of the compiler warning, because in GTK+ we try hard to have the code compile without warnings.  (I know there's currently some other warnings related to event masks in gdk/quartz/, I have a patch on my other machine that fixes this).  I forgot to think of the case when you compile for an older SDK on a newer SDK machine.

I will remove the #ifdef I added and have to learn to live with the compile warning on Tiger for now ;)  I tried to look for a way to suppress the compile warning on older platforms (perhaps a compiler flag), but it doesn't seem easily possible and it is probably not worth the effort.

There's a rather involved way to get rid of the compile warning:
--- a/gdk/quartz/gdkwindow-quartz.c
+++ b/gdk/quartz/gdkwindow-quartz.c
@@ -2589,7 +2589,14 @@ gdk_quartz_window_set_decorations (GdkWindow       *window,
        */
       if ([impl->toplevel respondsToSelector:@selector(setStyleMask:)])
         {
-          [impl->toplevel setStyleMask:new_mask];
+	  NSMethodSignature *sig =
+	    [impl->toplevel methodSignatureForSelector:@selector(setStyleMask)];
+	  NSInvocation *method =
+	    [NSInvocation invocationWithMethodSignature:sig];
+	  gpointer mask_ptr = &new_mask;
+	  [method setSelector:@selector(setStyleMask:)];
+	  [method setArgument:mask_ptr atIndex:2];
+	  [method invokeWithTarget:impl->toplevel];
         }
       else
         {

(It would be easier if setStyleMask took a pointer to something instead of an int: [impl->toplevel performSelector:@selector(setStyleMask) withObject:foo] instead of all the MethodSignature and Invocation stuff, but it doesn't.)

Anyway, if you think it worthwhile, I can commit it.

(In reply to comment #25) 
> (It would be easier if setStyleMask took a pointer to something instead of an
> int: [impl->toplevel performSelector:@selector(setStyleMask) withObject:foo]
> instead of all the MethodSignature and Invocation stuff, but it doesn't.)
> 
> Anyway, if you think it worthwhile, I can commit it.

I thought about an invocation like this, but it doesn't really help readability as you would likely agree with me ;)  

I propose we leave things as they are currently, if people really start complaining about the compile warnings, we can always introduce something like this.

Yup, it's ugly, and it would have to be heavily commented to explain what's going on and why.

I doubt that anyone will complain. It's not like it's the only warning.

Hey guys, I know you've closed this bug, but I just want to mention that you can get rid of that warning by doing something similar to what I proposed in bug #516725. Basically, define a protocol that has the signature of the setStyleMask method and then cast impl->toplevel to an id that implements that. Something like:

@protocol CanSetStyleMask
- (void)setStyleMask:(int)mask;
@end

if ([impl->toplevel respondsToSelector:@selector(setStyleMask:)])
{
    [(id<CanSetStyleMask>)impl->toplevel setStyleMask:new_mask];

...

So it does. I'd forgotten about that trick. Committed to gtk-2-24, gtk-3-2, and master. Thanks!

*** Bug 657663 has been marked as a duplicate of this bug. ***