Community
Participate
Working Groups
Build ID: M20071023-1652 Steps To Reproduce: 1. Create a command with a valid key binding 2. Place it through Menu Contributions extension on toolbar 3. Put mouse pointer over newly created button(command) on toolbar => tooltip shows but without key sequence 4. Press the keys from keybinding sequence => command is executed 5. Bind the command Action/ActionSet and place it in workbench => when tooltip is shown it includes binded key sequence More information: We are trying to migrate from ActionSets to new MenuContribution solutions. In older solution keybindings were shown to user, in new one they are not. This is not acceptable for our clients...
Do you have a sample plug-in project that you can attach to this bug to reproduce the behaviour?
Created attachment 112126 [details] sample rcp application that shows the bug The attached rcp application shows the described bug. The application provides contributions via org.eclipse.ui.menus to add the save command to a file menu and to a toolbar. While the default key binding CTRL+S is shown in the menu item, the toolbar button's tooltip only shows the command's name. To demonstrate the previous behaviour, I added another toolbar and added the default workbench action SAVE to it (see ApplicationActionBarAdvisor). The action button's tooltip does show the key binding in the tooltip.
This can be fixed in org.eclipse.ui.menus.CommandContributionItem.update(String). When creating the tooltip, it needs to get the best active binding from org.eclipse.ui.keys.IBindingService.getBestActiveBindingFor(ParameterizedCommand) PW
(In reply to comment #3) > This can be fixed in > org.eclipse.ui.menus.CommandContributionItem.update(String). When creating the > tooltip, it needs to get the best active binding from > org.eclipse.ui.keys.IBindingService.getBestActiveBindingFor(ParameterizedCommand) > I tried this in my ganymede installtion (build id I20080617-2000) but didn't get any binding returned from getBestActiveBindingFor(command). I did a little debugging and noticed that the BindigService's BindingManager does not contain any active bindings at the time the toolbar contributions are evaluated. The menu bar contribution seems to get evaluated later, when the file menu is actually opened and debugging this showed, that the CommandContributionItem instance for the menu item gets an active binding from the BindingService - the BindingManager then contains a filled HashMap of active bindings. Digging a bit deeper I found, that org.eclipse.jface.bindings.BindingManager.recomputeBindings() is called to initialize the BindingManager when a kex binding is queried from BindingService. At the time the toolbar contributions are evaluated there don't seem to be any active contexts yet, hence the empty activeBindings. Later in application initialization, recomputeBindings() gets called again from contextManagerChanged(ContextManagerEvent). This results in all the bindings being set as active and later to be found by the menu contribution. Could it be that the initialization code is in wrong order and the toolbar contributions should be evaluated after the active contexts are set? I'm not that deep in the platzform code, so I didn't know where to check for this. regards, Thorsten
(In reply to comment #4) > Could it be that the initialization code is in wrong order and the toolbar > contributions should be evaluated after the active contexts are set? I'm not > that deep in the platzform code, so I didn't know where to check for this. > Wow, you've already dug deeply into the platform code. I had hoped to avoid this, but it looks like CommandContributionItems will have to add themselves using org.eclipse.jface.bindings.BindingManager.addBindingManagerListener(IBindingManagerListener) There is no BindingManager currently exposed as API, so I'll need to do some investigation to find out where is a good place to make it happen. Then org.eclipse.jface.bindings.BindingManagerEvent.isActiveBindingsChangedFor(ParameterizedCommand) can be used by each CCI to see if its tooltip needs to be updated. PW
Created attachment 113819 [details] A patch for the bug Paul, I've attached a fix. Please review and let me know whether this is fine.
(In reply to comment #6) > Created an attachment (id=113819) [details] > A patch for the bug You are definitely heading in the correct direction. Good catch on that ExternalActionManager bug :-) Because CCI can see services, I would like to not use the ExternalActionManager. Instead look at exposing the BindingManager add/remove listener functionality through the IBindingService, similar to the pattern used for the CommandManager listener/ICommandService. In CCI then you would want code similar to org.eclipse.jface.action.ExternalActionManager.CommandCallback.bindingManagerChanged(BindingManagerEvent), where you check for the binding changes using event.isActiveBindingsChanged() and event.isActiveBindingsChangedFor(command) instead of the property change listener. Thanx, PW
(In reply to comment #7) > You are definitely heading in the correct direction. Good catch on that > ExternalActionManager bug :-) Paul, As per your suggestion, I'm looking at not using ExternalActionManager. That means the bug in that class would be irrelevant to this bug. I don't want to leave it as it is, so filed a bug separately (Bug#249574) and submitted a patch. For this bug I'll concentrate on the CCI and the API additions for now.
Created attachment 114173 [details] Patch adding the listerner for binding changes (In reply to comment #7) > Because CCI can see services, I would like to not use the > ExternalActionManager. Instead look at exposing the BindingManager add/remove > listener functionality through the IBindingService, similar to the pattern used > for the CommandManager listener/ICommandService. > > In CCI then you would want code similar to > org.eclipse.jface.action.ExternalActionManager.CommandCallback.bindingManagerChanged(BindingManagerEvent), > where you check for the binding changes using event.isActiveBindingsChanged() > and event.isActiveBindingsChangedFor(command) instead of the property change > listener. Paul, I've implemented your suggestions. Also I'm not a big fan of methods spawning several lines. So when adding the code for setting the tooltip, I've refactored the update method into smaller methods. Let me know whether this is fine or should revert it to the same old large method.
Good work. Here are some comments: 1) could you change the setToolTipText(*) method to a getToolTipText() method that can calculate the correct tooltip? Then you can use it in updateToolItem() and updateButton() 2) Instead of using JFaceResources.format(*) you should use org.eclipse.ui.internal.WorkbenchMessages for the workbench level i18n message. You'll have to create an equivalent method/property in messages.properties and then use org.eclipse.osgi.util.NLS.bind(String, Object, Object) 3) splitting up the method seems fine to me. PW
Created attachment 114290 [details] v02 + generic method getToolTipText() added (In reply to comment #10) > 1) could you change the setToolTipText(*) method to a getToolTipText() method > that can calculate the correct tooltip? Then you can use it in > updateToolItem() and updateButton() Done. Available in the attached patch. I didn't do this earlier, because the button's tooltip was not displaying the key bindings earlier! > 2) Instead of using JFaceResources.format(*) you should use > org.eclipse.ui.internal.WorkbenchMessages for the workbench level i18n message. > You'll have to create an equivalent method/property in messages.properties and > then use org.eclipse.osgi.util.NLS.bind(String, Object, Object) hmmmm. Not sure whether we should be using WorkbenchMessages here. Can you verify it again?
Created attachment 114330 [details] CCI v03 Your patch v02 looks good. I move the update*() methods around so they reflect the order of the if statements (shows up as fewer diffs) and added a null check (the UI Test Suites were failing with many errors). You are correct, WorkbenchMessages isn't really the right place. You should add an NLS class and properties file for the internal package that matches the public package: org.eclipse.ui.internal.menus (we don't generally put NLS classes in public packages, unless the intent is to make something i18n API). PW
Created attachment 114419 [details] v03 + NLS string instead of JFaceResources.format (In reply to comment #12) > You should add an NLS class and properties file for the internal package Done.
(In reply to comment #13) > Created an attachment (id=114419) [details] > v03 + NLS string instead of JFaceResources.format > Please respin this patch based on > Created an attachment (id=114330) [details] > CCI v03 it includes the update*() method order change, plus NPE protection (I have 29 failures + 712 errors in my UI Test Suites). Please run the tests before patch review. The NLS addition looks good. PW
Created attachment 114510 [details] v04 - patch based on v03 (In reply to comment #14) > Please respin this patch based on > > > Created an attachment (id=114330) [details] [details] > > CCI v03 > Done.
(In reply to comment #15) > Created an attachment (id=114510) [details] > v04 - patch based on v03 Looks good. Released to HEAD >20081008 We'll need to write some tests for this in the /org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/menus/MenusTestSuite.java PW
Created attachment 115137 [details] Test v01 (In reply to comment #16) > We'll need to write some tests for this in the /org.eclipse.ui.tests/Eclipse UI > Tests/org/eclipse/ui/tests/menus/MenusTestSuite.java > > PW > Done.
(In reply to comment #17) > Created an attachment (id=115137) [details] > Test v01 This failed with a junit.framework.ComparisonFailure: expected:<Testing Tooltip ([Alt+Shif]t+1)> but was:<Testing Tooltip ([Shift+Al]t+1)> on linux :-) I updated the code to use KeySequence (the format() calls should make it match on either platform). The patch included 2 parts from the dynamic contribution item patch (it didn't compiled, but I simply removed them). I simplified the test by one level, filling a ToolBarManager and then instantiating the ToolBar (and removing the toolbar element from the menuContribution). Released to HEAD >20081015 PW
Reassign
done
This bug seems very familiar to what we are seeing. In our case, the first editor that we open has an action in its toolbar which has tool tip e.g "Insert (Ctrl + Enter)" . Now you open the second editor for the same type of file and the key bindings disappear, e.g "Insert". This happens for pretty much all actions in the second editor's toolbar. Any subsequent editors have the same issue. Although, the commands execute fine in all editors if you use the associated key bindings. The only difference is that Isee this bug was found in 3.3.1 but we did not see this issue in 3.3.1. We just recently moved to 3.4.1 and this behavior started. I just want to make sure that this bug can cause the behavior described above. Can anybody please confirm?
