Hi Phil,
please see updated the webrev here
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/08/
and comments inline
On 18.02.2016 0:10, Phil Race wrote:
Desktop.java looks fine now. Moving on to the other API classes
Top-level question :
Why are AppEvent and TaskBar in java.awt and not java.awt.desktop ?
Agreed on AppEvent, but Taskbar is placed in java.awt on purpose to
make it adjacent with Desktop and SystemTray.
And AppEvent seems to use a lot of nested classes.
If they were in java.awt.desktop then they could be top-level classes
without creating too much clutter. I don't think it matters that 'this
is how the com.apple APIs were done' - people need to recode anyway
AppEvent.java :
more wildcards ..
28 import java.awt.desktop.*;
I notice the docs in this file (and perhaps more I have yet to see)
reference types without @code :-
eg :
* Constructs a FilesEvent
* Constructs an OpenFilesEvent
..
* Get the URI the app was asked to open
146 * @return the URI
and more. Not essential I suppose but preferable.
69 public List<File> getFiles() {
70 return files;
71 }
Is there any need to return a copy for any of these ?
95 * Gets the search term. The platform may additionally provide
the search
96 * term that was used to find the files. This is for example the
case
97 * on Mac OS X, when the files were opened using the Spotlight
search
98 * menu or a Finder search window.
99 *
100 * This is useful for highlighting the search term in the
documents when
101 * they are opened.
102 * @return the search term used to find the files
Could we say "optionally" instead of "additionally"
and make clear that it may be "null"
a very long line :-
209 * Event sent when the application has become the foreground app, and
when it has resigned being the foreground app.
And "resigned" is a voluntary term. If something else is placed into
the foreground the
this app may not get asked if it wants to allow that.
Maybe instead say "and when it is no longer the foreground app."
Done.
Also is there anything we can (or should) say about how some of
these events interact ?
eg, if a foreground app is hidden does only the appHidden listener
get called ?
Both listeners could be called, it's already mentioned here:
* Called when the hidden app is shown again (but not
necessarily brought to
* the foreground).
*
* @param e event
*/
public void appUnhidden(final AppHiddenEvent e);
BTW the term 'unhidden' makes sense if the app was previously
'hidden' but less so
if the app is being newly shown.
/**
252 * The system does not provide a reason for a session change.
253 */
254 @Native
255 public static final int REASON_UNSPECIFIED = 0x0;
256
I think we should use an enum here.
Also unless there is a reason I have not yet come across, as many as
possible
of these classes should be marked 'final'
Taskbar.java :
One meta-concern I have here is that it seems to have a very blurry
distinction
with existing API :
https://docs.oracle.com/javase/8/docs/api/java/awt/SystemTray.html
https://docs.oracle.com/javase/8/docs/api/java/awt/TrayIcon.html
Aren't both of these APIs dealing with the same area ?
Should we really add this new class or tweak system tray as needed ?
On Windows they dealing in the same area with taskbar, but on Mac OS
SystemTray and Taskbar(Dock) are separated. So I prefer to left it as
it is.
/**
112 * Stops displaying the progress.
113 */
114 @Native
115 public static final int STATE_OFF = 0x0;
Another case where an enum could be used.
sun.awt.AppContext context = sun.awt.AppContext.getAppContext();
these days this sometimes can return null ..
There are many usages of getAppContext() without null check all over
our code, so I think that this should be fixed under a separate bug.
AppForegroundEvent :
* @param e the app became foreground notification.
I am not entirely sure what that means or even what it
should say, perhaps because AppForegroundEvent does not
have any fields.
How about
"@param e an event indicating that the receiving application is now the foreground
app"
30 /**
31 * Implementors are notified when the app is hidden or shown by the user.
32 * This notification is helpful for discontinuing a costly animation if
it's not visible to the user.
33 */
34 public interface AppHiddenListener extends SystemEventListener {
.. we already can get notification when a window is hidden.
Can I assume that this event indicates *all* windows are hidden and
each hidden window still gets its hidden event ? And that there is
no guarantee of order of delivery ?
It is a Mac specific hide, Cmd+H, or right click in dock and select
hide. All windows are hidden,
but in this case windows may not receive such notifications and only
gets AppHiddenEvent.
/**
31 * Implementors receive notification when the app has been asked to open
again.
32 *
33 * This notification is useful for showing a new document when your app
has no open windows.
34 */
35 public interface AppReopenedListener extends SystemEventListener {
So .. this means something more than that the windows have been hiddent ?
Does it mean that all windows have been disposed() but the app is still running
?
Is this the case when the system menu bar can be used be the user to request
a new window ? Can you please give an example.
It is another Mac specific feature, it does exactly how it described.
You can have no windows(not even created), no system menu bar and
some running thread, AppReopenedListener
will be called on click in dock icon or on double click on the
application executable.
-phil.
On 02/16/2016 07:04 AM, Alexander Zvegintsev wrote:
Hi Phil,
thanks for the review, please see updated webrev here:
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/07/
--
Thanks,
Alexander.
On 02/15/2016 09:58 PM, Phil Race wrote:
Desktop.java : 28 import java.awt.desktop.*; Can we replace this
wild card import with an enumeration of the imported classes. I am
sure you need all of them but it is still recommended practice.
Also I do not see where you are adding the new package to the ones
for which javadoc is generated. This would be an edit to
$TOPLEVEL/common/CORE_PKGS.gmk I am focusing first on the API part
of this so you can finalize the CCC.
> 43 * The {@code Desktop} class allows interact with various desktop
capabilities.
interact -> interaction
Or better
The {@code Desktop} class allows interaction with various platform
desktop capabilities.
> * @since 1.9 These should all be @since 9 < 130 * Returns the {@code Desktop}
instance of the current > 268 * Returns the <code>Desktop</code>
instance of the current
I am surprised to see this changed .. we are making the opposite update
elsewhere.
All the cases you have changed need to be changed back.
>641 * Adds sub-types of {@link SystemEventListener} to listen for
notifications Does that link work ? The referenced class is in a
different package. I am fairly sure that in all such cases you
need to fully qualify the class using the package. I believe that
javac should run doclint as part of the JDK build so you should
examine the output/logs.
686 * @param aboutHandler the handler to respond to the
687 * @throws UnsupportedOperationException if the current
platform seems like line 686 is incomplete. Same for line 704 :
704 * @param preferencesHandler the handler to respond to the
801
802 /**
803 * Sets the default strategy used to quit this application. The
default is
804 * calling SYSTEM_EXIT_0.
805 *
806 * @param strategy the way this application should be shutdown
807 * @throws UnsupportedOperationException if the current platform
808 * does not support the {@link
Desktop.Action#APP_QUIT_STRATEGY} action
809 * @see QuitStrategy
810 * @since 1.9
811 */
812 public void setQuitStrategy(final QuitStrategy strategy) {
813 checkActionSupport(Action.APP_QUIT_STRATEGY);
814 peer.setQuitStrategy(strategy);
Some of the actions discuss permissions and others do not.
Shouldn't this say something about permissions ? You do need permission to
exit the VM, so should you be able to set a QuitStrategy if you don't have
such permission ??
More later ..
-phil.
On 01/21/2016 11:16 PM, Alexander Zvegintsev wrote:
Hi Semyon,
I am not sure about flexibility, but providing additional info
for a user session change could be useful
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/05/
(changed awt_Toolkit.cpp, AppEvent.java,
UserSessionListener.java, _AppEventHandler.java)
--
Thanks,
Alexander.
On 01/19/2016 11:52 AM, Semyon Sadetsky wrote:
Hi Alexander,
What do you think about accompanying the application events with
a flag containing the extended information that may be provided
by various native platforms? For example, the session events may
provide a reason on the Windows platform.
Also this may add flexibility that allows to support new
features in the future.
--Semyon
On 1/17/2016 11:55 PM, Alexander Zvegintsev wrote:
Hi Semyon,
Is it possible to use WM_QUERYENDSESSION for
Action.APP_SUDDEN_TERMINATION?
Sure, so please see updated webrev:
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/04/
awt_Toolkit.cpp:
extended list of supported messages for user session listener.
awt_Desktop.cpp, awt_Toolkit.cpp, WDesktopPeer.java:
added SuddenTerminaton support for Windows
--
Thanks,
Alexander.
On 01/15/2016 04:14 PM, Semyon Sadetsky wrote:
Hi,
On 1/15/2016 12:39 PM, Alexander Zvegintsev wrote:
Hi Semyon,
Yes, it is just LOCK/UNLOCK for now, because it is the most
common scenario of desktop usage.
Do you mean that we should consider remote desktop logins
too? Something like:
if (wParam == WTS_CONSOLE_CONNECT
|| wParam == WTS_CONSOLE_DISCONNECT
|| wParam == WTS_REMOTE_CONNECT
|| wParam == WTS_REMOTE_DISCONNECT
|| wParam == WTS_SESSION_UNLOCK
|| wParam == WTS_SESSION_LOCK) {
BOOL activate = wParam == WTS_CONSOLE_CONNECT
|| wParam == WTS_REMOTE_CONNECT
|| wParam == WTS_SESSION_UNLOCK;
env->CallStaticVoidMethod(clzz, AwtToolkit::userSessionMID,
activate
? JNI_TRUE
: JNI_FALSE);
}
That's seems better to me.
Or if you refer to WTS_SESSION_LOGOFF, then it seems useless
for us,
AFAIK only services will receive this notification, and
anyway all apps the user was running are already killed by
this time.
Is it possible to use WM_QUERYENDSESSION for
Action.APP_SUDDEN_TERMINATION?
--Semyon
--
Thanks,
Alexander.
On 12.01.2016 19:21, Semyon Sadetsky wrote:
Hi Alexander,
awt_Toolkit.cpp:
case WM_WTSSESSION_CHANGE: {
jclass clzz =
env->FindClass("sun/awt/windows/WDesktopPeer");
DASSERT(clzz != NULL);
if (!clzz) throw std::bad_alloc();
if (wParam == WTS_SESSION_LOCK || wParam ==
WTS_SESSION_UNLOCK) {
env->CallStaticVoidMethod(clzz, AwtToolkit::userSessionMID,
wParam == WTS_SESSION_UNLOCK
? JNI_TRUE
: JNI_FALSE);
}
break;
}
So only the WTS_SESSION_UNLOCK is propagated to java as a
session act/deact and other messages just ignored?
Other messages:
#define WTS_CONSOLE_CONNECT 0x1
#define WTS_CONSOLE_DISCONNECT 0x2
#define WTS_REMOTE_CONNECT 0x3
#define WTS_REMOTE_DISCONNECT 0x4
#define WTS_SESSION_LOGON 0x5
#define WTS_SESSION_LOGOFF 0x6
#define WTS_SESSION_LOCK 0x7
#define WTS_SESSION_REMOTE_CONTROL
some of them seems to me more suitable to be chosen as the
session act/deact event. Could you comment that for me?
--Semyon
On 11/24/2015 6:02 PM, Alexander Zvegintsev wrote:
Please review the updated fix:
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/03/
removed fullscreen related code (moved to JDK-8143914 [0])
fix serialization in AppEvent
[0] https://bugs.openjdk.java.net/browse/JDK-8143914
Thanks,
Alexander.
On 11/21/2015 03:33 AM, Alexander Zvegintsev wrote:
Hi Phil,
Can someone explain why this is needed given the existing
support of
GraphicsDevice.setFullScreenWindow(Window) ?
GraphicsDevice.setFullScreenWindow is used for an
exclusive full screen mode.
Mac OS has another option to create a virtual desktop with
provided window in it.
You can switch between them by three-finger horizontal swipe.
Why does it have to be a RootPaneContainer ? Why is this
tied to Swing ?
This appears to narrow it to JDialog and JWindow.
It reuses swing code to set native windows style bits.
Please see updated webrev:
http://cr.openjdk.java.net/~azvegint/jdk/9/8143227/02/
updated permission
added missing @throws @since and @implNote
browseFileDirectory is now return void
RootPaneContainer -> JDialog and JWindow
--
Thanks,
Alexander.
On 11/20/2015 09:03 PM, Phil Race wrote:
On 11/20/2015 09:12 AM, Sergey Bylokhov wrote:
I am worried about setWindowCanFullScreen and
requestToggleFullScreen.
On the latest osx this functionality was merged with
maximize button. So probably it will be better to change
behavior of window.setExtendedState() + MAXIMIZED_BOTH?
Can someone explain why this is needed given the existing
support of
GraphicsDevice.setFullScreenWindow(Window) ?
And what happens if you use *both* ? They still need to
play well together
if there is some reason the new one is needed.
Other comments :
> * Note, Aqua Look and Feel should be active to
support this on Mac OS.
Needs @implNote
There seems to be lots of missing SecurityException tags
given all the checkAWTPermission() calls.
is checkAWTPermission() really the right call for all of
these actions ?
Does it "cover" being able to delete files and quit the
app ? I am not sure it is
correct in all cases.
And also there are missing @since tags.
Opens a folder containing the {@code file} in a default
system file manager.
933 * @param file the file
934 * @return returns true if successfully opened
935 * @throws NullPointerException if {@code file}
is {@code null}
936 * @throws IllegalArgumentException if the
specified file doesn't
937 * exist
938 */
939 public boolean browseFileDirectory(File file) {
So what happens if there is no "support" for this ?
Exception or "false" ?
Are you comfortable that all these APIs that return
"true" if successful are
implementable on all platforms. i.e I mean that does the
platform return
a value you can pass on as success/failure.
---
861 * Attaches a {@link FullScreenListener} to the
specified top-level
862 * {@link Window}.
863 *
864 * @param window to attach the {@link
FullScreenListener} to
865 * @param listener to be notified when a full
screen event occurs
866 * @throws IllegalArgumentException if window is
not a
867 * {@link javax.swing.RootPaneContainer}
868 */
869 public void addWindowFullScreenListener(final
Window window,
870 final FullScreenListener listener) {
-------
Why does it have to be a RootPaneContainer ? Why is this
tied to Swing ?
This appears to narrow it to JDialog and JWindow.
-phil.
