<AWT Dev> [9] Review request for 8169589: [macosx] Activating a JDialog puts to back another dialog

[dmitry markov]

Hi Sergey,

You are right. The methods orderAboveSiblings() and 
orderAboveSibligsImpl() are executed on the appkit thread. I have 
updated the fix. Now these functions invoke 
getOwnedWindows_NoClientCode() via window accessor instead of direct 
call of getOwnedWindows from the Window class. The updated webrev is 
located at http://cr.openjdk.java.net/~dmarkov/8169589/webrev.03/

Thanks,
Dmitry
On 28/11/2016 22:47, Sergey Bylokhov wrote:
> Hi, Dmitry.
> As far as I understand the orderAboveSiblingsImpl() is execute on the 
> appkit thread? if yes, then you should not call 
> target.getOwnedWindows() on it, because it can be overridden by the 
> user. You should call "getOwnedWindows_NoClientCode" via accessor, or 
> get a list of child windows via the native api.
>
> On 28.11.16 15:12, Alexey Ivanov wrote:
>> Hi Dmitry,
>>
>> Looks fine to me.
>>
>>
>> Regards,
>> Alexey
>>
>> On 28.11.2016 14:53, dmitry markov wrote:
>>> Hi Alexey,
>>>
>>> I have updated the fix based on your suggestions. Please find new
>>> webrev here: http://cr.openjdk.java.net/~dmarkov/8169589/webrev.02/
>>>
>>> Thanks,
>>> Dmitry
>>> On 28/11/2016 12:51, Alexey Ivanov wrote:
>>>> Hi Dmitry,
>>>>
>>>> If you expand imports
>>>>
>>>>  31 import java.awt.*;
>>>>
>>>> you'll be able to use java.util.List via import:
>>>>
>>>> 1123             java.util.List<Window> pwChildWindows = new
>>>> ArrayList<Window>(Arrays.asList(w.getOwnedWindows()));
>>>>
>>>>
>>>> Actually you don't need this local variable as well as new ArrayList
>>>> object: you can pass the result of Arrays.asList directly to
>>>> childWindows.addAll().
>>>>
>>>> If you remove this local variable, there will be no inconsistency:
>>>>
>>>> 1098         ArrayList<Window> childWindows = new ArrayList<Window>();
>>>> 1123             java.util.List<Window> pwChildWindows = new
>>>> ArrayList<Window>(Arrays.asList(w.getOwnedWindows()));
>>>>
>>>> I mean the former is declared as ArrayList whereas the latter is List.
>>>>
>>>>
>>>> Also you can use zero-sized array allocation in
>>>>
>>>> 1129 orderAboveSiblingsImpl(childWindows.toArray(new
>>>> Window[childWindows.size()]));
>>>>
>>>> because it “seems faster, safer, and contractually cleaner”, see
>>>> https://shipilev.net/blog/2016/arrays-wisdom-ancients/#_conclusion
>>>>
>>>>
>>>> Regards,
>>>> Alexey
>>>>
>>>> On 26.11.2016 16:01, dmitry markov wrote:
>>>>> Hi Sergey,
>>>>>
>>>>> I have added some remarks to the code. The updated webrev is located
>>>>> at http://cr.openjdk.java.net/~dmarkov/8169589/webrev.01/
>>>>> <http://cr.openjdk.java.net/%7Edmarkov/8169589/webrev.01/>
>>>>>
>>>>> Proposed functionality performs ordering operation from the very
>>>>> bottom, (i.e. root owner) so that the windows are ordered above
>>>>> their nearest parent; ancestors of the window, which is going to
>>>>> become ‘main window’, are placed above their siblings.
>>>>>
>>>>> Summary of changes:
>>>>> - orderAboveSiblings() is responsible for retrieval of root owner
>>>>> and initial creation of the list of the windows which have to be
>>>>> ordered.
>>>>> - orderAboveSiblingsImpl(Window[] windows) performs ordering of the
>>>>> windows specified by input array. If the window is one of ancestors
>>>>> of 'main window' or is going to become main by itself, the window
>>>>> will be ordered above its siblings; otherwise the window is just
>>>>> ordered above its nearest parent. This method is recursively called
>>>>> until all windows in window hierarchy are ordered.
>>>>> - Two helper methods: getRootOwner() is responsible for retrieval of
>>>>> root owner for the window and isOneOfOwnersOrSelf(CPlatformWindow
>>>>> window) - tests whether the current window is one of ancestors of
>>>>> the specified window.
>>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>>> On 25 Nov 2016, at 16:16, Sergey Bylokhov
>>>>>> <Sergey.Bylokhov at oracle.com> wrote:
>>>>>>
>>>>>> Hi, Dmitry.
>>>>>> Can you please adds some comments to the code and describe what is
>>>>>> going on.
>>>>>>
>>>>>> On 25.11.16 16:08, dmitry markov wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review a fix for jdk9, please?
>>>>>>>
>>>>>>>    bug: https://bugs.openjdk.java.net/browse/JDK-8169589
>>>>>>>    webrev: http://cr.openjdk.java.net/~dmarkov/8169589/webrev.00/
>>>>>>>
>>>>>>> Problem description:
>>>>>>> Current implementation of CPlatformWindow.orderAboveSiblings() just
>>>>>>> recursively pops up the windows from ‘active’ parent-child window
>>>>>>> chain.
>>>>>>> At the same time other child windows (which are not in active 
>>>>>>> chain)
>>>>>>> stayed ‘untouched’ and may be placed behind their nearest
>>>>>>> parent/owner.
>>>>>>>
>>>>>>> Fix:
>>>>>>> CPlatformWindow.orderAboveSiblings() should be modified. It has to
>>>>>>> take
>>>>>>> into account that a window may own more than one child window.
>>>>>>>
>>>>>>> Note: JCK tests passed on the build with the fix.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Dmitry
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Best regards, Sergey.
>>>>>
>>>>
>>>
>>
>
>