[rfc][icedtea-web][policyeditor] Keyboard shortcuts and mnemonics touchup
Andrew Azores
aazores at redhat.com
Mon Jun 30 19:15:00 UTC 2014
(Sorry for the maybe not-so-good email formatting here - replying from a web interface today unfortunately)
----- Original Message -----
> 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.
Thanks for the tip! VK_CONTEXT_MENU is a nice enhancement. I only have i3 installed so I haven't tested with any other window managers (anyone else reading this, please give it a shot too) but it works fine here with the actual menu key. I also took your advice of changing the Rename accelerator to F2 - sounds reasonable to me, and I wasn't aware that there was actually a "standard" for that. Taking a look at my file manager though (Thunar) I do see that same accelerator listed.
>
> >> 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.
I moved the "Copy URL to clipboard" item up one so it's adjacent to the other "Copy" item. However, I did not rename the "Copy URL to clipboard" item. Not yet, at least. This is because the other Copy and the Paste actions actually do not currently use the system clipboard, but that's another task that will be started on soon. Once Copy and Paste actually use the clipboard then I'll go ahead and rename the "Copy URL". There's another email thread about Copy/Paste where Jiri and I were just discussing this earlier today.
>
> > 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 ;-) )
Also done, good idea.
>
> > 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.
Well, it seems null checks are actually just not required with those two methods, so I guess it's fine to omit. Maybe it will be useful to somebody in the future somehow for removing an accelerator.
>
> 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
>
I know :) that's why I keep coming back to improve PolicyEditor this way. Nobody's really asked me to do these particular things, I just want to make the Editor accessible and easy to use now that I've got it (mostly) providing the actual functionality that it was supposed to provide. Maybe I'm not getting every detail right on the first attempt, but, well, everything needs to be learned for the first time before it can be done right ;)
Thanks,
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyeditor-keybindings-3.patch
Type: text/x-patch
Size: 21799 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140630/8b9519c9/policyeditor-keybindings-3-0001.patch>
More information about the distro-pkg-dev
mailing list