Community
Participate
Working Groups
Created attachment 221095 [details] Test projects to confirm the bug When the workbenach.xmi is loaded, an NPE is thrown. Debugging the code reveals, that the NPE is caused, because the element (the Part that is to be shown) does not have a parent (eContainer), so element.getParent() returns null. This seems to be the case for all parts that are added through a fragment. I have created a simple testcase to confirm the issue. 1) Start the application with the testapp.product in de.emsw.gosa.product.testapp. 2) Close the application 3) Start the application again and DO NOT clean the configuration, so workbench.xmi gets loaded See: http://www.eclipse.org/forums/index.php/m/911441/#msg_911441 !ENTRY org.eclipse.e4.ui.workbench 4 0 2012-09-11 17:33:18.756 !MESSAGE Exception occurred while rendering: org.eclipse.e4.ui.model.application.ui.basic.impl.PartStackImpl@1a4c5b4 (elementId: testapp.stack.right, tags: [], contributorURI: platform:/plugin/de.emsw.gosa.product.testapp) (widget: CTabFolder {}, renderer: org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer@1e2161d, toBeRendered: true, onTop: false, visible: true, containerData: 10000, accessibilityPhrase: null) !STACK 0 java.lang.NullPointerException at org.eclipse.e4.ui.workbench.renderers.swt.LazyStackRenderer.showTab(LazyStackRenderer.java:156) at org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.showTab(StackRenderer.java:1024) at org.eclipse.e4.ui.workbench.renderers.swt.LazyStackRenderer.postProcess(LazyStackRenderer.java:98) ...
*** Bug 389282 has been marked as a duplicate of this bug. ***
Created attachment 221096 [details] Proposed Bugfix in org.eclipse.e4.ui.model.internal.ModelUtils The problem arrises when elements are replaced with elements from fragments. If the objectToBeReplaced is also the selectedElement of an MElementContainer, the call to EcoreUtil.replace will leave the selectedElement without a parent (because it is still a reference to the objectToBeReplaced which has been removed from the container). This patch takes a direct approach and checks if the selectedElement also needs to be replaced. Maybe the replace should be handled more abstractly, i.e. by using ECrossReferenceList or something like that, so that all occurrences of the objectToBeReplaced are substituded?
Christoph, I had trouble applying the second part of the patch, the one patching PartServiceImpl. Anyway I don't think that is relevant to the bug. Paul, I reproduced the issue and the patch fixes it but I got no commit rights on Platform, just e4. For the moment I would apply it.
Yeah ... sorry ... the changes to PartServiceImpl where forced by the compiler (I didn't find the setting to turn off the error ... and didn't search long either ;-)).
Created attachment 221119 [details] Proposed Bugfix in org.eclipse.e4.ui.model.internal.ModelUtils using UsageCrossReferncer This patch uses a UsageCrossReferencer to find all References to the objectToBeReplaced and also sets the found References to the new element. This includes the feature selectedElement. Imho this approach is more elegant, since it handles all kinds of references and not just the feature selectedElement. Or can anyone think of a possible problem with this approach? (This time the patch does NOT include any changes to PartServiceImpl ;-))
If I'm not mistaken fragments SHOULD not get merged in if we launch from a persisted state! Tom
See Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=389663 In that case bug 389663 also solves this problem. Still ... maybe the patch would still make sense, i.e. if a model-element from the Application.e4xmi is replaced by an element with the same id from the fragment? If there is a replace case during fragment merging, it needs to be done all the way. Otherwise the framework should throw an exception, because there can never be a replace.
Duplicate: Bug 382184
Christoph, good pickup... Fixing references is necessary when *replacement* happens, whether it's done using EMF mechanisms or through direct manipulations should be determined based on performance IMO. Only if the EMF performance is really bad should we give up the general approach but it'd be nice to know (i.e. we only have a handful of parts but in the IDE (+ features) we can have 1000's of menu / tb items). The questions of whether a given fragment should be processed or not and when it should be processed are interesting; here's my take on a couple of the points. 1) Start with the expectation that the model you start with is the one saved on the last close. Check with Equinox to see whether there have been *any* changes to the extension registry, if not skip processing altogether. 2) If the contribution has already been processed (perhaps in a specific window, see below) then we should check whether it's the same 'version' (this may require more work to figure out but pretend we can do this...;-). Yes? skip it, it's already been processed. No? Merge the new snippet with the old. This is where the reference fixing comes in. Note that this cannot be simply container based since a part may have moved from its original container... Finally, it seems that we need a way to manage snippets that are supposed to be processed for (at least) new top level windows (i.e. a trim contribution). Here's we'd have to loop on the fragment for each window if we need processing on startup.
Everything part of the UI-Structure-Containment is never touched when coming from a restore (you can compare this to the IPrespectiveFactory in 3.x and the perspective extension point). Other stuff like MPartDescriptors, MCommands, MHandlers, MSharedElement, ... are merged in, things I'm undecided: * Menu * Toolbar here I'd expect that this elements are not moving or least not very often (in contrast to the MPart which moves around a lot).
*** Bug 382184 has been marked as a duplicate of this bug. ***
Created attachment 233309 [details] Patch based on R4_3_maintenance Hi Eclipse-Team I would realy like this bug to get fixed. I am migrating to kepler and I see that my patch has not been applied. Is there a reason why not? I don't see any danger in this small change: Original code: } else { // replace EcoreUtil.replace((EObject)existingEObject, (EObject)element); found = true; } My Suggestion: } else { // replace EObject root = EcoreUtil.getRootContainer((EObject) existingEObject); EcoreUtil.replace((EObject)existingEObject, (EObject)element); Collection<Setting> settings = UsageCrossReferencer.find((EObject) existingEObject, root); for (Setting setting : settings) { setting.set(element); } found = true; } So all I am saying is: If a model element gets replaced then also fix the references. (If this replacement should happen or not is not relevant for this issue.) I am also fine with the solution suggested by Eric in Bug #382184. I will attach a new patch based on the 4_3_maintenance branch.
Paul or Tom, could you comment on Christoph's patch? PW
The patch looks good to me. It passes all JUnits and my smoke tests Christoph: you need to do two things before we can commit it: 1) Sign a CLA (this is something new with Eclipse). Click the CLA icon next to your name in bugzilla for details. 2) Create the patch in Gerrit (preferably on master - we will apply it first to 4.4, and back port it to 4.3). Details are here: http://wiki.eclipse.org/Platform_UI/How_to_Contribute#Creating_a_patch Paul
(In reply to comment #13) > Paul or Tom, could you comment on Christoph's patch? > > PW The patch looks good to me
Paul: I think I successfully pushed the patch to gerrit https://git.eclipse.org/r/14685
Release on master (4.4) as https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=89472f2da05022ddb83f2d943909f73e888d94a5
Closing. Fix has been in 4.4 for some time.
Verified (visually) in 4.4.0.I20140528-2000.
