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

[Jacob Wisor]

On 06/27/2014 05:14 PM, Andrew Azores wrote:
> On 06/26/2014 03: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)

Oh, now I understand what you are intending to do... So you want to kind of 
simulate a right-click on the keyboard. Then the key should probably be 
KeyEvent.VK_CONTEXT_MENU instead of KeyEvent.VK_ENTER. On Windows 
KeyEvent.VK_CONTEXT_MENU maps to SHIFT+F10 (right-click). On other window 
managers KeyEvent.VK_CONTEXT_MENU should also map to right-click or some other 
key combination in place of a right-click, or maybe just the menu key. Try it 
with your window manager. This one is tricky.

>> 2) Every menu item now has an accelerator shortcut and they all actually work
>> properly
>> 3) Accelerator shortcuts are more sane/intuitive
>> 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)
>>
>> Coming up later will be accelerators/mnemonics for CustomPolicyViewer.
>>
>> Thanks,
>>
>
> Here's round two of the patch, taking into account the helpful input from Omair
> and Jacob. Difference from last patch:
>
> 1) Mnemonics and accelerators are separated in Messages.properties

Good.

> 2) KeyStroke.getKeyStroke is used, so accelerators are fully defined in
> Messages.properties, including modifier keys

Good.

> 3) The Edit menu has been renamed Codebase, and menu items have have "codebase"
> removed from their labels

Even better! The more compact the better. Please do not confuse with "the 
shorter the better". ;-) Yeah, writing well understandable texts is difficult 
but it pays off.

> 4) Some mnemonics tweaked to be a little more convenient
> 5) "Copy URL to codebase" is control shift c rather than control b

"Copy URL to codebase" is also too long. See below for details.

 > @@ -562,11 +562,13 @@ PEOpenMenuItem=Open...
 >  PESaveMenuItem=Save
 >  PESaveAsMenuItem=Save As...
 >  PEExitMenuItem=Exit
 > -PEEditMenu=Edit
 > -PERenameCodebaseItem=Rename codebase
 > -PECopyCodebaseItem=Copy codebase
 > -PEPasteCodebaseItem=Paste codebase
 > -PECopyCodebaseToClipboardItem=Copy codebase URL to clipboard
 > +PECodebaseMenu=Codebase
 > +PEAddCodebaseItem=Add new
 > +PERemoveCodebaseItem=Remove
 > +PERenameCodebaseItem=Rename
 > +PECopyCodebaseItem=Copy
 > +PEPasteCodebaseItem=Paste
 > +PECopyCodebaseToClipboardItem=Copy URL to clipboard

I would prefer to shorten PECopyCodebaseToClipboardItem to simply "Copy URL". It 
should be obvious that this data goes to clipboard, so no need for mentioning it 
explicitly. Because, if "Copy" does copy too, where does it copy to if "Copy URL 
to clipboard" copies to clipboard? So if you set PECopyCodebaseToClipboardItem 
to just "Copy URL" then you could set PECopyCodebaseItem to perhaps "Copy 
codebase". Although this may seem redundant now, I think it is far less 
confusing than to have only one JMenuItem labeled as copying to clipboard. Well, 
you'll figure. In any case, please put copyCodebaseItem and 
copyCodebaseToClipboardItem in proximity to each other in the JMenu, e.g. like 
LibréOffice does with "Paste" and "Paste special...". This should be implication 
enough that they both operate on the clipboard but on different data.

 >  PERenameCodebase=Rename codebase to:
 >  PEPasteCodebase=Paste copied codebase as:
 >  PEViewMenu=View
 > […]
 > +PERenameCodebaseItemMnemonic=E
 > +
 > +# See javax.swing.KeyStroke.getKeyStroke(String)
 > +PEAddCodebaseItemAccelerator=control N
 > +PERemoveCodebaseItemAccelerator=control shift R
 > +PEOpenMenuItemAccelerator=control O
 > +PESaveMenuItemAccelerator=control S
 > +PESaveAsMenuItemAccelerator=control shift S
 > +PEExitMenuItemAccelerator=control Q
 > +PECustomPermissionsItemAccelerator=control U
 > +PECopyCodebaseItemAccelerator=control C
 > +PEPasteCodebaseItemAccelerator=control V
 > +PERenameCodebaseItemAccelerator=control R

Alternatively, I would suggest F2 here because it is a common accelerator on 
many desktop managers for renaming.

 > +PECopyCodebaseToClipboardItemAccelerator=control shift C
 > […]
 > @@ -934,85 +846,130 @@ public class PolicyEditor extends JPanel
 >       * @param component the component for which to set a mnemonic
 >       * @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);
 > +    private static void setButtonMnemonic(final AbstractButton button, final 
 > String mnemonic) {
 > +        if (mnemonic.length() != 1) {

Yeah, checking the mnemonic's length should be enough.

I think it should also be a good idea to log a warning or debug level message 
here, just in case the mnemonic is empty or longer than 1. This would greatly 
help to catch any errors while localizing (and Jiri should put an additional 
test for accelerators and mnemonics to work too ;-) )

 >              return;
 >          }
 > -        component.setMnemonic(trig);
 > +        final char ch = mnemonic.charAt(0);
 > +        button.setMnemonic(ch);
 > +    }
 > +
 > +    private static void setMenuItemAccelerator(final JMenuItem menuItem,
 > final String accelerator) {
 > +        final KeyStroke ks = KeyStroke.getKeyStroke(accelerator);
 > +        if (ks != null) {
 > +            menuItem.setAccelerator(ks);

Although I am pretty sure setAccelerator() also accepts null — in case one would 
like to remove an accelerator — checking ks for null is okay here.

It would be a good idea to have a debug or warning message logged here too, just 
in case the KeyStroke turns out to be null.

 > +        }
 > […]
 > +        /*
 > +         * JList has default Ctrl-C and Ctrl-V bindings, which we want to
 > override with custom actions
 > +         */
 > +        final InputMap listInputMap = editor.list.getInputMap();
 > +        final ActionMap listActionMap = editor.list.getActionMap();
 > +
 > +        final Action listCopyOverrideAction = new AbstractAction() {
 > +            @Override
 > +            public void actionPerformed(final ActionEvent e) {
 > +                editor.copyCodebaseButtonAction.actionPerformed(e);
 > +            }
 > +        };
 > +
 > +        final Action listPasteOverrideAction = new AbstractAction() {
 > +            @Override
 > +            public void actionPerformed(final ActionEvent e) {
 > +                editor.pasteCodebaseButtonAction.actionPerformed(e);
 > +            }
 > +        };
 > +
 > +        listInputMap.put(copyCodebaseItem.getAccelerator(),
 > "CopyCodebaseOverride");
 > +        listActionMap.put("CopyCodebaseOverride", listCopyOverrideAction);
 > +        listInputMap.put(pasteCodebaseItem.getAccelerator(),
 > "PasteCodebaseOverride");
 > +        listActionMap.put("PasteCodebaseOverride", listPasteOverrideAction);
 > +

Yep, looks good.

 > […]
 > @@ -1063,20 +1020,31 @@ public class PolicyEditor extends JPanel
 >              groupCh.setToolTipText(R("PEGrightClick"));
 >              groupCh.addMouseListener(new MouseAdapter() {
 >                  @Override
 > -                public void mouseClicked(MouseEvent e) {
 > +                public void mouseClicked(final MouseEvent e) {
 >                      if (e.getButton() == MouseEvent.BUTTON3) {
 > -                        groupPanel.setVisible(!groupPanel.isVisible());
 > -                        PolicyEditor.this.validate();
 > -                        final Window w =
 > SwingUtilities.getWindowAncestor(PolicyEditor.this);
 > -                        if (w != null) {
 > -                            w.pack();
 > -                        }
 > +                        toggleExpandedCheckboxGroupPanel(groupPanel);
 > +                    }
 > +                }
 > +            });
 > +            groupCh.addKeyListener(new KeyListener() {
 > +                @Override
 > +                public void keyTyped(final KeyEvent e) {
 > +                }
 > +
 > +                @Override
 > +                public void keyReleased(final KeyEvent e) {
 > +                }
 > +
 > +                @Override
 > +                public void keyPressed(final KeyEvent e) {
 > +                    if (e.getKeyCode() == KeyEvent.VK_ENTER) {

In any case, you could add KeyEvent.VK_CONTEXT_MENU here because it is roughly 
an equivalent to a right-click.

 > +                        toggleExpandedCheckboxGroupPanel(groupPanel);
 >                      }
 >                  }
 >              });
 >              groupCh.addActionListener(new ActionListener() {
 >                  @Override
 > -                public void actionPerformed(ActionEvent e) {
 > +                public void actionPerformed(final ActionEvent e) {
 >                      final String codebase = getSelectedCodebase();
 >                      if (codebase == null) {
 >                          return;

This stuff may seem like details or nit picking but this is actually part of 
what makes good software. Going into details, tuning, making it easier and 
easier, making look and feel consistent with the rest of the system, etc. is 
what makes software such a pleasure to use. Most users do not perceive this 
actively but all major software packages that are heavy on UI are only so 
accessible and relatively easy to use because their developers spend a 
considerable amount of time on perfecting the UI experience. So keep on 
perfecting! ;-)

Jacob