Community
Participate
Working Groups
As of some milestone after M3, SHIFT-ALT-O does not work at all.
Works for me using plain Eclipse SDK 3.2 RC1.
I think something was hosed in my GNOME. I apologize. I restarted GNOME, and it works fine, now.
Just for correcness sake: this bug is a real issue -- but it's a duplicate of 135535.
Sorry for the back and forth. I upgraded to RC3, and the problem is still there (linux, gtk2). A typical reproducing scenario is: 1) start Eclipse against my project 2) Test ALT-SHIFT-O. It works. 3) F11 (and run the last test -- a simple unit test). 4) ALT-SHIFT-O does not work anymore No entries are generated in the logm by the way. Thanks, Z
*** This bug has been marked as a duplicate of 135535 ***
That bug 135535 is marked as closed, and the reporter verified the problem is fixed, but I still have *this* problem with RC3 (and I can't reopen that bug).
Test Case: 1. open A.java ==> Alt+Shift+O works 2. open B.java ==> Alt+Shift+O does not work ==> it only works in the very first editor. Same problem with the 'Show Selected Element Only' command if a key binding is assigned to it. Since day one we register our global action BasicTextEditorActionContributor.IActionBars, IWorkbenchPage) follows: bars.setGlobalActionHandler(ITextEditorActionDefinitionIds.TOGGLE_SHOW_SELECTED_ELEMENT_ONLY, fTogglePresentation); bars.setGlobalActionHandler(IJavaEditorActionDefinitionIds.TOGGLE_MARK_OCCURRENCES, fToggleMarkOccurrencesAction); We didn't change that code. It looks like a regression in the key binding / command story in Platform UI which got introduced after 3.2 M4. Marking as 'major' since this affects all clients that register their action in that way. Note that the action handler itself is still present under its registered ID but the key binding got lost.
Created attachment 41170 [details] Workaround for the two Java editor actions The attached patch works around the bug by hammering the identical global action handler into the action bar every time the a new editor is set. This does no harm and fixes the problem for the two JDT Text actions. The bug should still be investigated for 3.2 since other clients might be affected. Martin, Mike please cast your vote.
I'd rather we actually fixed the real problem, than apply the patch, but the workaround in the patch seems pretty innocuous. I'll investigate the real problem, but +1 to applying the workaround in the mean time.
+1 to release the workaround in the mean time.
It looks like the back end handler of the legacy action is including a "too specific" expression ... i.e. it's tied the to the part, when it should be tied to that partId. We're investigating a fix for it, but it will not be included in RC4. PW
Agree that this is P2, and I'd really like to see it fixed for R3.2.
Verified in I20060512-0010 that the workaround brings back the two broken key bindings for 'Toggle Mark Occurrences' and 'Show Selected Element Only'.
I am not sure I understand the workaround. can somebody please explain the difference?
Sure, but let me first correct the scrambled part of comment 7: >Since day one we register our global action >BasicTextEditorActionContributor.IActionBars, IWorkbenchPage) follows: should read: Since day one we register our global action BasicJavaEditorActionContributor.init(IActionBars, IWorkbenchPage) follows: The problem seems that the command framework looses (or explicitly clears?) the key binding for the action that got registered in init(...) even though the action is still there i.e. when calling actionBars.getGlobalActionHandler(myActionId) you get back your action but the key binding is not hooked. Setting the global action handler again when setActiveEditor(IEditorPart) is called hammers back the key binding.
Created attachment 41357 [details] right direction workbench patch v01 This patch is for looking, not for touching! The problem was the editor action bars were being tied to the service locator for the first editor part. Any actions that were registered were being evaluated if that first editor part was active. The right direction is to step back a level, tie the editor action bars to workbench window level service locator, and add an EditorActionBarExpression that evalutes to true when any editor of that type is active. This has highlighted some strange behaviour in the SlaveHandlerService that needs careful examination. PW
Created attachment 41492 [details] editor action bar patch v02 This fixes the problem with Toggle Mark Occurences. It ensures that EditorActionBar actions (that are turned into handlers) are activated with the correct expression (Part ID active and Workbench Window active). It adds a protected API method to org.eclipse.ui.SubActionBars to expose the expression used to active handlers. I don't believe this will cause a downstream problem, as all of the code dealing with services and service locators and expressions is new in 3.2. This fix is restricted to the org.eclipse.ui.workbench plugin. PW
John, Boris, could I please get you to review "editor action bar patch v02" Thanx, PW
I discussed this with Paul, and have checked out the patch. It seems to be doing something reasonable, but I'm still somewhat distressed about the API addition. John and Boris will both have to say that it's an acceptable risk, and we need to do full disclosure on the mailing list.
I don't fully understand the patch yet, but I found a debugging statement (call to System.out.println) in LegacyEditorActionBarExpression.evaluate() that needs to be removed. Paul, can we go through the patch together tomorrow?
Guys: It's _too_late_ to be putting out patches with bogus print statements in them.
Created attachment 41588 [details] editor action bar patch v03 (with print removed :-) Dani, could I also get you to review this patch, it's on org.eclipse.ui.workbench. PW
This patch provided a negligible improvement in the editor performance tests. PW
I took another quick look at the patch, but don't have time to go through it thoroughly. I do want this fixed though, so I will +1 for the pmc if you can get 3 other component leads to review.
I will look at the patch tomorrow first thing in the morning. How good is the test coverage for the area that gets changed by this patch?
Paul, I would like to go over the patch with you, but could you fix a few things first to minimize the size of the patch to review noise for reviewers: - Remove the change to PartSite. This appears to be just extra info in the toString for debugging purposes. If you think this is important, please explain. - Remove the change in in SubActionBars.EXPRESSION - this appears to just be a resorting of the methods, but it adds noise to the patch Also, I noticed in the two fields you added, you adopted the convention of adding the "f" prefix (fEditorHandlerExpression and fActiveEditorId). This should be made consistent with UI conventions (ditch the "f").
Created attachment 41637 [details] Alternate fix without API addition Just throwing this out as an idea, but here is a slight twist on Paul's latest patch that avoids the API addition. I think we should avoid adding API at this stage unless we believe it is absolutely essential, and I'm not sure it is in this case. This change removes the added API method SubActionBars.getHandlerExpression and downcasts to obtain the correct handler for the EditorActionBars case. Not elegant, but the API could be added in the next release and this downcast removed.
We have done hacks like the one John describes in other places. That would be a reasonable way to proceed as long as we have a TODO to fix the code once we ship.
(In reply to comment #26) > - Remove the change to PartSite. This appears to be just extra info in the > toString for debugging purposes. If you think this is important, please > explain. This one is important debugging information for tracing. It's necessary for tracking down bugs like this one and bug 135535 (I actually meant to release it with 135535 but it slipped through the cracks). PW
Reviewed John's version of the patch since we shouldn't add new API at this point if not absolutely needed. The patch makes sense and I verified that it fixes the problem but like John, I'd also like to go over it together with Paul. Paul, John, can we do this directly after today's arch call (reuse the phone line)? Comments to the code itself: - serviceLocator is now only set in Editor/SubActionBars and setServiceLocator(IServiceLocator) is now never called by Eclipse SDK code - I would add "editorHandlerExpression= null;" to EditorActionBars.dispose() - LegacyEditorActionBarExpression constructor: I'd throw an IAE and not an NPE if the argument is null ( - LegacyEditorActionBarExpression.equals(...) could directly compare the two activeEditorId which are guaranteed to be non-null instead of calling Expression.equals(...) which performs unneeded null-checks.
Created attachment 41713 [details] editor action bar patch v05 - no API Updated the non-API patch with Dani's comments: - I would add "editorHandlerExpression= null;" to EditorActionBars.dispose() - LegacyEditorActionBarExpression constructor: I'd throw an IAE and not an NPE if the argument is null ( - LegacyEditorActionBarExpression.equals(...) could directly compare the two activeEditorId which are guaranteed to be non-null instead of calling Expression.equals(...) which performs unneeded null-checks. Plus added the hashCode debugging back. PW
Mike Valenta, could I please get you to review this patch? Thanx, PW
+1
The patch ("editor action bar patch v05 - no API") looks good. I have reviewed it together with Paul.
Talked to Paul about latest patchv05. Approving for 3.2 RC3.
I have also reviewed patch v05 with Paul and Boris. +1 for RC5.
Released into HEAD >20060518 PW
After use in RC5, eventually the toggle mark occurrences stopped working with a: HANDLERS >>> Unresolved conflict detected for 'org.eclipse.ui.edit.text.toggleShowSelectedElementOnly' HANDLERS >>> resolveConflicts: eval: HandlerActivation(commandId=org.eclipse.jdt.ui.edit.text.java.toggleMarkOccurrences,handler=ActionHandler(org.eclipse.jdt.internal.ui.javaeditor.ToggleMarkOccurrencesAction@1beb199),expression=AndExpression(LegacyEditorActionBarExpression(org.eclipse.jdt.ui.CompilationUnitEditor),WorkbenchWindowExpression(org.eclipse.ui.internal.WorkbenchWindow@19fe451)),sourcePriority=278529) HANDLERS >>> resolveConflicts: eval: HandlerActivation(commandId=org.eclipse.jdt.ui.edit.text.java.toggleMarkOccurrences,handler=ActionHandler(org.eclipse.jdt.internal.ui.javaeditor.ToggleMarkOccurrencesAction@8ee5c0),expression=AndExpression(LegacyEditorActionBarExpression(org.eclipse.jdt.ui.CompilationUnitEditor),WorkbenchWindowExpression(org.eclipse.ui.internal.WorkbenchWindow@19fe451)),sourcePriority=278529) HANDLERS >>> Unresolved conflict detected for 'org.eclipse.jdt.ui.edit.text.java.toggleMarkOccurrences' PW
Created attachment 42049 [details] Confilct for toggle mark occurrences log containing conflict PW
This happens when you have a couple of java editors open, do a CTRL+SHIFT+W, and then open some more java editors. The dispose() of the EditorActionBars is calling setActiveEditor(null), but it's interacting badly with the workaround in BasicJavaEditorActionContributor#setActiveEditor(*). Opening a new java editor creates a new EditorActionBars, which also adds its own activation and causes the conflict. All of the other calls to setGlobalActionHandler(*) go through getAction(*) which returns null if the active editor is null. If we remove the workaround in BasicJavaEditorActionContributor // XXX: Workaround for Platform UI bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=137091 then this should fix it. PW
Can you explain why it's not working with the workaround in place? AFAIK there's no need to explicitly clear a global action handler and setGlobalActionHandler will be called when switching to the next Java editor. It sounds like removing the workaround is again a workaround ;-)
So the problem is that in the dispose it's calling setGlobalActionHandler() ... after the call to super.dispose(). The super.dispose() is what cleans up all of the action handlers that EditorActionBars knows about. It's a leak. It's is correct to remove it. PW
OK, I see. The problem is different from the one reported in this bug: after closing the last Java editor and then reopening a new one breaks the key binding. This is caused by the workaround (bug 137091). I suggest to close this bug again and fix bug 137091 for RC6.
Re comment 44: I mentioned the wrong bug. The bug which mentions the workaround is bug 142266.
Just for the records: the workaround worked in 3.2 RC4 when this bug was in place.
I've resolved this and am monitoring the change of bug 142266 Re-open this bug as needed :-) PW
