Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
+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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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. 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 -- 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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Re: Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Code review request: 7025699: Policy Tool is not accessible by keyboard
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
Code review request: 7025699: Policy Tool is not accessible by keyboard
Hi All Please take a look at http://cr.openjdk.java.net/~weijun/7025699/webrev.00/ The bug is about policytool not accessible through a keyboard. Mainly two problems: First menu items have no key accelerators. Second, pressing ENTER on any button is not working. Basically I made these changes: 1. Assign shortcut keys to 5 menu items, like this -menu.add(QUIT); +menu.add(new MenuItem(QUIT, new MenuShortcut(KeyEvent.VK_Q))); 2. Make all buttons ENTER aware. In the program, an action listener is added to a button with button.addActionListener(new MainWindowListener(tool, this)); and this MainWindowListener implements ActionListener. After this, it seems the button can react to mouse click event. It also reacts to spacebar press, but no other key. Anyway, I make all these button listeners also implementing KeyListener, and override the keyReleased event there so that when ENTER is pressed on a button, trigger a mouse click event on it. Now it seems working in Mac OS X, Linux and Windows XP. However, I'm really not a GUI guy and I am not sure if this fix is the right way. The bug mentions High Contrast Mode is also not working. I have no idea how to fix that. Thanks Max
