Community
Participate
Working Groups
I20030910 1. In standard config define a key binding in Text Editing context that uses Esc, e.g. Esc w for Clear Mark 2. Open a Java or Text editor 3. Press Ctrl+j to start incremental find 4. Press Esc Observer: the incremental find mode is not left Test Case 2: 1. Open a Java file 2. At a valid location press Ctrl + Space to open content assist 3. Press Esc Observer: content assist is not closed This happens because the styled text widget on which we install a key(verify) listener never receives the Esc. Removing the key binding fixes the problem.
Might be related to bug 43140.
This is a non-trivial problem with the new key binding architecture, as it exists outside of the normal widget hierarchy. Basically, any key that is bound will take priority over all other widget behaviour -- by design, really. However, this is not the normal behaviour seen in other sophisticated key binding environments, such as Microsoft's .NET developer app. In these applications, 'Escape' reaches the widget first. Only if the widget declines to process it does the event reach the global processing phase. Take this example. Bind "Activate Editor" to 'Escape'. Make a resource navigator a fast view. Click on one of the resources and try to change its name. In other apps, clicking 'Escape' once would abort the edit, and again would "Activate Editor". On the current Eclipse platform, the first 'Escape' would "Activate Editor".
Created attachment 6254 [details] Workbench, messages.properties 80% of the work for 20% of the functionality. This allows zero or more keys to be defined as "out-of-order". They are defined in a property file. By default, only "ESC" is defined as out-of-order. An out-of-order key is never processed before traversal by the key bindings. On key down with an out-of-order key, it registers a listener on the widget (a verify listener, if it is a StyledText). After all listeners on a widget have responded and none have set doit=false, then key bindings kick in. While there is flexibility to define more out-of-order keys easily, there is a strong chance that some keys might not work in all cases. In particular, I'm thinking that window traversal keys would be a bad idea. This code (by necessity) is tightly coupled with how SWT processes events. If the order of event-handling architecture in SWT were to change, it is likely that this code will break. However, it seems unreasonable to ask SWT to provide such a highly specialized set of functionality in their API.
"never processed by the key bindings on traversal" should be "never processed before traversal by the key bindings"
1.) Out-of-order key bindings should be statically initialized once. 2.) Fuzz factor of 5 (or just press "Guess") is needed to apply the patch. 3.) Doesn't work on Windows XP, but works on Linux-GTK. Investigating....
Created attachment 6262 [details] Workbench, messages.properties (version 2) + The problem with Windows XP was a case of the patch not applying cleanly. + Out-of-order keys are initialized statically + Problems parsing the out-of-order keys is sent to the WorkbenchPlugin for logging. + Fixed patch application problems.
Created attachment 6263 [details] ActionContributionItem This makes what is probably going to be considered a breaking API change to ActionContributionItem. It fixes the last little bit of the key routing problem, and sets a definite tone that all key bindings should come through the global key binding architecture. Without this patch, if you bind an out-of-order key to a command/action (such as "ESC" to "Activate Editor"), you may find that the command is triggered when you only intended to cancel the operation. The out-of-order key will still get processed by the widget, but it will also trigger the command. This occurs because ActionContributionItem sets a menu accelerator. This patch comments out two lines in ActionContributionItem which neuters its ability to set accelerators on MenuItems. (Note: it still lets ActionContributionItem set accelerator text.) I believe that the current behaviour is not the behaviour seen in other sophisticated applications with configurable keys. I feel that certain keys should always be allowed some default behaviour with priority over the commands/actions they are bound to. "Esc" is a good example of such a key.
chris: review/apply. If you agree that we should apply the patch ActionContribution, we should probably make a request-for-comments on eclipse- dev.
Chris has mentioned that this would not work well on the Mac, since SWT does not support setting accelerator text well (so all key bindings would be left-aligned). Also, it wouldn't get the much-coveted blinking menu effect.
Created attachment 6297 [details] version 3 (Workbench, messages.properties) Fixed to apply on the most recent head.
'version 3 (Workbench, messages.properties)' patch applied..
the patch to ActionContributionItem is out of sync, and i think i would reject this patch anyway. it is true that the command system registers accelerators on the menu in addition to using a key filter on Display. registering accelerators on the menu is redundant now - the key filter on Display works fine for all key strokes. the purpose of registering accelerators on the menu as well at this point is mostly mac-specific, for two reasons: 1. the mac likes to blink the menu item for the user when the key is pressed - we need a real accelerator to get this behavior. 2. SWT menu item api is not consistent on the mac - MenuItem.setAccelerator() on the mac causes the accelerator to be visible. this doesn't happen on any other platform. MenuItem.setAcceleratorText on the mac puts text left-aligned. this also doesn't happen on any other platform. basically, if we remove setting accelerators on the menu, the mac just ain't right. BUT... on a mac, go into the emacs key configuration and notice all the multiple key stroke key sequences. notice there is no blink for these, and these key sequences are left-aligned on the menu. there is nothing we can do about this without SWT's help, and the reality is that it isn't so bad for these cases. enough babbling, the important part starts here :) ... 1. i would apply a fix that treats 'out-of-order' accelerator much like emacs style multiple key stroke key sequences. on the mac, you won't get a blink, and the key sequence will be left-aligned on the menu. 2. the issue of neutering IAction.setAccelerator is *mostly* orthogonal - it only causes a problem here if someone out there actually registers an 'out-of- order' accelerator using IAction.setAccelerator, which should be unlikely at this point. considering that IAction.setAccelerator has been deprecated since 2.1, i will be happy to commit (then propose :) a breaking change for this for M5 if you write me one. the reality is that we can no longer have two systems competing for accelerators menu (because of issues like this).
I've opened an enhancement request to SWT asking for consistent menu API from MacOS X. For M5, I'd propose plying them with beer until they see things our way. In general, I feel that this is the "best", as it improves SWT's Mac support, makes our code cleaner, and provides the best visual effect (fixing the current Emacs weirdness). If that fails, my vote would be for out-of-order keys to get the same special handling as multi-keys on the Mac.
The worst of this is passed. We're now squabbling over some minor points for "completeness". Downgrading to minor, and moving to P3.
Doug, I'm not sure if this is minor yet, please convince me: What if a user binds an 'out-of-order' key stroke to a command (as a standalone key sequence, not as part of a multi-key stroke key sequence). Consider that a user binds a command to 'Esc'. As this is a single key stroke, I will register it on the menu. As accelerators have priority over the display key filter, the menu will eat 'Esc'. Will this not cause problems for you? If so, the patch I need would have the menu not register actual accelerators for out-of-order key strokes or multi-key stroke key sequences. Currently, I only handle the latter.
First of all, the problems it causes are not a regression. Second of all, binding to "Esc" means that the command is executed when "Esc" is pressed -- rather than a cancel action thing (like "close the code assistant"). Not breaking (for example, the code assistant will close anyway), but not ideal. The big not ideal is in a navigator tree on windows, while renaming, pressing "Esc" would execute the command and *accept* the current changes. This is a bit weird. But again, not a regression from what I can tell....
I wouldn't measure breakage just from a code point of view. If something worked in 2.1 and now it does not then I'd say it's a breakage.
Urg. I guess I'm not communicating this very well. 1.) The patch that has gone in fixed the regression from 2.1.1 (note that the patch didn't make it into the last integration build -- try the head or a nightly build). 2.) The outstanding work is to fix something that was broken in 2.1.1 and is still broken now. Hence the decision on my part to downgrade to minor (since no one noticed the problem in 2.1.1, it can't be all that important), and to remove it as a requirement for M4.
With the removal of the native accelerators, the last part of this problem is fixed.
Verified to work in I200312162000.
