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

[Leif Samuelsson]

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
>>