<AWT Dev> Review-request for 8143227: Platform-Specific Desktop Features
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Wed Feb 24 17:12:02 UTC 2016
Small question about moveToTrash(). Should we describe that the user can
delete folders as well? Also I am not sure about permissions which we
check. For example the usual File.delete() calls "SM.checkDelete()" but
we check SM.checkWrite() for "<<ALL FILES>>". Moreover
checkFileValidation() checks the read permission for the file as well.
At the end it is an interesting question: what permissions are needed to
delete the file.
On 21.02.16 14:38, Alexander Zvegintsev wrote:
> 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.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--
Best regards, Sergey.
More information about the awt-dev
mailing list