Bugzilla – Full Text Bug Listing
| Summary: | [Commands] [Compat] Git-Commit Dialog destroys IEclipseContext by calling IEvaluationContext#addVariable | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Thomas Schindl <tom.schindl> | ||||||||||||
| Component: | UI | Assignee: | Paul Webster <pwebster> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Paul Webster <pwebster> | ||||||||||||
| Severity: | major | ||||||||||||||
| Priority: | P3 | CC: | carolynmacleod4, pwebster, remy.suen | ||||||||||||
| Version: | 4.2 | ||||||||||||||
| Target Milestone: | 4.2 M4 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Thomas Schindl
Tom, do you only get this problem with EMF's wizards? I don't see to have this problem with the regular 'File' or 'Folder' wizards. Also, which view were you using? It is NOT reproduceable all the time (just launched 4.2 and this time the dialog points to the correct project) but somehow one can manage to get into this situation and have no idea how. I've been in the Package-Explorer yesterday when this happened to me. I should have opened the ContextView to see where the problem is located but it somehow looks like we can get into a state where the Package Explorer Selection is not synced appropriately. It's possible that this is a new manifestation of bug 333506. Tom, do you remember if any other actions were "stuck" on a selection? (In reply to comment #3) > It's possible that this is a new manifestation of bug 333506. > Not unlikely. > Tom, do you remember if any other actions were "stuck" on a selection? Sorry no :-( Ok I think I now know how to reproduce it. One needs to bring up the git-commit dialog an afterwards the selection in the New Ecore Modelwizard is pointing the the location you used when doing the git commit. So there are the steps i used to reporduce it: a) Edit some file so that it gets dirty (I used org.eclipse.e4.tools.emf.editor3x.XMIResourceFunction) b) Bring up the context menu on the file in Package Explorer c) Team > Commit ... d) Cancel commit e) Select another project in Package Explorer f) Bring up Context Menu on it g) New > Other ... > Ecore Model Watch the dialog selecting the commit directory as the default location Created attachment 203605 [details]
screenshot
So here are my findings: After having opened the egit-commit dialog the Package Explorer Part-Context has set the "selection" which IMHO is wrong because the selection variable should be set only on the perspective. The only value that should change in the PartContext is output.selection. Once the "selection" is set the popup:-Context does not resolve any more the selection in the perspective but the one from the Part which gets passed through the handler system and injects the wrong value into our wizards. So what we need to findout is how the egit commit dialog is able to set the selection variable in the Parts context. (In reply to comment #7) > So here are my findings: After having opened the egit-commit dialog the Package > Explorer Part-Context has set the "selection" which IMHO is wrong because the > selection variable should be set only on the perspective. The only value that > should change in the PartContext is output.selection. Ups looke like the selection is managed on the window level. Ok I found the location where the "selection" variable is set into the IEclipseContext of the PartImpl.
It is found in RepositoryAction and looks like this:
public void run(IAction action) {
IServiceLocator locator = getServiceLocator();
ICommandService srv = (ICommandService) locator
.getService(ICommandService.class);
IHandlerService hsrv = (IHandlerService) locator
.getService(IHandlerService.class);
Command command = srv.getCommand(commandId);
ExecutionEvent event = hsrv.createExecutionEvent(command, null);
if (event.getApplicationContext() instanceof IEvaluationContext) {
((IEvaluationContext) event.getApplicationContext()).addVariable(
ISources.ACTIVE_CURRENT_SELECTION_NAME, mySelection);
}
try {
this.handler.execute(event);
} catch (ExecutionException e) {
Activator.handleError(e.getMessage(), e, true);
}
}
Created attachment 203763 [details]
expression context class
Because creating patches using egit is not possible I've attached a ExpressionContext-Class which stores additional informations using an extra map.
One thing I find strange here is that because we are always using the active leaf to look up values the getParent() hiearchy is not making a whole lot of sense, not?
Created attachment 203768 [details]
Patch
now as a patch
(In reply to comment #11) > Created attachment 203768 [details] > Patch I discussed this patch with Tom on IRC and it does what we want it to do. A ExpressionContext (var defined) | B IEvaluationContext implementation (var defined) | C ExpressionContext (var undefined) | D ExpressionContext (var defined) If we ask C for a value, with the patch we'd get B, which is what we'd expect. Pre-patch right now D is returned (where we assume D's IEclipseContext is the active child of C's IEclipseContext). This is suspect as I don't think IEvaluationContext's contract implies that it knows anything about its children. I brought up the issue where we may want to check whether a null value was explicitly entered into the map but since the default implementation of IEvaluationContext in EvaluationContext does not care about this (since its addVariable(String, Object) method prevents nulls), it was deemed unnecessary. Though we should probably enhance ExpressionContext to also prevent nulls? Created attachment 203778 [details]
fixes the lookup
patch which also fixes the parent look ups as described by remy
You have to contrast that against what is providing the original ExpressionContext to clients. In most of the ExpressionContexts we create, we have access to the application context or the window context, but for the purposes of evaluation we need the legacy "application context". Our equivalent in e4 is context.getActiveLeaf(). Perhaps whenever we expect to hand out a context, we need to create an ExpressionContext and then an EvaluationContext child, and hand that out? It will see the application context through the parent, without setting any variables in leaf contexts. PW *** Bug 359105 has been marked as a duplicate of this bug. *** Changed platform to All, because I am experiencing this on Windows. It's definitely serious, because I can't do any git from my Package Explorer because it seems to think some random file is selected. [...]
> Perhaps whenever we expect to hand out a context, we need to create an
> ExpressionContext and then an EvaluationContext child, and hand that out? It
> will see the application context through the parent, without setting any
> variables in leaf contexts.
>
Well the problem with creating a childcontext is that we don't know when to destroy it not, so we'll produce a "context leak". Can't we release the attached patch and leave the bug open until Paul is back?
It seems like we'd also have to change LegacyHandlerService's executeCommandInContext(*) method. If the caller passes in something that's an ExpressionContext, we just spawn a child context off of its root context. What this means is that any variables that the caller had previously inserted into that evaluation context is ignored. We should look at this in the context of re-evaluating our current approach to command evaluation I think... Paul - is there any thing planned to fix this defect - this really makes working with Eclipse 4.2 and git cumbersome I still don't see why we aren't creating an ExpressionContext and then an EvaluationContext and then handing that out. An EvaluationContext does not have to be disposed. PW (In reply to comment #21) > I still don't see why we aren't creating an ExpressionContext and then an > EvaluationContext and then handing that out. An EvaluationContext does not > have to be disposed. > > PW Why does an EvaluationContext NOT has to be disposed, wouldn't we create a memory leak then? I have admit I'm not an expert in the command area but from what i understand the whole thing is hierarchical which something the current implementation is also not doing (see remy's comment). (In reply to comment #22) > Why does an EvaluationContext NOT has to be disposed, wouldn't we create a > memory leak then? The 3.x EvaluationContext doesn't have a dispose lifecycle. Any state was supposed to be used and then let go (GCed) as it would not necessarily be valid for the next use. Technically its parent would hold the ExpressionContext which would hold an IEclipseContext ... we would have to be careful not to use a child IEclipseContext because we would not be able to guarantee when it should be disposed. PW But how do you fix the problem of modification without createing a new IEclipseContext which you can't release because IEvaluationContext has no defined lifecycle (well you can when the object finalizer is run but that sounds like not a very good idea). Ah ok I think now I got your idea:
we have something like this:
-------8<-------
ContextEvaluationContext {
private IEclipseContext context
}
ExpressionEvaluationContext {
private Map variables;
ExpressionEvaluationContext(ContextEvaluationContext parent) {
super(parent);
}
}
-------8<-------
When we instantiate the LegacyHandlerService we create an ExpressionContext to wrap it that we use for a # of things.
When we use createExecutionEvent(*) we also create an ExpressionContext that wraps our main IEclipseContext, which leads to the variable being set in our running context (and I believe that's the behaviour we don't want).
So what if we do:
public ExecutionEvent createExecutionEvent(Command command, Event event) {
EvaluationContext legacy = new EvaluationContext(evalContext, evalContext.getDefaultVariable());
ExecutionEvent e = new ExecutionEvent(command, Collections.EMPTY_MAP, event, legacy);
return e;
}
They'll set variables into an EvaluationContext, which doesn't effect our IEclipseContext hierarchy at all.
PW
This is not true because someone might call
IEvaluationContext expressionContext = ....
expressionContext.getParent().setVariable("selection",....)
So we really need to make modifying the IEclipseContext backing an IEvaluationContext impossible.
(In reply to comment #27) > This is not true because someone might call > > IEvaluationContext expressionContext = .... > expressionContext.getParent().setVariable("selection",....) I'm willing to let them hang themselves here, as in general this pattern would by them nothing in 3.x. I realize it's a risk, but until we hit it we need to only add as much indirection as necessary to avoid adding even more to our performance problems. PW (In reply to comment #28) > (In reply to comment #27) > > This is not true because someone might call > > > > IEvaluationContext expressionContext = .... > > expressionContext.getParent().setVariable("selection",....) > > I'm willing to let them hang themselves here, as in general this pattern would > by them nothing in 3.x. I realize it's a risk, but until we hit it we need to > only add as much indirection as necessary to avoid adding even more to our > performance problems. > > PW What about we: a) issue a warning message to the log b) or issues a warning and don't store informations I'll try to come up with a patch. Tom Created attachment 207037 [details]
Pass out EvaluationContexts
I should be able to release this for tonight's build. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4fb6b460fc55fb7fe1d79aa4c641542833aecc5e PW In I20111205-2330 |