<AWT Dev> Review-request for 8143227: Platform-Specific Desktop Features
Alexander Zvegintsev
alexander.zvegintsev at oracle.com
Sun Feb 21 11:38:22 UTC 2016
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.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20160221/d5f14d39/attachment-0001.html>
More information about the awt-dev
mailing list