Community
Participate
Working Groups
Created attachment 256135 [details] example e4 app with processor Currently, the ModelAssembler#processModel(boolean) method processes the model elements in this order: - processors marked to run before fragments are executed - fragments are processed - processors marked to run after fragments are executed - imports are resolved Because the imports are resolved last, a ClassCastException is thrown when trying to get the container of the model elements from a processor: Caused by: java.lang.ClassCastException: org.eclipse.e4.ui.model.fragment.impl.ModelFragmentsImpl cannot be cast to org.eclipse.e4.ui.model.application.ui.MUIElement at org.eclipse.e4.ui.internal.workbench.ModelServiceImpl.getContainer(ModelServiceImpl.java:850) at TestModelProcessingOrder.ExampleProcessor.execute(ExampleProcessor.java:18) I'm attaching an example e4 app with a processor that can be used to reproduce the problem. Is there a particular reason why the imports are resolved last?
So there's a bug in that getContainer() shouldn't CCE since we should be able to have arbitrary containers. It does seem strange that the post-fragment processors operate on a semi-complete model. Change the ordering seems legit, and indeed it's what I assumed how the operations were ordered. The impact is that a post-fragment processor cannot alter the elements loaded from a fragment. But I'm not sure a fragment is appropriate in that situation.
We will provide a fix for this one, actually both, the order issue and the CCE
New Gerrit change created: https://git.eclipse.org/r/67335
I pushed the fix for the model processing order, based on the refactored version of the ModelAssembler.
I opened bug 488478 for the problem with the getContainer method of the model service.
Jonas, technically this is a change in API (since order of processing is changed) and requires PMC approval before being committed.
Re "Jonas, technically this is a change in API (since order of processing is changed)" -- Is the processing order actually specified in the API, or are we just worrying about the possibility of there being existing code that relied on the old order? If it's the latter, then I'd like to understand what we've done to communicate the change to the community and get feedback on impact.
I am not aware that the current order is communicated somehow. IMHO, the current order does not make sense at all. Actually, it is more the opposite case: The API allows that processors can be executed AFTER fragments, In fact, without the fix, those processors are executed somewhere in the middle of the fragment merging, before their imports get resolved. About existing users: It is hard for me to envision a real use case, where somebodies code would not work after this fix. It would be: - Element A is in a fragment to be merged - Element B is reference by A and is in the core model or another fragment and therefore an import in the fragment of A Now you need an after-fragment-processor, which accesses A after merging, retrieves B from it and then assume, that A is still the import proxy, but not yet the resolved real element. Another theoretical use case would be to programatically add imports to the fragment after it has been merged. But it would be hard to access the fragment at that point in time. To sum up: It is hard to imagine, that someone relies on the current order. Of course, you never know. I therefore would suggest to add this to the N&N?
One more comment: Personally, I would not consider this to be an API change, although you could argue on that...
I agree that if we haven't communicated the order, then it's not an API change. I also agree that the current order seems broken. My worry was just whether we've given our consumers enough time to react to the change, if we believed that there is working code out there that could depend on the current behaviour. It sounds like the answer to that is no.
I missed that the ModelAssembler is an internal class. That said, the order is in the javadoc, it has been there for several releases, and it's a behavioural change. But the actual behaviour is weird and requires the developer to write special-case code to handle the weird intermediate state. I think posting to e4-dev and the Eclipse 4 Forum pointing to this bug would be sufficient.
So do we agree that this does not require PMC approval? In this case, I would accept the fix, and create the posts Brian mentioned.
The old order does not make sense, +1 for changing that. I think we need to discuss that in the next PMC call.
(In reply to Lars Vogel from comment #13) > The old order does not make sense, +1 for changing that. I think we need to > discuss that in the next PMC call. I'll put it in the agenda for Tuesday.
This change has been approved by the PMC. See https://wiki.eclipse.org/Eclipse/PMC Jonas, you can commit the change.
(In reply to Lars Vogel from comment #15) > Jonas, you can commit the change. Please ensure you announce it widely. I would guess that posting to cross is not necessary. I leave that to Dani to correct, if that is also required. I suggest that you follow Brians suggestion: --- I think posting to e4-dev and the Eclipse 4 Forum pointing to this bug would be sufficient. ---
Mass move to 4.6 RC1. We might push out more to 4.7.
Gerrit change https://git.eclipse.org/r/67335 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6ff98a1fa99203354800319d25697d482423467a
Verified in I20160427-2000
