Community
Participate
Working Groups
Created attachment 221120 [details] Test projects to confirm the bug Suppose you start with this simple application model: - Application (ID: app) - TrimmedWindow (ID: window) - PartSashContainer (ID: sash1) - PartStack (ID: stack1) - PartStack (ID: stack2) With some contributions in a fragment: - StringModelFragment (ID: stack1, feature: children) - Part (ID: part1) - StringModelFragment (ID: stack2, feature: children) - Part (ID: part2) - Part (ID: part3) After loading the base model and the fragments, the application model looks like this: - Application (ID: app) - TrimmedWindow (ID: window) - PartSashContainer (ID: sash1) - PartStack (ID: stack1) - Part (ID: part1) - PartStack (ID: stack2) - Part (ID: part2) - Part (ID: part3) After the application is started the user can drag/drop part3 and drop it in stack1 next to part1. Closing the application, this will be persisted in workbench.xmi: - Application (ID: app) - TrimmedWindow (ID: window) - PartSashContainer (ID: sash1) - PartStack (ID: stack1) - Part (ID: part1) - Part (ID: part3) - PartStack (ID: stack2) - Part (ID: part2) So far, so good. The problem arises on the next start of the application. First, the model from workbench.xmi is loaded (as above). Next, all the fragments are loaded and the contributions are added/replaced in the model. Currently, this is achieved by merging the contributions into the parent defined by the StringModelFragment. Since part3 is contributed to stack2 and stack2 does not have a child with that id, the part will be added to the list. The resulting model looks like this: - Application (ID: app) - TrimmedWindow (ID: window) - PartSashContainer (ID: sash1) - PartStack (ID: stack1) - Part (ID: part1) - Part (ID: part3) - PartStack (ID: stack2) - Part (ID: part2) - Part (ID: part3) This is not what was intended and leads to all sorts of problems. The issue can be reprocued with the attached projects: 1) Start the application with testapp.product in de.emsw.gosa.product.testapp 2) Drag/Drop "Part Four" from the right next to "Part One" on the left 3) Close the application 4) Start the application again WITHOUT clearing the configuration, so workbench.xmi is loaded
Created attachment 221121 [details] Proposed Bugfix in org.eclipse.e4.ui.internal.workbench.ResourceHandler This patch solves the issue by NOT loading fragments into the model, when a persisted model is present (workbench.xmi). This seems to be the way to go, since workbench.xmi already includes all elements from the fragments.
(In reply to comment #1) > This patch solves the issue by NOT loading fragments into the model, when a > persisted model is present (workbench.xmi). This seems to be the way to go, > since workbench.xmi already includes all elements from the fragments. New bundles that are installed will never have their fragments applied. I wonder if we shouldn't instead record and filter based on the ids of the incorporated fragments.
@Brian: With "new bundles" are you refering to (a) bundles beeing installed at runtime or do mean (b) bundles that have been added to the configuration after the first start? Or did I miss your point all together? (a) Support for this would have to be added through a different mechanism (i.e. listening to osgi events). The code piece in question, ResourceHandler.loadMostRecentModel(), is called during startup. (b) This is a question of model evolution. For us, this is a very important topic. In this case the framework should also be able to handle changed fragments (new, deleted, changed model contributions in existing fragments) as well as removed fragments (or processors).
*** Bug 393132 has been marked as a duplicate of this bug. ***
any easy solution would be to allow only: * execute on new * execute always This would probably solve the most common use case for RCP who don't allow to install new features once deployed if they allow they can simply wipe the restored state.
Raising to major - we need to have a solution in 4.3 for this. The work-around for 4.2.x is to using processors. Dependending on how we implement it maybe we can even include that in 4.2.2
Created attachment 222995 [details] patch to allow developers to spec what to do with contribution
It might be better to reopen bug 393132 and applying the patch because this bug more specifically ask for better mergeing. In this defect the next step would be to make the merge much more intelligent. Instead of searching for the element it would have to first check if the element is not already part of the restore model (it can use the XMI-ID for this - Eric I guess this is a real use case for XMI-ID!!!!!) and replace it in its containment-feature in the parent it is currently in. Christoph: Does the attached patch adress your immediate use case? I guess it does because you can now turn of merging when you come from a restore state. To stay backward I've for now set "always" as the default but I'm very inclined to make "cleanstate" the default. More states could be added in future (e.g. once, modified) but for that we'll have to track old fragments (e.g. remembering an MD5-SUM and recording which fragment an element came from) Comments? If we go with the current patch we won't break existing apps (i guess most of them didn't even notice what happens) so can we do such an exsd change in 4.2.2? I think this is a major flaw in our current RCP story and the earlier we can fix it the better. Paul, Eric?
Thomas: Your patch solves my immediate use case as you suspected. I verfied that using execute=cleanstate for the fragments also fixes this bug. Thank you for looking into this! I am all in favor for a "merge when changed" option.
Comment on attachment 221121 [details] Proposed Bugfix in org.eclipse.e4.ui.internal.workbench.ResourceHandler Obsolete with the patch from Thomas
(In reply to comment #8) > > If we go with the current patch we won't break existing apps (i guess most > of them didn't even notice what happens) so can we do such an exsd change in > 4.2.2? I think this is a major flaw in our current RCP story and the earlier > we can fix it the better. Paul, Eric? The fundamental problem is we're continually processing fragments when we don't want to be, right? I want to just add the part of your patch (we can do it under bug 393132 since that sounds good) that prevents running the fragments if we're loading a previously saved (and so constructed) model. We don't need to add to the schema, and in 4.3 we can perhaps look at loading the model and applying new fragments case (or removing fragments from plugins that have been uninstalled). Or is there a hidden downside? Is the fix for Bug 382184 still useful on its own? PW
the patch from #382184 is not needed if we go with this patch.
IMHO we should allow that fragments are processed several times and just search for the ID. I'm concerned that if we change this now we will loose the dynamics of adding fragments at runtime. Also AFAIK we have no API to trigger the reconstruction of the application model and its hard for users to add command line flags. Maybe we can add a global parameter which allow to define if fragments are spplied on top of a saved application model?
(In reply to comment #13) > > Maybe we can add a global parameter which allow to define if fragments are > spplied on top of a saved application model? For the longer term this bug will address the better fragment processing story w/ merging. But in 4.2.2 fragments will be processed on model creation that first time. Processors are for recurring model manipulation on startup. PW
Marking critical because this bug leads to runtime problems when used together with min/max! See newsgroup thread
(In reply to comment #13) > IMHO we should allow that fragments are processed several times and just > search for the ID. I'm concerned that if we change this now we will loose > the dynamics of adding fragments at runtime. Also AFAIK we have no API to > trigger the reconstruction of the application model and its hard for users > to add command line flags. > > Maybe we can add a global parameter which allow to define if fragments are > spplied on top of a saved application model? I think the above is a first step, we can add more merging logic and type later on, I've currently only implemented the 2 most important ones!
Tom, can you give me a link to the newsgroup thread ? This sounds similar to what I had to do when enabling Trim dragging; never remove the old TB's on shutdown (to prederve their current position) but on a start first check to see if they're there...only create one if not.
We really need to address this problem in M7. It is causeing too many issues.
I think we have 2 categories: a) rendered model artifacts b) templates (part-descriptors, snippets) and we need to deal with them differently: * elements of a) are only added if they don't already exist * elements of b) are always merged in
That sounds about right. What do you mean by 'merged in' when we're talking about snippets ?
Hi, Is there any news on this? What can I do to avoid either not having any layout changes stored (by using clearPersistedState) or having my menuItems (defined in e4xmi files) multiply with every restart? The patch in comment #7 seems to address this, but I don't see it applied in the code. Thanks for any help.
targeting for 4.4 - I hope i can find the OSS-Cycles to implement a solution
Tom, do you want to re-asses this for 4.4? PW
yes please put it on my plate
(In reply to Paul Webster from comment #11) > > I want to just add the part of your patch (we can do it under bug 393132 > since that sounds good) that prevents running the fragments if we're loading > a previously saved (and so constructed) model. Hi, this is closely related to bug 376486 I am working on. I was unaware of this bug but I have seen this behavior as well. I indeed resolved it by not loading fragments and processors for an already constructed workbench. That seems to work fine. The drawback is that one needs to run with -clearPersistedState to get new fragments and processors in.
Paul can we get this in, maybe even in M7? https://git.eclipse.org/r/22826
(In reply to Thomas Schindl from comment #26) > Paul can we get this in, maybe even in M7? > > https://git.eclipse.org/r/22826 We can ask for an exception in M7 for this if we want. It's seems pretty risky, as we're asking for the extension contributor to guess how they want their fragment applied. PW
why guess - we keep the current state but give them more opportunities: * always - status quo * initial - only when coming from an initial model * notexits - if we can not find the root element in the model
Dani, Paul, John - what's your opinion I think this a really critical feature for e4 rcp applications I think contributors very well know how they want their fragment to be merged.
(In reply to Thomas Schindl from comment #29) > Dani, Paul, John - what's your opinion I think this a really critical > feature for e4 rcp applications I agree with Tom, this is a critical feature for RCP apps.
We can look at this right after EclipseCon PW
(In reply to Paul Webster from comment #31) > We can look at this right after EclipseCon Once we update the EP documentation then we can make a case to include this change to the PMC PW
I'll update the docs tomorrow and provide and upload a new gerrit patch
i've uploaded a new review to https://git.eclipse.org/r/24062
Paul are you ok with the latest review? Can we push this to the PMC to get approval?
Sorry, while the bundle has API this is at the top of the schema: <strong>This is not an API. The schema for this extension point is not frozen and might change in a non-backward compatible way in the future.</strong> I'll go ahead and push this. PW
We might have to revisit this in Mars, but for now it's available as opt-in in the extension point: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=92a9cb0ecc1150c962342cc6e184785845b6fce4 PW
*** Bug 469229 has been marked as a duplicate of this bug. ***
