Community
Participate
Working Groups
I have several plugins that I use in several RCP applications (both compatibility and native e4). I need to have 2 fragments for each plugin for application elements because my applications have 2 different ids. org.eclipse.e4.ide.application and org.eclipse.e4.legacy.ide.application Perhaps I am missing something, but what would work better than my current approach (maintaining 2 identical fragments would be to do one of the following" 1. Allow to specify multiple fragment parent IDs: org.eclipse.e4.ide.application | org.eclipse.e4.legacy.ide.application 2. Or to have another fragment element that would let me specify the main fragment (org.eclipse.e4.ide.application) and then a list of mirrored elements to also consider (org.eclipse.e4.legacy.ide.application, this.app, that.app, etc)
Yes this is something that I have been working on as well. https://github.com/E4Examples/importworkbenchbridge See also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=376486
I'd like to see the xpath support for contributing enhanced and finally included, as that would provide a way to meet most of these use cases. PW
(In reply to Paul Webster from comment #2) > I'd like to see the xpath support for contributing enhanced and finally > included, as that would provide a way to meet most of these use cases. > > PW We already have the model in memory and have functions to query it. What would we gain by adding xpath?
(In reply to Wim Jongman from comment #3) > (In reply to Paul Webster from comment #2) > > I'd like to see the xpath support for contributing enhanced and finally > > included, as that would provide a way to meet most of these use cases. > > > > PW > > We already have the model in memory and have functions to query it. What > would we gain by adding xpath? @Paul, do you mean specify the target for the contribution as an XPath expression?
(In reply to Wim Jongman from comment #3) > > We already have the model in memory and have functions to query it. What > would we gain by adding xpath? This works against the in-memory model. XPath offers a more expressive way to pin point a contribution as opposed to simply parentId. See http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org.eclipse.e4.emf.xpath for the integration with EMF. PW
(In reply to Paul Webster from comment #5) > (In reply to Wim Jongman from comment #3) > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org. > eclipse.e4.emf.xpath for the integration with EMF. See also Bug 324954.
I started working on a patch for ModelUtils.java First, let me explain my use case. I wanted to create 1 menu, and add it to the main menu bar, and also into a part's view menu. I also want to define a single part and contribute it to multiple part stacks. Before getting involved in resolving complex XPath expressions, I started with a very simple one: IDs separated by '|'. This should be backwards compatible when a full fledged XPATH resolver is in place. If multiple targets exist, I use EcoreUtil.copy to clone the contributed element, so each target gets a unique copy. There are probably some bad side affects to this approach, so I will await feedback.
Created attachment 246137 [details] screenshot: The fragment with multiple targets
Gerrit Review: https://git.eclipse.org/r/#/c/31905/
(In reply to Steven Spungin from comment #7) > Before getting involved in resolving complex XPath expressions, I started > with a very simple one: IDs separated by '|'. This should be backwards > compatible when a full fledged XPATH resolver is in place. This is the wrong direction, as I mentioned before. It's a problem that applies in a lot of locations (like MMenuContributions only have one parent id but could apply to multiple locations) and solving it piece meal in only one location isn't really a good idea. That being said, I'm open to discussing a shorter term fix that moves us in the right direction without having to wait for a larger feature. w.r.t. the patch it should use comma's as separators for the IDs, that's our standard separate used and we don't support comma's in IDs. PW
@Paul, there are two separate issues here 1. How we encode the target 2. How we deal with cloning or reusing contributions. I am totally open to ANY implementation in both cases. I am just finding it extremely difficult to maintain my applications with the current spec. I chose the '|' instead of a comma because previous comments mentioned XPath, and that is the token used in that query language. I cannot find a reference to what is valid ElementID syntax. I would appreciate blessing/guidance on the following items: In reference to item #1 Create a popup textbox editor to create/view the query, as expressions might get quite long and benefit from code completion, syntax hilighting, and further tooling. Allow a single id, CSV id list, or XPath expression (If I knew what was valid for ElementID, we might be able to ignore the xpath prefix) org.eclipse.e4.legacy.ide.application org.eclipse.e4.legacy.ide.application , org.eclipse.e4.ide.application xpath:@id='org.eclipse.e4.ide.application' | @id='org.eclipse.e4.legacy.ide.application' In reference to item #2 Please review the 'cloning procedure' and advise if there is a better way to duplicate a contribution. I am not familiar enough with the code to predict side-effects. I am trying to find a solution that is backwards compatible, yet a valid subset of any future development. XPath would be nice, but just having a simple list would solve many of my issues. Thanks, and looking forward to your direction!
What about trying your merge(*) modification with the XPath option. Instead of the split, add the org.eclipse.e4.emf.xpath bundle to the manifest and at the top of your method check for "xpath:" and if that's there, strip it off and use: String parentExpression = getParentElementId().substring(6); XPathContextFactory<EObject> f = EcoreXPathContextFactory.newInstance(); XPathContext xpathContext = f.newContext((EObject)application); Iterator<Object> i = xpathContext.iterate(parentExpression); while (i.hasNext()) { Object obj = i.next(); if (obj instanceof MApplicationElement) { MApplicationElement o = (MApplicationElement) obj; // stuff .... } } Then this should work: xpath:.[@id = 'org.eclipse.e4.legacy.ide.application' | @id = 'org.eclipse.e4.ide.application'] PW
Thanks for the snippit Paul. There is not much documentation on the emf.xpath library and it saved me much time. Issues org.eclipse.e4.emf.xpath has dependencies to org.apache.jxpath 1.2 I ended up replacing that bundle with org.apache.servicemix.bundles.commons-jxpath;bundle-version="1.3.0". The commons-jxpath bundle then required org.apache.commons.beanutils and org.apache.commons.collections. Once I was dependency free, I tried the expression. There seems to be an issue with the XPath parser. This works xpath://*[@elementId='app.menu.primary'] | //*[@elementId='app.menu.secondary'] This does not xpath://*[@elementId='app.menu.primary' | @elementId='app.menu.secondary'] My fragment contributed using legacy ID, the new ID list, and the xpath expression. Very nice! I can take a look at the xpath code and fix the logic issue (and add some tests...), but I am not sure if I should check in the code with the dependency to org.eclipse.e4.emf.xpath et al. I can shove the apache libs into that bundle; Is Eclipse Foundation OK with that? Perhaps they are in another repo have not pulled?
Created attachment 246225 [details] screenshot: 3 ways to specify fragment element id app.menu.primary app.menu.primary,app.menu.secondary xpath://*[@elementId='app.menu.primary'] | //*[@elementId='app.menu.secondary'] This expression does not work yet xpath://*[@elementId='app.menu.primary' | @elementId='app.menu.secondary']
This expression DOES work (using 'or' instead of '|') xpath://*[@elementId='app.menu.primary' or @elementId='app.menu.secondary'] The read me file in org.eclipse.e4.emf.xpath states it includes 3rd party bundles, but it does not. I removed the dependency and added 3 Apache jars to the /lib folder but did not update the copyright file yet. Add least the code is now self contained and can be easily downloaded and tested. I will remove them and re-add the dependency if an RCP app includes them elsewhere. Hopefully nobody is currently targeting an elementId that starts with 'xpath:' :)
I was able to add a direct menu item to every view menu using this: xpath://menus[tags='ViewMenu'] The JXPath 1.3 from orbit works, so I am going to remove the libs that were added and add the dependency back. --- There are issues with namespaces, e.g. //*[xsi:type="menu:Menu" and tags='ViewMenu'] does not work, and is why I used //menus. Perhaps we should hardcode the namespaces prefixes listed in the document into the XPath resolver? xmlns:xmi="http://www.omg.org/XMI" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:advanced="http://www.eclipse.org/ui/2010/UIModel/application/ui/advanced" xmlns:application="http://www.eclipse.org/ui/2010/UIModel/application" xmlns:basic="http://www.eclipse.org/ui/2010/UIModel/application/ui/basic" xmlns:menu="http://www.eclipse.org/ui/2010/UIModel/application/ui/menu" xmlns:ui="http://www.eclipse.org/ui/2010/UIModel/application/ui"
Last comment regarding namespaces is not very relevant as we are searching the class object and not the xmi document; I am just getting used to JXPath. With that said, something like //*[class/interfaces='foo.IBar'] does not work and I am trying to target a specific interface type. My solution was to register a custom function: //.[efx:instanceOf('foo.IBar')] Any suggestions on this are welcome. In the meantime I updated the ContextFactory to allow adding custom functions. This now works: (will contribute to every object that implements MMenu) xpath://.[efx:instanceOf('org.eclipse.e4.ui.model.application.ui.menu.MMenu')] Gerrit Id has changed https://git.eclipse.org/r/#/c/32506/
(In reply to Steven Spungin from comment #16) > I was able to add a direct menu item to every view menu using this: > xpath://menus[tags='ViewMenu'] > Why is this not sufficient? See Tom's examples in Bug 324954. We're walking the contained EMF objects by their attribute name, not their type. I guess emf.xpath doesn't surface a way to do more general type checks. I'd like to avoid adding the ability to add arbitrary functions, at least initially. PW
(In reply to Paul Webster from comment #18) > (In reply to Steven Spungin from comment #16) > > I was able to add a direct menu item to every view menu using this: > > xpath://menus[tags='ViewMenu'] > > > > Why is this not sufficient? See Tom's examples in Bug 324954. We're > walking the contained EMF objects by their attribute name, not their type. > I guess emf.xpath doesn't surface a way to do more general type checks. > > I'd like to avoid adding the ability to add arbitrary functions, at least > initially. > > PW I though it was intuitive to have the InstanceOf function given we are searching an object and not an xml document and the EMF way is to use interfaces. I only figured out the //menus path because I had set a breakpoint and dug into the object. Rather than hardcoding another method in the emf.xpath library, I though it wise to implement the ability for any developer to add functions to their queries. I think it will be useful having functions to iterate/target the model. The implementation was trivial; We don't need to expose it as API just yet, but JXPath allows be useful in debugging. Do you want me to comment out the code? Make it private? Put it in another branch? Please advise.
(In reply to Steven Spungin from comment #19) > (In reply to Paul Webster from comment #18) > > I'd like to avoid adding the ability to add arbitrary functions, at least > > initially. > > > Do you want me to comment out the code? Make it private? Put it in another > branch? Please advise. I did not look into the code yet, but let us know once you followed Pauls suggestion so that we can have a look at the code again,.
Code has been stripped down. See syntax in the comment for the merge() function. This will only change existing behavior if the parentId starts with 'xpath:'
(In reply to Steven Spungin from comment #21) > Code has been stripped down. See syntax in the comment for the merge() > function. > > This will only change existing behavior if the parentId starts with 'xpath:' Thanks Steven, looks very good IMHO. It will be extremely useful for extender of the IDE and of RCP applications. I would be good to add this to the existing ResourceHandlerTest test or to write a new for this. AFAIK we need to add the two required bundles to our SDK distribution to make this work at runtime. I opened Bug 462000 to track this. Once this is done, we can merge your development.
Created attachment 251508 [details] one use case of what this will solve For every plugin fragment in my library, I need to target 2 application models as shown in the screenshot. With this patch I will only have to maintain 1 list.
> I would be good to add this to the existing ResourceHandlerTest test or to > write a new for this. AFAIK we need to add the two required bundles to our > SDK distribution to make this work at runtime. I opened Bug 462000 to track > this. Once this is done, we can merge your development. 1. Should I submit the tests as a separate patch? 2. Where is the best place to put post documentation for end-users? There seems to be several options. Also, is is appropriate to enter 'write documentation for ...' as a bug?
(In reply to Steven Spungin from comment #24) > > I would be good to add this to the existing ResourceHandlerTest test or to > > write a new for this. AFAIK we need to add the two required bundles to our > > SDK distribution to make this work at runtime. I opened Bug 462000 to track > > this. Once this is done, we can merge your development. > > 1. Should I submit the tests as a separate patch? You can add them if you like, or write another one. > 2. Where is the best place to put post documentation for end-users? There > seems to be several options. Also, is is appropriate to enter 'write > documentation for ...' as a bug? vogella.com (just kidding). If we add this, we should add a N&N entry for it. I think the whole fragment stuff is not yet documented at Eclipse.org.
I have to move this to M7, because of Bug 422102. Once we got IP approval this can go in. Hopefully beginning of M7.
This is a RCP feature highly requested by RCP customers. It allows to define a model extension and contribute it to RCP applications without specifying the exact target ID, similar to what we have in plugin.xml based extensions. Dani, please give PMC approval.
(In reply to Lars Vogel from comment #27) > This is a RCP feature highly requested by RCP customers. It allows to define > a model extension and contribute it to RCP applications without specifying > the exact target ID, similar to what we have in plugin.xml based extensions. > > Dani, please give PMC approval. What are the changes that you request approval for?
(In reply to Dani Megert from comment #28) > What are the changes that you request approval for? https://git.eclipse.org/r/#/c/32506/
(In reply to Lars Vogel from comment #29) > (In reply to Dani Megert from comment #28) > > What are the changes that you request approval for? > > https://git.eclipse.org/r/#/c/32506/ I just rebased it, which created some unnecessary ws changes and will ask Steven to fix it. The requested change start in line 289.
(In reply to Lars Vogel from comment #30) > (In reply to Lars Vogel from comment #29) > > (In reply to Dani Megert from comment #28) > > > What are the changes that you request approval for? > > > > https://git.eclipse.org/r/#/c/32506/ > > I just rebased it, which created some unnecessary ws changes and will ask > Steven to fix it. The requested change start in line 289. How can I assess if that change breaks clients? The API itself (org.eclipse.e4.ui.model.fragment.MModelFragment.merge(MApplication)) has no documentation. I'm also missing tests for that new feature.
(In reply to Dani Megert from comment #31) > > How can I assess if that change breaks clients? The API itself > (org.eclipse.e4.ui.model.fragment.MModelFragment.merge(MApplication)) has no > documentation. > > I'm also missing tests for that new feature. The existing test for fragment merges is ResourceHandlerTest. This test runs fine with the change of Steven, we are planning to extend this test to include this API enhancement.
(In reply to Lars Vogel from comment #32) > (In reply to Dani Megert from comment #31) > > > > How can I assess if that change breaks clients? The API itself > > (org.eclipse.e4.ui.model.fragment.MModelFragment.merge(MApplication)) has no > > documentation. > > > > I'm also missing tests for that new feature. > > The existing test for fragment merges is ResourceHandlerTest. This test runs > fine with the change of Steven, we are planning to extend this test to > include this API enhancement. If elementIds are used as they currently are documented, the logic path is exactly the same as it was. Only if the prefix xpath: is used, or a comma separated list is entered will things change. And that code is safely inside a try statement. I suggest we simply mark this an experimental feature in the documentation, wherever that may be.
Steven updated https://git.eclipse.org/r/#/c/32506/ to that it does not contain any whitespace change and I asked him to give priority to the update of the existing test case.
(In reply to Steven Spungin from comment #33) > If elementIds are used as they currently are documented, Where can I find that documentation?
(In reply to Dani Megert from comment #35) > (In reply to Steven Spungin from comment #33) > > If elementIds are used as they currently are documented, > > Where can I find that documentation? AFAIK we have no documentation for fragment merging, so I'm also curious about that statement.
I didn't say 'officially documented?' :) http://www.vogella.com/tutorials/Eclipse4Modularity/article.html http://eclipsesource.com/blogs/2012/06/26/eclipse-4-e4-tutorial-part-3-extending-the-application-model/
Dani, Javadoc has been contributed by Steven. Please advice with the PMC flag.
Tests are done. I will upload ASAP. I found a 'bug' in the original code base. If a feature is not set (example: children) an NPE will be thrown. I say we just ignore the model fragment. Yes?
(In reply to Steven Spungin from comment #39) > Tests are done. I will upload ASAP. > > I found a 'bug' in the original code base. If a feature is not set > (example: children) an NPE will be thrown. I say we just ignore the model > fragment. Yes? Whats the preferred way to notify that a fragment has missing a feature? I don't want to pollute the console...
(In reply to Steven Spungin from comment #40) > (In reply to Steven Spungin from comment #39) > > Tests are done. I will upload ASAP. > > > > I found a 'bug' in the original code base. If a feature is not set > > (example: children) an NPE will be thrown. I say we just ignore the model > > fragment. Yes? Please open a new bug for this error. > Whats the preferred way to notify that a fragment has missing a feature? I > don't want to pollute the console... We should log this into the error log.
Gerrit change https://git.eclipse.org/r/32506 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=90f6e90af7c0bea344c6847a0492cad5bbd1b928
Looks fine to me Steven, thanks a bunch for your great work. Sorry for the long processing time.
The tests are failing in the build, Steven can you have a look. http://download.eclipse.org/eclipse/downloads/drops4/I20150408-1100/testresults/html/org.eclipse.e4.ui.tests_macosx.cocoa.x86_64_7.0.html
(In reply to Lars Vogel from comment #44) > The tests are failing in the build, Steven can you have a look. > http://download.eclipse.org/eclipse/downloads/drops4/I20150408-1100/ > testresults/html/org.eclipse.e4.ui.tests_macosx.cocoa.x86_64_7.0.html I think this is more of an issues of how JXPath is integrated into the test case. There are a couple of issues here. #1 Dependency issue org/apache/commons/jxpath/ri/model/beans/NullPointer (hahaha funny name for a class; It really fogs things in the stack trace :) ) Is defined in JXPath version 1.3 // Compiled from NullPointer.java (version 1.3 : 47.0, super bit) public class org.apache.commons.jxpath.ri.model.beans.NullPointer extends org.apache.commons.jxpath.ri.model.beans.PropertyOwnerPointer { Perhaps 1.3 is not the version we are linking to? #2 Resource resource = handler.loadMostRecentModel() is called from testModelProcessor. This test should NOT be calling any new path code based on your excellent direction to put the new code in a separate method/fragment. However, the xpath test is somehow getting persisted and then loaded here, so although we moved the xpath to a new fragment, it is getting loaded with path based parentElementIds, and this causes the new methods to get called. My best guess is that in the test method localContext.set(E4Workbench.PERSIST_STATE, Boolean.TRUE); localContext.set(E4Workbench.CLEAR_PERSISTED_STATE, Boolean.TRUE); or -clearPersistedState in the launch config is not working This will be fixed when #1 is fixed, but it seems to me the test case wants to clear persisted state and it is not happening; this should be filed as a new bug.
New Gerrit change created: https://git.eclipse.org/r/45697
I moved the test case for XPath to a new class in order to bypass issue #2 by initiating a fresh HeadlessStartupTest. At this point only 1 test should fail should the class-not-found dependency issue arise.
@Lars, can you report on the version of JXPath that was getting pulled in from the failed test?
(In reply to Steven Spungin from comment #48) > @Lars, can you report on the version of JXPath that was getting pulled in > from the failed test? What makes this hard to debug (for me) is that the tests run locally. I mark this Bug as fixed and suggest that we work on the test in RC1 via Bug 465818. I have a deeper look tomorrow, today I was busy with PDE icon work.
*** Bug 324954 has been marked as a duplicate of this bug. ***
Is there any WikiPage that describes how to use this feature? I'm able to extend any application as the Model editor offers that directly, but how (for example) I add an perspective to ANY Perspective stack?
