<AWT Dev> <Swing Dev> Code review request: 7025699: Policy Tool is not accessible by keyboard

[Anthony Petrov]

+1

--
best regards,
Anthony

On 10/17/2013 09:11 PM, alexander potochkin wrote:
> Hello Leif
>
> Looks good to me
>
> I had a look at the PolicyTool code
> and indeed it is better to leave the IO processing  as is at this moment.
>
> Thanks
> alexp
>
>> Hi All,
>>
>> A new webrev is available at:
>>
>>   http://cr.openjdk.java.net/~weijun/7025699/webrev.01/
>>
>> The following new changes were made:
>>
>> 1. Added call to SwingUtilities.invokeLater() in main() to run the GUI
>> initialization on the Event Dispatch Thread.
>>
>> 2. Minor corrections to initial size and placement of the main window.
>>
>> 3. More mnemonics defined for buttons and labeled fields on the Policy
>> Entry and KeyStore dialogs.
>>
>> Note that we have decided to not implement the use of SwingWorker for
>> File I/O in this release. The main reasons are that it is not covered
>> in the scope of the bug report, the blocking can not actually be
>> considered a regression compared to the AWT version, and it would
>> require enough refactoring of the code to risk affecting the general
>> logic and cause new bugs.
>>
>> I am the Contributor for this bug fix, and Max (Weijun) will be the
>> Committer.
>>
>> Thanks,
>> Leif
>>
>>
>>
>> On 2013-10-15 10:00, Leif Samuelsson wrote:
>>> Thanks for the good feedback. We will make sure to move to move the
>>> GUI instantiation to the EDT and the file I/O to a worker thread.
>>>
>>> All other operations are event driven and therefore occur directly
>>> on the EDT.
>>>
>>> Leif
>>>
>>>
>>> On 2013-10-15 09:56, alexander potochkin wrote:
>>>> Hello
>>>>
>>>>> Well, performing I/O or other blocking operations on EDT can only
>>>>> freeze the app's GUI for the period of blocking. If
>>>>> developers/users are OK with that, this is fine with me too.
>>>>>
>>>>
>>>> I don't think an I/O blocking operation on EDT is acceptable. It can
>>>> freeze the whole application and give a bad experience to the users.
>>>> Please consider using the SwingWorker API
>>>> http://docs.oracle.com/javase/tutorial/uiswing/concurrency/worker.html
>>>>
>>>>
>>>>> However, you should still call all Swing APIs (including creating
>>>>> your components/windows) on the EDT. And I don't see this is being
>>>>> done in your current code. As a minimum, the displayToolWindow()
>>>>> method should be dispatched on the event thread. I haven't examined
>>>>> the code closely, but if there are any other GUI updates from
>>>>> separate threads, those should also be moved to the EDT. See the
>>>>> following tutorial for details:
>>>>>
>>>>> http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html
>>>>>
>>>>>
>>>>
>>>> Indeed, rewriting an AWT app as a Swing app is not only about
>>>> changing Labels to JLabels
>>>> Please make sure that all Swing code are used on the event
>>>> dispatching thread only.
>>>>
>>>> Thanks
>>>> alexp
>>>>
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 10/15/2013 06:13 PM, Weijun Wang wrote:
>>>>>> Policy Tool is a GUI editor for the plain text policy file. The
>>>>>> only I/O
>>>>>> is loading the policy file from disk and it should be quite small,
>>>>>> so I
>>>>>> think there won't be a problem here.
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>> On 10/15/13 8:08 PM, Anthony Petrov wrote:
>>>>>>> Hi Max,
>>>>>>>
>>>>>>> I don't have expertise in this code so I haven't reviewed the fix
>>>>>>> thoroughly. I'd like to point out one thing though: unlike AWT,
>>>>>>> Swing is
>>>>>>> a single-threaded GUI toolkit. While in AWT you can create
>>>>>>> components/windows and call APIs on any thread, in Swing everything
>>>>>>> GUI-related must be performed on the Event Dispatch Thread (EDT)
>>>>>>> only.
>>>>>>> Any long, non-GUI-related operations (like I/O, computations, etc.)
>>>>>>> should be dispatched on other threads (with a SwingWorker, for
>>>>>>> example).
>>>>>>>
>>>>>>> I don't see a single SwingUtilities.invokeLater/invokeAndWait()
>>>>>>> call in
>>>>>>> PolicyTool.java, so I thought I'd ask whether you're aware of the
>>>>>>> threading limitations imposed by Swing and how those are going to be
>>>>>>> addressed?
>>>>>>>
>>>>>>> --
>>>>>>> best regards,
>>>>>>> Anthony
>>>>>>>
>>>>>>> On 10/12/2013 04:49 AM, Weijun Wang wrote:
>>>>>>>> Hi All
>>>>>>>>
>>>>>>>> Please review the fix at
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~weijun/7025699/webrev.00/
>>>>>>>>
>>>>>>>> The fix includes porting PolicyTool from AWT to Swing, defining
>>>>>>>> mnemonics for menu items and buttons, and adding keyboard
>>>>>>>> shortcuts for
>>>>>>>> the File -> New/Open/Save items. Several tests are updated also.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Max
>>>>
>