[rfc][icedtea-web][policyeditor] Keyboard shortcuts and mnemonics touchup

Fri Jun 27 11:30:53 UTC 2014

On 06/26/2014 09:16 PM, Andrew Azores wrote:
> Hi,
>
> This patch enhances PolicyEditor's accessibility quite a lot. Changes include:
>
> 1) Pressing "Enter" while a Group Checkbox has focus will expand the group
> (pressing space still selects the entire group at once)
> 2) Every menu item now has an accelerator shortcut and they all actually work
> properly
> 3) Accelerator shortcuts are more sane/intuitive

Thank you for addressing this on your own. Accelerators and mnemonics have been 
lumped together since IcedTea-Web 1.5 in the policy editor.

> 4) Defining shortcut keys in Messages.properties is simpler (! This one requires
> help from translators to fix the key bindings. It's also not entirely ideal
> because the modifier masks are not configurable via Messages.properties right
> now either - anyone have ideas on how to do that more neatly than having the
> Messages actually specify eg "C-S" or "C-S-S" for Ctrl-S/Ctrl-Shift-S per
> shortcut, for example? This wouldn't really be so bad though, I guess,
> especially since these strings would be defined solely by the person writing the
> translations, and never input by users)

Have a look at javax.swing.KeyStroke.getKeyStroke(java.lang.String). The Strings 
that this method takes could easily be stored in a properties file, the format 
is relatively well defined, and it should also work for non-English alphabets. 
It is actually the exact same String that KeyStroke.toString() produces on a 
KeyStroke instance.

> Coming up later will be accelerators/mnemonics for CustomPolicyViewer.

Yeah, please do not confuse accelerators and mnemonics. Also, please keep the 
terminology apart.

 > +# Menu navigation (Alt-mask)

This should rather say "mnemonics" than "Alt-mask"

 > +PEFileMenuMnemonic=F
 > +PEEditMenuMnemonic=E
 > +PEViewMenuMnemonic=V
 > +
 > +# Key shortcuts (Ctrl-mask)

This should say "accelerators" than "Ctrl-mask".

 > +PEAddCodebaseMnemonic=N
 > +PERemoveCodebaseMnemonic=R
 > +PEOpenMenuItemMnemonic=O
 > +PESaveMenuItemMnemonic=S
 > +PEExitMenuItemMnemonic=Q
 > +PECustomPermissionsItemMnemonic=U
 > +PECopyCodebaseItemMnemonic=C
 > +PEPasteCodebaseItemMnemonic=V
 > +PECopyCodebaseToClipboardItemMnemonic=B
 > +
 > +# Key shortcuts (Ctrl-Shift-mask)

These are accelerators too.

 > +PESaveAsMenuItemMnemonic=S
 > +PERenameCodebaseItemMnemonic=R
 > […]
 > @@ -363,7 +365,7 @@ public class PolicyEditor extends JPanel
 >
 >          copyCodebaseToClipboardButtonAction = new ActionListener() {
 >              @Override
 > -            public void actionPerformed(ActionEvent e) {
 > +            public void actionPerformed(final ActionEvent e) {

The method can be final too. ;-)

 >                  final String selectedCodebase = getSelectedCodebase();
 >                  if (selectedCodebase.isEmpty()) {
 >                      return;
 > @@ -396,7 +398,17 @@ public class PolicyEditor extends JPanel
 >              }
 >          };
 >
 > -        setAccelerators();
 > +        closeButtonAction = new ActionListener() {
 > +            @Override
 > +            public void actionPerformed(final ActionEvent event) {

The method can be final too. ;-)

 > +                final Window parentWindow =
 > SwingUtilities.getWindowAncestor(PolicyEditor.this);
 > +                if (parentWindow instanceof PolicyEditorWindow) {
 > +                    ((PolicyEditorWindow) parentWindow).quit();
 > +                }
 > +            }
 > +        };
 > +        closeButton.setText(R("ButClose"));
 > +        closeButton.addActionListener(closeButtonAction);

If this button is supposed to close the parent window and PolicyEditor is a 
reusable JPanel Component then the close button should not be part of this 
JPanel. Instead, buttons handling the lifetime and/or visibility of a window 
should be direct children of the window they are handling.
Unless, you have a "bar" of buttons like "OK", "Cancel", and "Apply" that you 
can group together in a JPanel for reuse.

Besides, although this can be found in numerous examples in Java books, simply 
closing or quitting a Window isn't actually enough. The associated listeners can 
still receive events after the window has been closed. Usually, this is not a 
problem because mostly there is no way for the user to put focus on them or 
their children, or for the components to acquire focus. But, this is technically 
not correct since the listeners live on as long as they have not been garbage 
collected. So, they can still receive events programmatically.
I do not insist, but for the sake of completeness you could remove all the 
listeners after quitting the parent window.

 > […]
 > @@ -935,14 +847,11 @@ public class PolicyEditor extends JPanel
 >       * @param mnemonic the mnemonic to set
 >       */
 >      private static void setComponentMnemonic(final AbstractButton component, 
 > final String mnemonic) {
 > -        final int trig;
 > -        try {
 > -            trig = Integer.parseInt(mnemonic);
 > -        } catch (final NumberFormatException nfe) {
 > -            OutputController.getLogger().log(nfe);
 > +        if (mnemonic.length() != 1 || !mnemonic.matches("[a-zA-Z]")) {

It is probably not a good idea to work with regular expressions here. For one, 
this expression does not cover alphabets besides English. For two, if you really 
want to use a regular expression in this case it should be a final static 
compiled pattern instead of a String.

 >              return;
 >          }
 > -        component.setMnemonic(trig);
 > +        final char ch = (char) mnemonic.getBytes()[0];
 > +        component.setMnemonic((int) ch);
 >      }
 >
 >      private static JMenuBar createMenuBar(final Window window, final
 > PolicyEditor editor) {
 > @@ -965,7 +874,7 @@ public class PolicyEditor extends JPanel
 >
 >          final JMenuItem saveAsItem = new JMenuItem(R("PESaveAsMenuItem"));
 >          setComponentMnemonic(saveAsItem, R("PESaveAsMenuItemMnemonic"));
 > - 
saveAsItem.setAccelerator(KeyStroke.getKeyStroke(saveAsItem.getMnemonic(),
 > ActionEvent.CTRL_MASK));
 > + 
saveAsItem.setAccelerator(KeyStroke.getKeyStroke(saveAsItem.getMnemonic(),
 > ActionEvent.CTRL_MASK | ActionEvent.SHIFT_MASK));

This is NOT correct! Do not do this. Accelerators != mnemonics. Although 
accelerators' keys and mnemonics seem be equal in most cases in English, this is 
definitely not the case for many other languages, and in few cases not even in 
English. So, do not do this. Accelerators and mnemonics are two distinct 
concepts, so please do not mix them.

 >          saveAsItem.addActionListener(editor.saveAsButtonAction);
 >          fileMenu.add(saveAsItem);
 > @@ -973,31 +882,47 @@ public class PolicyEditor extends JPanel
 >          setComponentMnemonic(exitItem, R("PEExitMenuItemMnemonic"));
 >          exitItem.setAccelerator(KeyStroke.getKeyStroke(exitItem.getMnemonic(),
 > ActionEvent.CTRL_MASK));
 >          exitItem.addActionListener(editor.closeButtonAction);
 > -        exitItem.addActionListener(new ActionListener() {
 > -            @Override
 > -            public void actionPerformed(final ActionEvent e) {
 > -                window.dispose();
 > -            }
 > -        });
 >          fileMenu.add(exitItem);
 >          menuBar.add(fileMenu);
 >
 >          final JMenu editMenu = new JMenu(R("PEEditMenu"));
 >          setComponentMnemonic(editMenu, R("PEEditMenuMnemonic"));
 >
 > +        final JMenuItem addNewCodebaseItem = new
 > JMenuItem(R("PEAddCodebase"));
 > +        setComponentMnemonic(addNewCodebaseItem, R("PEAddCodebaseMnemonic"));
 > +
 > addNewCodebaseItem.setAccelerator(KeyStroke.getKeyStroke(addNewCodebaseItem
 > .getMnemonic(), ActionEvent.CTRL_MASK));