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

[Andrew Azores]

On 06/27/2014 07:30 AM, Jacob Wisor wrote:
> 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.

PolicyEditor has only existed since very shortly before the 1.5 release.

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

Ah wow, I hadn't actually paid attention to that particular method. 
That's pretty much exactly what I was hoping to find.

>
>> Coming up later will be accelerators/mnemonics for CustomPolicyViewer.
>
> Yeah, please do not confuse accelerators and mnemonics. Also, please 
> keep the terminology apart.

How do you figure I've confused them here?

>
> > +# 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.

The reason I've distinguished between Alt, Ctrl, and Ctrl-Shift here is 
so that multiple keys mapping to the same value here are not mistaken 
for actually being the same key binding. Although using getKeyStroke as 
above will make that quite obvious anyway, so then those comments can 
certainly be removed.

>
> > +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);

I'm not going to make the methods final.

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

There actually are "Apply" and "Cancel" buttons both side by side in the 
panel. They could be moved into the parent Window, but then they'd need 
to be there in both the Frame and Dialog that are offered. The Panel is 
also never reused and installed into anything other than the parent 
Window it's put in initially.

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

If you can produce some sample code which demonstrates this as an actual 
issue then I'll be happy to consider removing all the listeners from 
every component before disposing. Right now this sounds very paranoid to me.

>
> > […]
> > @@ -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.

Hmm... fair enough.

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

I know they are two distinct things. What do you have in particular 
against setting the same character as the mnemonic and accelerator for 
an action? What do you propose as an alternative - would you prefer to 
have separate keys for each in Messages.properties?

>
> > 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));
>
> Again, do not do this! Accelerator != mnemonic.
>
> > + addNewCodebaseItem.addActionListener(editor.addCodebaseButtonAction);
> > +        editMenu.add(addNewCodebaseItem);
> > +
> > +        final JMenuItem removeCodebaseItem = new
> > JMenuItem(R("PERemoveCodebase"));
> > +        setComponentMnemonic(removeCodebaseItem,
> > R("PERemoveCodebaseMnemonic"));
> > +
> > 
> removeCodebaseItem.setAccelerator(KeyStroke.getKeyStroke(removeCodebaseItem
> > .getMnemonic(), ActionEvent.CTRL_MASK));
>
> Again, do not do this! Accelerator != mnemonic.
>
> > +
> > 
> removeCodebaseItem.addActionListener(editor.removeCodebaseButtonAction);
> > +        editMenu.add(removeCodebaseItem);
> > +
> > +        editMenu.addSeparator();
> > +
> >          final JMenuItem renameCodebaseItem = new
> > JMenuItem(R("PERenameCodebaseItem"));
> > +        setComponentMnemonic(renameCodebaseItem,
> > R("PERenameCodebaseItemMnemonic"));
> > +
> > 
> renameCodebaseItem.setAccelerator(KeyStroke.getKeyStroke(renameCodebaseItem
> > .getMnemonic(), ActionEvent.CTRL_MASK | ActionEvent.SHIFT_MASK));
>
> Again, do not do this! Accelerator != mnemonic.
>
>
> > 
> renameCodebaseItem.addActionListener(editor.renameCodebaseButtonAction);
> >          editMenu.add(renameCodebaseItem);
> >
> >          final JMenuItem copyCodebaseItem = new
> > JMenuItem(R("PECopyCodebaseItem"));
> > +        setComponentMnemonic(copyCodebaseItem,
> > R("PECopyCodebaseItemMnemonic"));
> > +
> > copyCodebaseItem.setAccelerator(KeyStroke.getKeyStroke(copyCodebaseItem
> > .getMnemonic(), ActionEvent.CTRL_MASK));
>
> Again, do not do this! Accelerator != mnemonic.
>
> > copyCodebaseItem.addActionListener(editor.copyCodebaseButtonAction);
> >          editMenu.add(copyCodebaseItem);
> >
> >          final JMenuItem pasteCodebaseItem = new
> > JMenuItem(R("PEPasteCodebaseItem"));
> > +        setComponentMnemonic(pasteCodebaseItem,
> > R("PEPasteCodebaseItemMnemonic"));
> > +
> > 
> pasteCodebaseItem.setAccelerator(KeyStroke.getKeyStroke(pasteCodebaseItem
> > .getMnemonic(), ActionEvent.CTRL_MASK));
>
> Again, do not do this! Accelerator != mnemonic.
>
> > pasteCodebaseItem.addActionListener(editor.pasteCodebaseButtonAction);
> >          editMenu.add(pasteCodebaseItem);
> >
> >          final JMenuItem copyCodebaseToClipboardItem = new
> > JMenuItem(R("PECopyCodebaseToClipboardItem"));
> > +        setComponentMnemonic(copyCodebaseToClipboardItem,
> > R("PECopyCodebaseToClipboardItemMnemonic"));
> > +
> > copyCodebaseToClipboardItem.setAccelerator(KeyStroke
> > .getKeyStroke(copyCodebaseToClipboardItem.getMnemonic(),
> > ActionEvent.CTRL_MASK));
>
> Again, do not do this! Accelerator != mnemonic.

Please do not do this non-helpful comment repetition. If you are so 
averse to it then please provide an actual alternative suggestion rather 
than just saying "No, this sucks, never do it ever" for half the length 
of your mail.

>
> >
> > copyCodebaseToClipboardItem.addActionListener(editor
> > .copyCodebaseToClipboardButtonAction);
> >          editMenu.add(copyCodebaseToClipboardItem);
> >          menuBar.add(editMenu);
>
> Apart from the occasional methods in anonymous classes that could be 
> made final too, the rest seems to be okay.
>
> Jacob


-- 
Andrew A