Community
Participate
Working Groups
Build Identifier: Version: 4.2.0 Build id: I20120503-1800 When adding a menu entry to the main menu of a RCP application with a model fragment, the new menu is added for every startup of the application (if the workspace is not cleared), which results in duplicate menu entries. The id of the main menu which will be extented is set to "com.example.e4.rcp.todo.mainmenu". fragment.e4.xmi: <?xml version="1.0" encoding="ASCII"?> <fragment:ModelFragments xmi:version="2.0" xmlns:xmi="http://www.omg.org/XMI" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:commands="http://www.eclipse.org/ui/2010/UIModel/application/commands" xmlns:fragment="http://www.eclipse.org/ui/2010/UIModel/fragment" xmlns:menu="http://www.eclipse.org/ui/2010/UIModel/application/ui/menu" xmi:id="_QpvUcJs7EeGwpbss216QnA"> <fragments xsi:type="fragment:StringModelFragment" xmi:id="_JYSDkJs8EeGwpbss216QnA" featurename="commands" parentElementId="com.example.e4.rcp.todo.application"> <elements xsi:type="commands:Command" xmi:id="_TIbH8Js8EeGwpbss216QnA" elementId="com.example.e4.rcp.todo.contribute.openmap" commandName="Say Hello"/> </fragments> <fragments xsi:type="fragment:StringModelFragment" xmi:id="_daBTkJs8EeGwpbss216QnA" featurename="handlers" parentElementId="com.example.e4.rcp.todo.application"> <elements xsi:type="commands:Handler" xmi:id="_rYKpcJs8EeGwpbss216QnA" elementId="com.example.e4.rcp.todo.contribute.openmap.handler" contributionURI="bundleclass://com.example.e4.rcp.todo.contribute.handler/com.example.e4.rcp.todo.contribute.handler.OpenMapHandler" command="_TIbH8Js8EeGwpbss216QnA"/> </fragments> <fragments xsi:type="fragment:StringModelFragment" xmi:id="_3_3w0Js8EeGwpbss216QnA" featurename="children" parentElementId="com.example.e4.rcp.todo.mainmenu"> <elements xsi:type="menu:Menu" xmi:id="_qGM10Js9EeGwpbss216QnA" label="Map"> <children xsi:type="menu:HandledMenuItem" xmi:id="_uibwkJs9EeGwpbss216QnA" label="Contribute Hello" command="_TIbH8Js8EeGwpbss216QnA"/> </elements> </fragments> </fragment:ModelFragments> Reproducible: Always Steps to Reproduce: 1. Create an RCP application and add a new menu via the model fragment shown above 2. Start the application twice without the clear workspace option 3. The menu entry will be added multiple times to the UI
Increasing prio
Created attachment 215452 [details] Exported plugins which reproduce the problem
For 4.2 it is up to the processor to decide if it needs to do anything, or if the model already contains changes. For example, see MenuThemeProcessor from "org.eclipse.e4.demo.contacts": @Execute public void process() { ... MApplication theApp = getApplication(); List<String> tags = theApp.getTags(); for(String tag : tags) { if (PROCESSOR_ID.equals(tag)) return; // already processed } ... // do whatever needs to be done ... tags.add(PROCESSOR_ID); super.process(); }
Ops, re-read and realized it is about fragments, not processors. Please ignore comment 3.
In the example menu and submenu don't have elementIDs. If you add elementIDs like this: <elements xsi:type="menu:Menu" xmi:id="_qGM10Js9EeGwpbss216QnA" elementId="com.example.e4.rcp.todo.contribute.openmap.menu" label="Map"> <children xsi:type="menu:HandledMenuItem" xmi:id="_uibwkJs9EeGwpbss216QnA" elementId="com.example.e4.rcp.todo.contribute.openmap.submenu" label="Contribute Hello" command="_TIbH8Js8EeGwpbss216QnA"/> </elements> then top menu is properly merged.
Tom, the ModelUtils#mergeList() has code to make sure elements with no ID are not considered equal. I guess that was intentional? I could remove that but I am not sure if it is a good idea to rely that much on EcoreUtil.equals().
(In reply to comment #6) > Tom, the ModelUtils#mergeList() has code to make sure elements with no ID are > not considered equal. I guess that was intentional? > > I could remove that but I am not sure if it is a good idea to rely that much on > EcoreUtil.equals(). Why are we not useing our xmi:id to check if an object is already there? This ID is guaranteed to be set.
At least if the normal ID is empty this sounds good. xmi:id in general will not work, as I can manually change my ID without adjusting xmi:id.
My suggestion for 4.2.0 would be to modify the tooling to show a warning if the elementId is not set. Is that acceptable?
(In reply to comment #9) > My suggestion for 4.2.0 would be to modify the tooling to show a warning if the > elementId is not set. Is that acceptable? That would be perfect, opened bug 379313 for that.
I saw lots of issues in my Eclipse 4 training, I suggest to make it maintenance required in the editor, see Bug 379304. I see no harm in having this default, having an ID creates no problem IMHO. Its a bit more effort for the user, but nasty follow up errors like this one will be avoided. If the user for some reason wants to remove the ID, he can edit the xmi file.
Created attachment 215519 [details] Preference page ... ever looked at preference page, folks ;-) Maybe now with the new model persistence stuff we should turn it on by default?
(In reply to comment #11) > I saw lots of issues in my Eclipse 4 training, I suggest to make it maintenance > required in the editor, see Bug 379304. > > I see no harm in having this default, having an ID creates no problem IMHO. Its > a bit more effort for the user, but nasty follow up errors like this one will > be avoided. If the user for some reason wants to remove the ID, he can edit the > xmi file. I'm against making it mandatory. We've take greate care in e4 that easy things are easy - if you really on want to create a simply application (no extensions, ...) you should not be forced to create the ID because it is of no use. There's currently one case an ID is manadatory which is for the command (because this is needed to bridge with the core.command stuff) and we should enforce it only there. I think the best solution is to turn the preference to autogenerate an id on by default.
@Tom: I must admit that I wasn't aware of this preference page. :-) +1 for changing the default to selected for autogenerated ID, I think that the perfect solution, the expert can change that, the beginner cannot make mistakes. Would you be ok with a patch with uses lower case for the generated ID's? Currently its for example: com.example.e4.rcp.todo.Part.0 and I think especially the prefix, e.g. com.example.e4.rcp.todo.Part., should be all lower cases. I also adjusted Bug 379304, so that only command ID should be mandatory.
Go ahead your are a committer, create a patch and push it to master - the tooling is still in incubation so you don't have to follow the strict rules. If you are not sure, add a patch and I'll push it ;-)
Hi, Auto generating an id by default sounds good for me. And lower case only too. The original bug was about contributed menus and for me auto generating ids for menus or menu entries does not work. May be a new bug? There is simply no id generated for "HandledMenuItem". I think the removed need to enter an id manually for every element is a big advantage of e4 over e3 and therefor a mandotory id should be combined with default auto generation in my opinion. Felix
Created attachment 215566 [details] Patch making the generated id lowercase
Target milestone: 4.3 to figure out if we need 2 sets of IDs and to improve model elements matching.
Information from the e4 chat - for the notes: - can't we use xmi:id ? - consider adding a warning to ModelAssember for elements with empty IDs - as to why we have two sets of IDs: "we added the xmi:id because of our old merge story. elementId is only guaranteed to be unique in a branch of a tree. would be better named elementName though"
Opened Bug 379460 for the lower case ID's.
Tom, since we no longer support the 'deltas' approach can we now do away with the magic xmi:id ? This would clean up the model a lot for folks that are looking at the file itself in a text editor (the GUID's are a bit on the scarey side).
