Comment 1 Dani Megert 2019-04-26 10:45:00 EDT

Can you attach an example or provide exact steps?

Comment 2 Stefan Nöbauer 2019-04-29 02:49:06 EDT

Created attachment 278423 [details]
Test Application

Test Application with one Fragment Bundle.

Comment 3 Stefan Nöbauer 2019-04-29 03:02:28 EDT

I also tried the apply="notexists" which I would like to use for my fragments in case of updates and additional UI Elements.

But with this setting somehow the "org.eclipse.e4.ui.internal.workbench.ModelAssembler" has different IDs to check against. It seams that the ID Map contains the IDs from the defined Model in the xmi files and the stored model has different IDs where no ui element is found in the Map of the Assembler.

Comment 4 Stefan Nöbauer 2019-04-30 07:52:34 EDT

Created attachment 278441 [details]
Test Application (with correct Product)

Comment 5 Dani Megert 2019-05-01 09:03:39 EDT

I can't reproduce this using 4.12 M1:
https://download.eclipse.org/eclipse/downloads/drops4/S-4.12M1-201904110625/

If you can reproduce with 4.12 M1 or newer, please reopen and provide more detailed steps.

Comment 6 Stefan Nöbauer 2019-05-02 03:09:58 EDT

Created attachment 278460 [details]
Screenshot 1

Comment 7 Stefan Nöbauer 2019-05-02 03:10:32 EDT

Created attachment 278461 [details]
Screenshot 2

Comment 8 Stefan Nöbauer 2019-05-02 03:10:52 EDT

Created attachment 278462 [details]
Screenshot 3

Comment 9 Stefan Nöbauer 2019-05-02 03:11:11 EDT

Created attachment 278463 [details]
Screenshot 4

Comment 10 Stefan Nöbauer 2019-05-02 03:11:33 EDT

Created attachment 278464 [details]
Screenshot 5

Comment 11 Stefan Nöbauer 2019-05-02 03:12:04 EDT

I still get the same Problem with 4.12.
Here some more info and steps to reproduce:

<extension
	 id="TestAppFragment.fragment"
	 point="org.eclipse.e4.workbench.model">
  <fragment
		apply=""
		uri="fragment.e4xmi">
  </fragment>
</extension>


1. Launch TestApp with "Clear" Workspace Data
[Screenshot 1]
[Screenshot 2]

2. Move part "Hallo Fragment" to top PartStack
[Screenshot 3]

3. Close App
4. Launch TestApp with "Don't clear" Workspace Data
[Screenshot 4]
[Screenshot 5]

5. Close and Launch TestApp again
App throws the Exception and App startet with default Application Model.

Tried different apply model:
"apply=EMPTY": 		behaviour like described above
"apply=always":		behaviour like described above
"apply=notexists":	No Exception but duplicate part
"apply=initial":	Works as expected

Comment 12 Dani Megert 2019-05-02 06:24:57 EDT

OK, I can reproduce it with the Hallo Fragment using https://download.eclipse.org/eclipse/downloads/drops4/I20190501-1800/ and a new workspace, however, I do not get any exceptions.

Comment 13 Stefan Nöbauer 2019-05-02 07:30:39 EDT

(In reply to Dani Megert from comment #12)
> OK, I can reproduce it with the Hallo Fragment using
> https://download.eclipse.org/eclipse/downloads/drops4/I20190501-1800/ and a
> new workspace, however, I do not get any exceptions.

In the test application from the attachment the apply policy is "notexists".
Switch that to either "EMPTY" or "always"

Comment 14 Stefan Nöbauer 2019-05-21 09:38:48 EDT

(In reply to Stefan Nöbauer from comment #3)
> I also tried the apply="notexists" which I would like to use for my
> fragments in case of updates and additional UI Elements.
> 
> But with this setting somehow the
> "org.eclipse.e4.ui.internal.workbench.ModelAssembler" has different IDs to
> check against. It seams that the ID Map contains the IDs from the defined
> Model in the xmi files and the stored model has different IDs where no ui
> element is found in the Map of the Assembler.

The check about the IDs is in the org.eclipse.e4.ui.internal.workbench.ModelAssembler at line 342.

if (checkExist && applicationResource.getIDToEObjectMap().containsKey(r.getID(o))) {
	continue;
}

It seems that new IDs will be generated on loading the persisted model. Therefore the newly generated IDs are not equals to the ones in the fragment.e4xmi

Why are new IDs generated?

Comment 15 Eclipse Genie 2019-05-23 11:30:03 EDT

New Gerrit change created: https://git.eclipse.org/r/142676

Comment 16 Jonas Helming 2019-05-24 06:46:21 EDT

Thank you for the contribution! Did you have a look at BR 487748? I think it is a duplicate and basically reports that "notexist" is not working at all. the current documentation of "notexist" says "only if the given element does not exist already in the model", so that is already what you want. So I would suggest to nor introduce another flag as you did, but fix the "notexist" instead. To do this, I suggest that you keep the line:

"fragment.getElements().removeAll(existingElements);"

But basically remove everything else related to "notInApp". Further I would then adapt your tests. please note that there is a test for 487748 called "testFragments_existingXMIID_checkExists()" that is currently ignore due to this bug. It would be great, if you could "unignore" it to check whether the other bug is fixed then, too.

Thank you again for reporting and contributing!

Comment 17 Eclipse Genie 2019-05-29 10:55:12 EDT

Gerrit change https://git.eclipse.org/r/142676 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=df1b1868f23e06b557f2804c840538f9a2d16bcb

Comment 18 Andrey Loskutov 2019-05-29 11:02:36 EDT

(In reply to Eclipse Genie from comment #17)
> Gerrit change https://git.eclipse.org/r/142676 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=df1b1868f23e06b557f2804c840538f9a2d16bcb

Are you sure? Jonas, we are at RC1 time and can't merge anything without approval.

Comment 19 Eclipse Genie 2019-05-29 11:06:27 EDT

New Gerrit change created: https://git.eclipse.org/r/143029

Comment 20 Eclipse Genie 2019-05-29 11:06:41 EDT

Gerrit change https://git.eclipse.org/r/143029 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=235f6c4cc79b51d741409c440e3d65f01747a0e2

Comment 21 Dani Megert 2019-05-29 11:09:37 EDT

I am sorry but I had to revert this as it violates the endgame rules. For details see https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_12.php, especially:

"All fixes submitted must have a project lead or PMC vote on the bug report,"

You can either look for approval or move it to 4.13.

Having said that, thanks very much for your contribution.

Comment 22 Jonas Helming 2019-05-29 11:51:11 EDT

Yeah, I am sorry for that. I would vote for moving to 4.13, is this OK for you Stefan?

Comment 23 Stefan Nöbauer 2019-05-29 15:33:53 EDT

Hi, 
We would need this fix very urgent for our next release of our product in August. Is there a change to get the PMC vote for the 4.12. Release? I would appreciate that a lot!

Best
Stefan

Comment 24 Dani Megert 2019-05-30 03:30:04 EDT

(In reply to Stefan Nöbauer from comment #23)
> Hi, 
> We would need this fix very urgent for our next release of our product in
> August. Is there a change to get the PMC vote for the 4.12. Release? I would
> appreciate that a lot!
> 
> Best
> Stefan
It is very late to make such a change in RC2 and there's always room for regressions. I don't know that code well enough, so, I won't -1 if another PMC member or project co-lead approves it. I think Lars is most qualified here as he made several changes to that class in the past.

Comment 25 Stefan Nöbauer 2019-05-30 06:26:17 EDT

(In reply to Dani Megert from comment #24)
> (In reply to Stefan Nöbauer from comment #23)
> > Hi, 
> > We would need this fix very urgent for our next release of our product in
> > August. Is there a change to get the PMC vote for the 4.12. Release? I would
> > appreciate that a lot!
> > 
> > Best
> > Stefan
> It is very late to make such a change in RC2 and there's always room for
> regressions. I don't know that code well enough, so, I won't -1 if another
> PMC member or project co-lead approves it. I think Lars is most qualified
> here as he made several changes to that class in the past.

I can understand that but IMHO this fix is very separated to a "notexists" feature that is broken anyways. Lars what di you think?

Comment 26 Thomas Schindl 2019-05-31 04:05:30 EDT

To me the change looks ok but I'm not sure we should break our rules and bring in stuff so late. 

BTW there's a work around available by replacing the resource loading with a patched version (we had done this in efxclipse in the past while waiting for upstream fixes)

Comment 27 Lars Vogel 2019-05-31 04:24:05 EDT

Stefan, this came really too late for 4.12. I suggest to register a custom resource handler to solve this. 

We can review/merge it early 4.13

Comment 28 Rolf Theunissen 2019-06-01 17:00:21 EDT

Bug 487748 duplicates Bug 440030

Comment 29 Lars Vogel 2019-06-11 05:54:37 EDT

(In reply to Jonas Helming from comment #22)
> Yeah, I am sorry for that. I would vote for moving to 4.13, is this OK for
> you Stefan?

Jonas, 4.13 is open, please merge again if the patch is still OK for you.

Comment 30 Eclipse Genie 2019-06-11 08:27:13 EDT

New Gerrit change created: https://git.eclipse.org/r/143714

Comment 31 Rolf Theunissen 2019-06-11 08:45:30 EDT

(In reply to Eclipse Genie from comment #30)
> New Gerrit change created: https://git.eclipse.org/r/143714

Looking at this gerrit and the gerrit attached to Bug 440030, https://git.eclipse.org/r/34817, this bug is a duplicate of Bug 440030.

Comment 32 Jonas Helming 2019-06-12 05:26:11 EDT

Created attachment 278902 [details]
An example application to create a partial merge

Make sure, you do not clear the workspace nor turn on -clearpersistedState

Launch the application, open toolbar item should be merged from the fragment and work.

Close the application

Open workspacelocation/runtime-testFragment.product/.metadata/.plugins/org.eclipse.e4.workbench/workbench.xmi
in a text editor

delete the toolbar item "testFragment.handleditem.trimbar.top.open" simulating that a former contribution is removed and therefore does not exist anymore.

restart the application. The toolbar item should be merged again but the reference should point to the command which has been merged before. Toolbar action should work as before then.

Comment 33 Stefan Nöbauer 2019-06-12 05:32:52 EDT

(In reply to Jonas Helming from comment #32)
> Created attachment 278902 [details]
> An example application to create a partial merge
> 
> Make sure, you do not clear the workspace nor turn on -clearpersistedState
> 
> Launch the application, open toolbar item should be merged from the fragment
> and work.
> 
> Close the application
> 
> Open
> workspacelocation/runtime-testFragment.product/.metadata/.plugins/org.
> eclipse.e4.workbench/workbench.xmi
> in a text editor
> 
> delete the toolbar item "testFragment.handleditem.trimbar.top.open"
> simulating that a former contribution is removed and therefore does not
> exist anymore.
> 
> restart the application. The toolbar item should be merged again but the
> reference should point to the command which has been merged before. Toolbar
> action should work as before then.

Thanks Jonas,

I try to add a test that is covering this usecase.

Comment 34 Jonas Helming 2019-06-12 05:40:58 EDT

Without thinking it completly through, I like the idea that Rolf brought up to use the "import" meachnism for this. Imports are created, if an element in the fragment points to an element that already exists in the application model. In our use case, elements in the fragment that already exists in the application model are a similar case. So preparing the fragment in a way that element which will not be merged are converted to imports seems like a pragmatic approach. You might want to look at the "Extract into fragment" action of the editor, because it calculates the imports for elements when extracted.

Again, I haven't thought this completly through, just wanted to drop some thoughts.

Comment 35 Stefan Nöbauer 2019-06-21 08:17:20 EDT

Hi
so i added a new Test to simulate the partly merge.

I would like to push a new Change as I don't like the Revert of a Revert!

Comment 36 Eclipse Genie 2021-08-17 12:57:53 EDT

This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.