(not so) trivial keyboard shortcut fixes and cleanup #4279
Conversation
matthijskooijman
force-pushed
the
keys
branch
from
c95d558 to
d06c421
Compare
matthijskooijman
changed the title
|
While testing, @cmaglie pointed out that the ctrl-tab/ctrl-shift-tab bindings would actually break in some circumstances. Further digging turned out that the accelerators in the tab popup menu would actually not work reliably (only until the menu was opened for the first time), so I dug in to find a way to make them more reliable. After a few failed attempts, I ended up with a working solution. As part of this solution, I switched the menu items to use Action objects, and it made sense to move the ctrl-alt-left/right bindings into EditorHeader as well. This left EditorListener a nearly empty class, containing just one overly-broad, poorly documented hack, so I figured out a slightly better solution for that as well, so EditorListener could be entirely removed. So, this PR ended up a fair bit more complex that the original, but at least things are even cleaner than before. |
matthijskooijman
force-pushed
the
keys
branch
from
d06c421 to
b3e07b5
Compare
|
I fixed one more commit to not introduce unused imports. @ffissore, I see Arduinobot saying the build failed, also on another PR, but it builds completely locally. Is there a problem with the bot? |
|
Yes, bandwidth problems :( It should be fixed now |
|
@ArduinoBot build this please |
Instead of defining JMenuItems, setting accelerator keys, and attaching an ActionListener inline, this first defines a list of actions (with a name, optional accelerator key and using a lambda as the action listener). Then menu items are added, that simply refer to the previously defined actions. The actions are defined in a inner class named Actions, of which one instance is created. This allows grouping the actions together inside the `actions` attribute, and allows external access to the actions (in case the same action is present in multiple menus, or otherwise performed from other places). This might not be the best way to expose these actions, and perhaps they should be moved elsewhere, but for now this seems like a good start. This adds new helper classes SimpleAction, to allow more consisely defining Action instances, and a new class Keys, to allow consisely defining keyboard shortcuts.
Some items in this menu had accelerator keys (shortcuts) defined. Normally, this automatically takes care of registering such keybindings when the menu item is added to a menu. However, this requires adding the item (indirectly) to a menubar, which is again added to a window. Since the tab menu is just a separate popup menu, this did not work. It seems an attempt was made to fix this by adding the popup menu to the EditorHeader JComponent, which indeed made the keybindings work. However, this is a hack at best, and as soon as the popup menu was opened, it would be moved to another container and again detached when the menu was closed, breaking the keyboard shortcuts again (re-adding the popup menu turned out not to work either, then the menu would actually be drawn on top of the tab bar). To properly fix this, keybindings for the menu items are added to the EditorHeader itself. By looking at the existing accelerator keystroke property of the actions, there is no need to duplicate the keystrokes themselves, and the displayed value will always match the actually bound value. To simplify this, some methods are added to the Keys helper class, which will likely come in handy in other places as well.
Previously, this would use a single ActionListener object, and pass the filename of the file to switch to in the action command. This means that whenever switching the filename needs to be looked up. This commit instead uses a lambda to capture the index of the tab to switch to for every tab (so it uses a different ActionListener for each tab).
These key combinations were registered as accelerator keystrokes in the tab bar popup menu, but also handled by EditorListener. This was probably added in an attempt to work around the broken accelerator keys on the tab bar popup menus, but in practice this only meant that the shortcut would sometimes (and now that the accelerator keys are fixed, always) switch tabs *twice*. Removing the handling from EditorListener helps to fix this. References: arduino#4228
Previously, EditorListener handled these keys, by registering a handler in the text area. This meant that they would only work while the text area was focused. By registering the keys as WHEN_IN_FOCUSED_WINDOW bindings, they can always be handled, and EditorHeader seems like a more appropriate place. Note that this does not currently work (so this commit breaks these keys), since these keys are also handled in a few different places as well, preventing these newly added keybindings to take effect. This will be fixed in the next commit. One additional change is that previously, these keybindings did not work when the text area was readonly. This was probably a remnant from when EditorListener took care of a lot of other editing keybindings, but this does not make much sense anymore now. Finally, with the old bindings, ctrl-shift-tab did not (seem to) work. What happened is that the binding for ctrl-tab did not check the shift state, so both bindings would fire on ctrl-shift-tab, switching forward and back again, making it seem the keys did not work. The Swing keybinding mechanism that is now used for these bindings checks the complete keystroke, including all modifier keys, so this problem is fixed by this change. References arduino#195
In the previous commit, these bindings were moved to EditorTab and registered in a cleaner way, but this move also allows more components to hijack these keystrokes and prevent them from reaching EditorTab. This commit makes the keybindings work again, by preventing other components from handling the keys. In particular: - JSplitPane had a binding to switch between its two panes, which is now removed after creating the JSplitPane. - The default focus traversal manager in Swing uses these keys to traverse focus (in addition to the the normal tab and shift-tab keys). By removing these keys from the set of "focus traversal keys" defined for the window, this should be prevented when the focus is on any component inside the window. - JTextPane didn't respond to the previous modification of the window-default focus traversal keys, since it defines its own set (to only contain ctrl-tab and ctrl-shift-tab, but not tab and shift-tab, for undocumented reasons). To fix this, focus traversal is simply disabled on the JTextPane, since this wasn't really being used anyway. There was some code in SketchTextArea that tried to modify the focus traversal keys for just the text area, which is now removed. This code wasn't really useful, since focus traversal is disabled for the text area already. Also, the code contained a bug where it would not actually set the new set of keys for the backward focus traversal. Closes arduino#195
Previously, there was a handler on the text area that consumed most KEY_TYPED events with control pressed. This was added a long time ago to fix a problem with ctrl-slash doing both the toggle comment action and inserting a /. Further investigation shows that with RSyntaxTextArea this problem is still present, but is caused by a weird binding on the slash key that Arduino is not even using. Removing that binding is a cleaner workaround for this problem, so this commit switches to that workaround. Ideally this would be fixed in RSyntaxTextArea, see bobbylight/RSyntaxTextArea#157
It did not contain any actual code anymore, so it can be removed, along with the infrastructure for setting it up.
matthijskooijman
force-pushed
the
keys
branch
from
b3e07b5 to
7eea624
Compare
|
And one more push, I forgot to add license headers on two new java files, which I now fixed. |
|
I gave it a quick test. It works for short key presses. As long as I hold down the ctr alt right for a rew seconds it doesnt work anymore. However the editor doesnt crash, but the shortcut seems to break (for the whole opened instance). Scrolling over the tabs is still not implemented, which would be the most intuitive switching. But I guess that wasnt aimed in this PR. |
Weird, I'm not sure what's going on there, then.
Correct :-) |
cmaglie
added
feature request
|
The described bug from my last post still exists. However if you dont escalate on holding down those keys it works fine. It should be fixed though. |
|
|
Here's a few cleanups and fixes to EditorListener, which handles some keyboard shortcuts.