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

[Sergey Bylokhov]

Looks fine.

On 29.11.16 12:10, dmitry markov wrote:
> 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.
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.