<AWT Dev> [10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Kevin Rushforth]

The latest webrev, version 06, looks good to me now.

Approved (although I am not a "R"eviewer for AWT).

-- Kevin


Alexander Zvegintsev wrote:
>
> HI Semyon,
>
> Please see my answer to Dmitry
>
>> Hi Dmitry,
>>
>> From my understanding JavaFX stage can't be easily integrated in JDK to support orderWindow() approach,
>>
>> addChildWindow() is a native(and the simplest) way to maintain one window above other one, should be called only once.
>>
>> IIUC the main concert of JDK-8080729 that child windows jumping to parent's display upon focus receiving, this is not an issue with current fix,
>>
>> because addChildWindow() will be called only upon dialog creation in case of JavaFX-Swing interop.
>>
>> Jump may happen if user want to create a child swing dialog on display other than JavaFX stage's one,
>>
>> but such rare scenario can be easily workarounded on a user side by calling setLocation() right after setVisible() call.
>>
>> So I would prefer to use addChildWindow() to make this fix as simple as possible.
>
> Thanks,
> Alexander.
> On 08/11/2017 02:45, Semyon Sadetsky wrote:
>>
>> Hi Alexander,
>>
>> In CPlatformWindow.java change you used addChildWindow(), but we get 
>> rig of addChildWindow() in 8080729 and start to manage windows order 
>> on java side. Can you test that this change doesn't cause any 
>> ordering issues with modal and non-modal child and sibling windows on 
>> mac?
>>
>> --Semyon
>>
>> On 11/07/2017 10:11 AM, Alexander Zvegintsev wrote:
>>>
>>> Hi Sergey,
>>>
>>> I am not able to crash it on several platforms, except one case:
>>>
>>> if we are terminating JavaFX application while EDT processing some 
>>> long task. But it is unrelated to the fix and reproducible on 
>>> current builds.
>>>
>>> I've filed a separate bug JDK-8190329 
>>> <https://bugs.openjdk.java.net/browse/JDK-8190329>.
>>>
>>>> Will the current fix cover the native dialogs like 
>>>> FileDialog/PrintDialog on linux and windows? 
>>> It will not, Windows already fixed by JDK-8088395 
>>> <https://bugs.openjdk.java.net/browse/JDK-8088395>
>>>
>>>
>>> Test added:
>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/
>>> Thanks,
>>> Alexander.
>>> On 13/10/2017 01:14, Sergey Bylokhov wrote:
>>>> Looks fine.
>>>> I am not sure but it looks like the fix has an assumption that the 
>>>> CPlatformWindow.setVisible() code will be executed on EDT/Appkit 
>>>> but it is not the case. This code can be executed on any 
>>>> thread(intentionally for crash), and it will be good to check that 
>>>> it works as expected by some stress test which will try to force 
>>>> the possible crash: 4 threads:
>>>>  - show/dispose Swing Node
>>>>  - show/dispose Dialog1/2/3 using different timeouts
>>>>
>>>> Will the current fix cover the native dialogs like 
>>>> FileDialog/PrintDialog on linux and windows?
>>>>
>>>> On 10/10/2017 13:54, Kevin Rushforth wrote:
>>>>> Note that there is now a 04 version.
>>>>>
>>>>> It looks good to me, although someone more familiar with AWT 
>>>>> should also check the AWT changes.
>>>>>
>>>>> We will need a test program for this (as a follow-on issue if not 
>>>>> now).
>>>>>
>>>>> -- Kevin
>>>>>
>>>>>
>>>>> Alexander Zvegintsev wrote:
>>>>>> Please review the updated version
>>>>>>
>>>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
>>>>>> Now we are postponing actual window closing, it happens only 
>>>>>> after we ensure that native window pointer is valid on Swing side.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexander.
>>>>>>
>>>>>> On 23/09/2017 08:01, Sergey Bylokhov wrote:
>>>>>>> Hi, Alexander.
>>>>>>> How can we be sure that the parent frame will not be disposed 
>>>>>>> while we use a pointer?
>>>>>>>
>>>>>>> long ownerWindowPtr = peer.getOverridenWindowHandle();
>>>>>>> <<<<< Dispose the peer
>>>>>>> if (ownerWindowPtr != 0) {
>>>>>>>     //Place window above JavaFX stage
>>>>>>>     CWrapper.NSWindow.addChildWindow(
>>>>>>>     ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
>>>>>>> <<<<< Boom
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> On 9/21/17 22:56, Alexander Zvegintsev wrote:
>>>>>>>> Hi Phil,
>>>>>>>>
>>>>>>>> Please review the updated fix with reflection incorporated
>>>>>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/
>>>>>>>>
>>>>>>>> New issue created JDK-8187803 
>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187803> as JDK 
>>>>>>>> counterpart of this issue.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexander.
>>>>>>>>
>>>>>>>> On 21/09/2017 22:25, Phil Race wrote:
>>>>>>>>> Some procedural comments :
>>>>>>>>> Since 90% of this is in AWT code, I'd have thought awt-dev 
>>>>>>>>> should be included here.
>>>>>>>>> I've added that list.
>>>>>>>>>
>>>>>>>>> And apart from needing separate bug ids, I don't see why the 
>>>>>>>>> bug below is confidential.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I agree with what Kevin pointed out off-line that as in the 
>>>>>>>>> dialog case, the FX side
>>>>>>>>> of the code can use reflection and simply be a harmless 
>>>>>>>>> non-functional no-op
>>>>>>>>> if the SwingAccessor does not provide the new method.
>>>>>>>>>
>>>>>>>>> BTW
>>>>>>>>> 264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
>>>>>>>>> should be "dd" not "d".
>>>>>>>>>
>>>>>>>>> -phil.
>>>>>>>>>
>>>>>>>>> On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> please review the fix
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/
>>>>>>>>>>
>>>>>>>>>> for the issue
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8185634
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>