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

[alexander potochkin]

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