Could I get a review for the latest version of the fix please? Here's the link for your convenience:

http://cr.openjdk.java.net/~anthony/x-21-popupInFullscreen-7145818.3/

--
best regards,
Anthony

On 3/5/2012 8:03 PM, Anthony Petrov wrote:
Hi Mike,

A few points:

* When a user enters the full screen using the OS widget, the full screen state isn't sync'd with Java's GraphicsDevice.getFullScreenWindow().

* Rather than calling native, I'd actually update the flag upon receiving native notifications about entering/leaving the FS mode.

* If you call -toggleFullScreen: for a window w/o the NSWindowCollectionBehaviorFullScreenPrimary collection behavior, then the menus and the dock stay visible. Apart from that, there's no the default 10.7-specific animation for such windows when entering the FS mode.

* If I mark an undecorated window as fullscreenable, its original size won't be restored when exiting the FS mode. So we can't simply enable the behavior automatically for all windows. An app must opt in.

* Apple JDK 1.6 won't let a user exit a FS Java application, too. So at least we don't introduce a regression here. Also, the full screen is a restricted functionality that's unavailable to unsigned code by default. Thus we generally trust code that's able to enter the mode (in that it should let user out when needed).

* The fix we're currently reviewing is supposed to fix a bug with popup windows not showing up for a Java app that doesn't use EAWT at all. As far as I've tested it, it does resolve the issue. And the behavior seems to be quite consistent with what Apple JDK 1.6 does.

* The only thing the fix worsens is for EAWT-aware app: it hides the menu bar unconditionally when entering the FS mode. I've just fixed that by avoiding hiding the menu bar if a window is marked as FULLSCREENABLE. I believe that a new fix at

http://cr.openjdk.java.net/~anthony/x-21-popupInFullscreen-7145818.3/

is what we need in 7u4. We may file a new change request to address EAWT-specific issues in a future release.

Please review.

--
best regards,
Anthony

On 03/02/12 21:13, Mike Swingler wrote:
There are a few points here:
* If the user marks the window as "fullscreenable" using the eAWT API,
that puts a widget in the upper right corner of the window which allows
the user to toggle it into full screen themselves.
* When in full screen (using AWT or eAWT API), the user should be able
to click the blue exit fullscreen button in the menu bar at any time (if
they entered into full screen themselves)
* Generally, it is undesirable for the user to be stuck in a state where
they cannot return to the desktop.

With the current diff, the isFullScreenMode boolean in Java will
definitely be out of sync if the user initiates either of these actions.

Instead of trying to keep the knowledge of the full screen state in Java
behind a lock, I think you probably need query native to see if the
window is in the fullscreen state. I've used the following testcase to
test the existing eAWT fullscreen API, and I think you'll find it's easy
to get into a state where the user is stuck in windowed mode without a
menu bar, and that sometimes the user is locked out from returning to
windowed mode when they clicked on the window widget.

After trying out your diff, I actually found it quite frustrating that
the menubar was suppressed and I *had* to use the four-finger swipe
gesture to get back to my desktop. How would you feel about just
dropping the whole menubar hiding stuff? Since the menu bar is
auto-hidden by default, users don't get to it unless they push up to the
top of the screen anyway.

Try out this test case, and be sure to use the window widget, as well as
the button that programmatically fires the eAWT API.

import java.awt.*;
import java.awt.event.ActionEvent;
import javax.swing.*;

import com.apple.eawt.AppEvent.FullScreenEvent;
import com.apple.eawt.*;

public class FullScreenTest {
public static void main(String args[]) {
SwingUtilities.invokeLater(new Runnable() {
public void run() { createFrame(); }
});
}


static void createFrame() {
JFrame frame = new JFrame();
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setTitle(System.getProperty("java.version"));


Container contentPane = frame.getContentPane();
contentPane.setLayout(new BorderLayout());
contentPane.add(createPanel(), BorderLayout.CENTER);


// turns on the fullscreen window titlebar widget in the upper right corner
FullScreenUtilities.setWindowCanFullScreen(frame, true);


// useful for re-adjusting content or hiding/showing palette windows
FullScreenUtilities.addFullScreenListenerTo(frame, new FullScreenAdapter() {
public void windowExitingFullScreen(FullScreenEvent e) {
System.out.println("exiting");
}
public void windowExitedFullScreen(FullScreenEvent e) {
System.out.println("exited");
}
public void windowEnteringFullScreen(FullScreenEvent e) {
System.out.println("entering");
}
public void windowEnteredFullScreen(FullScreenEvent e) {
System.out.println("entered");
}
});


frame.pack();
frame.setVisible(true);
}

static Component createPanel() {
final JPanel panel = new JPanel(new FlowLayout());


// toggle FullScreen from a toolbar button
panel.add(new JButton(new AbstractAction("Full Screen Me!") {
public void actionPerformed(ActionEvent e) {
Application.getApplication().requestToggleFullScreen(
(Window)panel.getTopLevelAncestor());
}
}));


return panel;
}
}

Regards,
Mike Swingler
Apple Inc.

On Mar 2, 2012, at 6:42 AM, Anthony Petrov wrote:

A tiny typo has been fixed, a new webrev is at:

http://cr.openjdk.java.net/~anthony/x-21-popupInFullscreen-7145818.2/

--
best regards,
Anthony

On 3/2/2012 6:20 PM, Anthony Petrov wrote:
Thanks for your suggestions Mike. Here's the latest version of the fix:
http://cr.openjdk.java.net/~anthony/x-21-popupInFullscreen-7145818.1/
--
best regards,
Anthony
On 3/2/2012 3:14 AM, Mike Swingler wrote:
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).

Reply via email to