Community
Participate
Working Groups
See org.eclipse.ui.internal.keys.NewKeysPreferencePage org.eclipse.ui.internal.keys.model.KeyController org.eclipse.ui.internal.keys.model.BindingModel We need to be able to adapt the e4 model to the pattern that BindingModel and ContextModel use. PW
Created attachment 189127 [details] To allow the NewKeysPreferencePage to make Binding changes Here's patch #1. All modifications take effect immediately (as they should), and will be persisted. Unfortunately, any SYSTEM key bindings that are unbound will not be remembered for the next start up (unless overwritten by a USER binding). This will be done eventually (patch #2)
I'd like to re-order some stuff to spread the responsibilities around. org.eclipse.e4.ui.bindings provides the runtime binding service. It has no knowledge of the model, or the complexities of the 3.x system. It looks like we should enhance org.eclipse.e4.ui.bindings.EBindingService.createBinding(TriggerSequence, ParameterizedCommand, String, String) so that it takes all of the important parameters for a KeyBinding: locale, platform, and bindingType (SYSTEM, USER) org.eclipse.e4.ui.workbench.swt is responsible for using the model to manage the runtime binding system. We need to make sure that BindingProcessingAddon can add and remove bindings from the runtime system. I would propose that we store some information we need as tags on the MKeyBinding: "type:user" and "type:system" can then be translated into Binding.USER and Binding.SYSTEM. We can also mark system bindings for delete using a "deleted" tag. Storing this information in the model would remove the need to have BindingCopies, I think. The BindingProcessingAddon is then responsible for interacting with the tags to keep the running system up to date. org.eclipse.ui.workbench is the compatibility layer part that can see the extension points. Just some notes: org.eclipse.ui.internal.keys.BindingService.readRegistryAndPreferences(ICommandService) ... using BindingPersistence on the BindingService BindingManager will replace whatever is there, possibly overwriting or merging. I wouldn't do that. We don't have a good story yet for keeping the BindingService bindingManager up to date, as technically it should reflect the the running system (somehow). I think you having the new keys pref page/BindingModel updates OK. I think then you'll just have to update your algorithm in org.eclipse.ui.internal.keys.BindingService.savePreferences(Scheme, Binding[]) 1) if it's a new binding, add the MKeyBinding to the correct MBindingTable with the type:user tag. 2) if it's a "new binding" that shows up, but really it's a system binding that had its delete marker removed, remove the "deleted" tag from the existing MKeyBinding 3) if a binding has been deleted, if it's a type:user binding simply remove it from the model 4) if a binding has been deleted and its a system binding, tag it as "deleted" PW
Created attachment 190585 [details] Second patch. Everything functional this time.
Some comments: - org.eclipse.e4.ui.bindings.EBindingService.createBinding(TriggerSequence, ParameterizedCommand, String, String) was to be enhanced such that it takes ALL params for a KeyBinding. It gets called from the BindingProcessingAddon class which reads in all MKeyBindings and converts them to KeyBindings, then adds them to the BindingTableManager on startup. The information String locale, String platform, and String bindingType are not stored in MKeyBindings. The default implementation was to pass a null locale and platform, and SYSTEM bindingType. I provided a workaround for determining the bindingType. Each MKeyBinding is compared with the list of USER Bindings to determine the bindingType. I'm going by the assumption that the list of USER bindings will generally be fairly small, so the check shouldn't be too computationally aggressive. However, I did not put the check in for the locale and platform of each Binding. Going through the list of SYSTEM bindings for each MKeyBinding seems like it might be a little too costly (O(n^2) in the worst case). - org.eclipse.ui.internal.keys.BindingService.readRegistryAndPreferences(ICommandService)... there is nothing in BindingService's BindingManager on startup, so if we don't use the BindingPersistence on it, then the KeysPreferencePage will crash on load. - There was one issue I ran into when unbinding a SYSTEM binding (the Activate Editor binding to be exact). Removing SYSTEM bindings from the preferences actually does work properly and gets persisted, but NOT for this binding. Following the code in the debugger for the next startup after a remove for a SYSTEM binding, BindingManager.removeBindings(...) actually finds the binding and tries to remove it, but it doesn't actually get removed. I wasn't able to figure this one out. Removing SYSTEM bindings on every other binding that *I* tried actually does work though. Regardless, the functionality mimics that of the 3.x keys pref page for the most part. The above comments only explain some of the issues that are "under the hood". If anyone runs into more issues or has any additional comments, then just let me know. - Jesse
Created attachment 190597 [details] Second patch. Everything functional this time.
Created attachment 190610 [details] Fixed Patch DIREGARD THE FIRST TWO PATCHES! Sorry for the above two. There were changes in the repo so I had some difficulty syncing everything up. This is the correct patch. Take a look! =) - Jesse
Oh, I should also mention that after having synced everything up and testing it out, the problem I was having with removing the Activate Editor binding has disappeared. I wonder if it has anything to do with the fact that I was using a Mac when developing this, or it's because of the updated files from the repo... Regardless, everything appears to be working correctly this time, at least when using Windows XP. =) - Jesse
Created attachment 190699 [details] Fixed patch v05 I've released this patch to HEAD for tonight's build. I cleaned up the sysouts, and we can look at moving away from using the runtime system after M6 PW
I'd like you to revisit how this is organized. 1) start with the org.eclipse.e4.ui.bindings.EBindingService.createBinding(TriggerSequence, ParameterizedCommand, String, String, String, String, int). In e4, the only important attributes are: TriggerSequence sequence ParameterizedCommand command String contextId Make the last parameter a Map of attributes. That will allow us to pass through schemeId, locale, platform, and/or bindingType (which for us can be the string "user"). Then anyone should be able to create a complete KeyBinding with the relevant information. 2) Update org.eclipse.e4.ui.workbench.swt.util.BindingProcessingAddon to be able to read model tags and generate the attribute map. You can create more tests in org.eclipse.e4.ui.bindings.tests.BindingTestSuite somewhere. Let's start with that. PW
I20110310-2200 1. Unbind a command. 2. Commit the changes in the preference dialog. 3. Try to restore the unbound command, it doesn't work.
Created attachment 191183 [details] KeyBinding Patch - Fixes restoring bindings. - BindingCopies is removed - Attribute maps generated to properly create KeyBindings I guess if there are any more problems, I'll hear about them soon enough... - Jesse
> I guess if there are any more problems, I'll hear about them soon enough... - Changing the scheme doesn't work. - Some of the commands seem to appear twice, e.g. 'Save'.
(In reply to comment #12) > > I guess if there are any more problems, I'll hear about them soon enough... > - Changing the scheme doesn't work. > - Some of the commands seem to appear twice, e.g. 'Save'. Note that this is based on 4.1 M6 (not on latest patch).
(In reply to comment #11) > Created attachment 191183 [details] > KeyBinding Patch > Hi Jesse, This patch won't apply on my workspace, although you can still use it as a reference. You need to: 1) reset your workspace, and then sync it up with head. I would go to the sync view (where you can see your changes) and use context menu>Override and Update to get rid of them. Then sync with CVS and accept all incoming changes so your environment matches HEAD 2) then move on to comment #9 Binding createBinding(TriggerSequence sequence, ParameterizedCommand command, String schemeId, String contextId, String locale, String platform, int bindingType); I'd like that to become Binding createBinding(TriggerSequence sequence, ParameterizedCommand command, String contextId, Map<String, String> attributes); attributes can be one of "schemeId", "locale", "platform", "type" When the attributes should be stored in the MKeyBinding, use getTags() and store them in there. tags are usually of the form "tagName:tagValue" The type can just be "type:user" since you'll be able to search for that directly. Modify BindingProcessingAddon so it can parse the tags and make an attribute map when creating bindings though createBindings(). Add a test class similar to BindingLookupTest to BindingTestSuite. In it, create key model elements with various tags and test that createBinding() returns the correct KeyBindings. Once you get that far, attach a patch and we'll give it a review. PW
Created attachment 191714 [details] Keys for Default Scheme Okay, let's try this one... It's good for the default scheme, has the implementation changes that you've asked for, and includes some new tests. I'm going to try to figure out a way to properly allow switching schemes without messing anything up tonight. - Jesse
Also, I think I may have found out the reason for the duplicate 'save' commands that show up in the keys pref page. Going through the commands list from the command manager, there are a number of different save commands: (IDs) org.eclipse.e4.ui.saveAllCommands org.eclipse.ui.file.save org.eclipse.ui.edit.revertToSaved org.eclipse.e4.ui.saveCommands org.eclipse.ui.window.savePerspective org.eclipse.ui.file.saveAll org.eclipse.ui.file.saveAs (Names) Save All Save Revert to Saved Save Save Perspective As Save All Save As Some of these commands have the same name, so I think that's the cause. I'll try to dig a little deeper and see what else I can find out about this. - Jesse
Created attachment 191882 [details] Multiple scheme support Okay, This patch will support multiple schemes. However there are some things that I should note: 1. As of right now, the binding table manager doesn't handle conflicts very well so they will just be ignored. This kind of screws over a few emacs key bindings (I think around 5 or 6). I will be working on a fix for this as it relates to https://bugs.eclipse.org/bugs/show_bug.cgi?id=317201. 2. The issue in 3.x when switching back and forth between schemes occurs here as well. If you switch schemes from the comboview in the keys pref page more then twice, it doesn't update again until you hit restore defaults. I'm still looking into this... 3. I'm not sure what the difference is between org.eclipse.ui.file.save and org.eclipse.e4.ui.saveCommands or any of the other bindings listed above that have the same name. They seem to be doing the same thing when executed. It's being read in from the commands extension point, so there's got to be some reason why it's there. - Jesse
Comment on attachment 191882 [details] Multiple scheme support AGH! I just noticed I posted the wrong patch... jeez. Sorry about this. I'll repost tomorrow.
Created attachment 191929 [details] Multiple scheme support The notes from comment 17 still apply.
Created attachment 194113 [details] Keys pref page v11 iteration 11 - use the BindingManager to manage active keybindings at the compatibility level, and propagate changes into the model. Looks like we need to hook up more listeners in BindingProcessingAddon. Tests can be found in dev.eclipse.org:/cvsroot/eclipse e4/org.eclipse.e4.compatibility/tests/org.eclipse.e4.ui.keybinding.tests and we should add and remove Bindings through the IBindingService to prove they work in the runtime. PW
Created attachment 194304 [details] Keys pref page v15 Iteration 15, changing schemes now works.
(In reply to comment #21) > Created attachment 194304 [details] > Keys pref page v15 Released. I've opened bug 344181 and bug 344182 for some further issues. Thanx Jesse! PW
