<AWT Dev> Review-request for 8143227: Platform-Specific Desktop Features
Alexander Zvegintsev
alexander.zvegintsev at oracle.com
Tue Feb 16 15:04:39 UTC 2016
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/20160216/2af3481a/attachment-0001.html>
More information about the awt-dev
mailing list