Bugzilla – Full Text Bug Listing
| Summary: | Wrong selection used to find the handler (was: Branch deletion command in "Branches" dialog broken in Juno under egit 2.1.0.201207070015) | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | R Shapiro <rshapiro> |
| Component: | UI | Assignee: | Paul Webster <pwebster> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | bsd, daniel_megert, david_williams, emoffatt, john.arthorne, manuel.doninger, pdolega, pwebster, robin |
| Version: | 4.2 | Flags: | john.arthorne:
pmc_approved+
bsd: review+ |
| Target Milestone: | 4.2.1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
R Shapiro
Same problem here, but i get the following NPE as root cause: Caused by: java.lang.NullPointerException at org.eclipse.egit.ui.internal.repository.tree.command.RemoveCommand.removeRepository(RemoveCommand.java:78) at org.eclipse.egit.ui.internal.repository.tree.command.DeleteCommand.execute(DeleteCommand.java:22) at org.eclipse.ui.internal.handlers.HandlerProxy.execute(HandlerProxy.java:293) at org.eclipse.ui.internal.handlers.E4HandlerProxy.execute(E4HandlerProxy.java:76) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:56) However it seems strange to me that RemoveCommand is called, when i want to delete a branch. The problem is that it uses the selection of the view to find the handler instead of the selection in the dialog. I suspect that the wrong context is used when trying to find the handler for the 'org.eclipse.ui.edit.delete' command.
NOTE: The handler is registered for the command on a given node in the 'plugin.xml' file:
<handler
commandId="org.eclipse.ui.edit.delete">
<class
class="org.eclipse.egit.ui.internal.repository.tree.command.DeleteBranchCommand">
</class>
<activeWhen>
<and>
<count
value="+">
</count>
<iterate>
<and>
<or>
<instanceof
value="org.eclipse.egit.ui.internal.repository.tree.RefNode">
</instanceof>
<test
property="GitRepository.isLocalBranch">
</test>
</or>
<not>
<test
property="GitRepository.isRefCheckedOut">
</test>
</not>
</and>
</iterate>
</and>
</activeWhen>
</handler>
Paul, Eric, there are quite a lot of similar looking bugs. It would be good to investigate this for 4.2.1. My guess is that this is caused by bug 384425. Unfortunately Oleg's away. (In reply to comment #5) > My guess is that this is caused by bug 384425. Unfortunately Oleg's away. That bug talks about the editor ID, hence I would not assume that it also affects a view. (In reply to comment #6) > That bug talks about the editor ID, hence I would not assume that it also > affects a view. My mistake: I (mistakenly) thought handler-lookup was done using getActive(), which is the topic of bug 384425. The handler being resolved in 4.x is a DeleteCommand which subclasses RemoveCommand. But in 3.8 it's a DeleteBranchCommand that's obtained. The problem here is that the handler enablement expressions aren't being evaluated using the IEvaluationContext provided by EGit. Daemon Thread [Thread-1] (Suspended (breakpoint at line 112 in LegacyHandlerService$HandlerSelectionFunction)) LegacyHandlerService$HandlerSelectionFunction.compute(IEclipseContext) line: 112 ValueComputation.get() line: 60 EclipseContext.internalGet(EclipseContext, String, boolean) line: 229 EclipseContext.internalGet(EclipseContext, String, boolean) line: 238 EclipseContext.get(String) line: 200 HandlerServiceImpl.lookUpHandler(IEclipseContext, String) line: 45 HandlerServiceImpl.executeHandler(ParameterizedCommand, IEclipseContext) line: 159 LegacyHandlerService.executeCommandInContext(ParameterizedCommand, Event, IEvaluationContext) line: 554 CommonUtils.runCommand(String, IStructuredSelection) line: 119 CheckoutDialog$3.widgetSelected(SelectionEvent) line: 205 1. EGit's CommonUtils provides an IEvaluationContext with the dialog selection and calls IHandlerService#executeCommandInContext() 2. LegacyHandlerService#executeCommandInContext() creates an IEC to wrap the provided EvaluationContext (staticContext) and calls HandlerServiceImpl#executeHandler() with said staticContext. 3. HandlerServiceImpl#executeHandler() uses getExecutionContext() (which returns the active leaf of the workbench IEC = the dialog IEC) to lookup the appropriate handler via #lookUpHandler(), which triggers HandlerSelectionFunction. 4. HandlerSelectionFunction selects a handler using the lookup IEC to seed the IEvaluationContext. So the HandlerSelectionFunction uses the dialog context to resolve "org.eclipse.ui.edit.delete" rather than the EGit-provided evaluation context. The dialog IEC's selection resolves to the workbench selection, and not the branch selection set from EGit, and so the DeleteCommand is chosen rather than DeleteBranchCommand. The solution is for HandlerSelectionFunction#compute() to somehow use the same contexts used in execution, and in particular to obtain the IEvaluationContext provided. *** Bug 383536 has been marked as a duplicate of this bug. *** It seems that bug 387615 is caused by the same or a similar problem. There no handler is found at all. Is that right? (In reply to comment #9) > It seems that bug 387615 is caused by the same or a similar problem. There > no handler is found at all. Is that right? Yes, if the button press also invokes a command. (In reply to comment #7) > > The solution is for HandlerSelectionFunction#compute() to somehow use the > same contexts used in execution, and in particular to obtain the > IEvaluationContext provided. Yes, thank you for that evaluation, that's the problem. I've pushed a fix to master for this, http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3c7f1f384d4de226f52410c7d0c88d72a5498be2 Brian, could you please review it for us in 4.2.1? Fix does 2 things: 1) Look to the IEvaluationContext default variable and create one in the ExpressionContext, which doesn't have a default variable that can be set 2) Use the passed in context to request and evaluate the HandlerSelectionFunction. Thanx, PW It isn't a perfect patch, but it probably deals with the majority of the cases as it does ensure that the context used for handler lookup at least has the default variable set. But the handler lookup will still miss any other variables that were provided in the IEvaluationContext. I wonder if the lookupContext loop in executeCommandInContext(), which sets the default variable, could also set the IEvaluationContext on the loopContext?
The only change I'd request is to the patch to CommandProvider to add a comment to doReset(): the call to dispose() to the ExpressionContext's eclipseContext comes out of nowhere. A comment as to why this is appropriate (since ExpressionContexts are created using a provided IEC, rather than the IEC being created as part of the ExpressionContext creation) would be helpful. I'd personally rather that ExpressionContext be created with a flag to indicate that the IEC should be disposed.
@@ -109,6 +110,9 @@ public class CommandProvider extends QuickAccessProvider {
protected void doReset() {
idToElement = null;
+ if (currentSnapshot instanceof ExpressionContext) {
+ ((ExpressionContext) currentSnapshot).eclipseContext.dispose();
+ }
currentSnapshot = null;
}
}
I did not (yet) review the patch but can confirm that it fixes the reported problem. Not that time for SR1 is running out and you will need PMC approval on the final patch. I've pushed a new patch to pwebster/bug384545-2 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=pwebster/bug384545-2&id=560039fc8d181c769389daa9da1e5da00c650170 The other patch created IEclipseContexts and they had the potential not to be disposed. IEvaluationContext doesn't have dispose in its lifecycle. This patch takes a different approach. Snapshot contexts are EvaluationContexts, just as they always were. Executing a command in context will create a snapshot IEclipseContext in they runtime hierarchy, so looking up the handlers will be correct. It copies over the IEvaluationContext variables so they're available for lookup ... the cost is some reflection in 4.2.1 as we don't generally expose the variable map contained in EvaluationContext. Dispose of IEclipseContext is handled in the executeCommandInContext(*) method where it is created. PW (In reply to comment #14) > I've pushed a new patch to pwebster/bug384545-2 > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=pwebster/ > bug384545-2&id=560039fc8d181c769389daa9da1e5da00c650170 That should be pwebster/bug384545-3 and http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=pwebster/bug384545-3&id=60500d7e8d2d3542813521e9d1112df663051ea8 PW Dani, is this important enough to make it into 4.2.1? PW Looks good! My only tweak would be to rename getContextVariables() to getContextVariablesField() as it took a few reads to understand what getVariables() was doing. (In reply to comment #17) > Looks good! My only tweak would be to rename getContextVariables() to > getContextVariablesField() as it took a few reads to understand what > getVariables() was doing. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=pwebster/bug384545-4&id=de77c3deab6502886fbd675bf534d9587584b2d3 is the same as comment #15 but includes the change to getContextVariablesField() PW The patch is a bit more complicated than I like for RC3, but the bug is quite severe so it is worth the trade-off. Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e74da6e100666157419095f7a6249c2838c00bf6 PW (In reply to comment #20) > Released as > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=e74da6e100666157419095f7a6249c2838c00bf6 John, Eric, there's a problem with this patch. contextFVariables = EvaluationContext.class.getField("fVariables"); //$NON-NLS-1$ should have a contextFVariables.setAccessible(true); immediately after it. Do I request a kill of the build? PW I've pushed a fix. PW verified in M20120905-1640 PW (In reply to comment #16) > Dani, is this important enough to make it into 4.2.1? > > PW Yes. I agree with bug 384545 comment 19 and verified that the fix works in 4.2-M20120905-2300 together with the latest EGit (2.1.0.201209051725). |