On Mar 1, 2012, at 1:10 PM, Anthony Petrov wrote:
Hi Mike,
Thanks for the review! Please see my comments inline.
On 3/1/2012 9:31 PM, Mike Swingler wrote:
On Feb 29, 2012, at 6:58 AM, Anthony Petrov wrote:
http://cr.openjdk.java.net/~anthony/x-21-popupInFullscreen-7145818.0/
A few points to consider:
* To protect against the unrecognized selector problem, you should test if the
AWTWindow object -respondsToSelector:@selector(toggleFullScreen) before just
calling it.
I'm aware of -respondsToSelector. But I suppose that in that case this simply
won't work on 10.6.8 at all. Since
a) presently it does work on 10.6.8 for reasons unknown, and
If OpenJDK is built on Snow Leopard, it works fine. I believe the problem is
the X11/FreeType versions in Lion are newer, and DYLD won't load libraries
linked against older versions. But that is going to start me on my rant about
how OpenJDK should bundle it's own FreeType...
b) we officially support 10.7+ only, hence the check makes little sense in
theory, and
Please think of OpenJDK, not just Oracle's proprietary binaries. There are
other users who currently compile on Snow Leopard and this is not an
inconvenience, since 10.7-only API is very rare in the JDK.
c) from ObjC perspective sending an unknown selector isn't an error, but just a
warning,
It is extremely poor form. The -respondsToSelector: check is very cheap, and is
very obvious what it is guarding against.
it seems to me that having this warning printed out to the console (which isn't
seen by 99% of users anyway) is OK when running on 10.6.8. Plus we get the full
screen working there. Isn't it awesome?
We know there are developers and users who will be running OpenJDK on Mac OS X
10.6.8, so it is perfectly reasonable to add this as an OS guard. We should not
actively damage our ability to run on 10.6 if it's trivially avoidable.
* Also, there is already API that calls -toggleFullScreen in the eAWT classes
that you might not be aware of. You should probably test for interactions with
that, since apps can opt their window into having a full screen widget icon and
independently toggle fullscreen.
Thanks for pointing this out. I'll rework this code to make sure calls from
EAWT update the boolean flag.
Great.
* In some cases, seeing the menubar is actually desirable, where as in the
"exclusive" mode, it's probably not. Perhaps you could consult a client
property on the window to determine if the menu bar should be hidden?
Excellent idea! I think that by default the menu should be hidden (for Java
spec's sake). But if a client property is set, then the menu would be visible.
Cool. There are several client property readers already on the root AWT window
that should be easily extendable.
I like this overall solution, because it uses the native platform concept of
full screen which doesn't trap the user from switching spaces like the Java SE
6 implementation did.
We've noticed an interesting detail with -toggleFullScreen when it's used in a
multi-screen environment. In that case the window will go full screen on the
biggest monitor (actually we have a MacBook with an external monitor
connected.) The OS seems to ignore the screen where the window were before
entering the FS mode (the main notebook display). Is this OK? Can we force it
to use the same screen where the window is originally located for the FS mode?
It's actually the monitor with the menu bar (the primary, as designated in the
Monitor's preference pane).
This is an issue with the OS, and should be filed at
<http://bugreporter.apple.com> (it's known, but if you have a specific API
suggestion to target a screen, or some sort of automatic behavior in mind, it would
be good to provide specific suggestions in the bug).
Thanks,
Mike Swingler
Apple Inc.
