Community
Participate
Working Groups
findElements() from EModelService does currently not find Menus and Toolbar elements. I think the model service should allow access to all model elements, I see no reason my certain elements should be handled differently.
Also MHandlers are not found. I think this issue should be put in the Javadoc of the method because the first impression of the method description kind of "hides" the fact that these elements are not considered in the search. If this method is not modified to suit this kind of search, IMHO there should be a kind of "findAllElements()...." for this purpose as the model service should provide a way for searching the Menus, Handlers etc.. contained in a MWindow or MApp. @Lars: I think you should move this to Platform/UI
@Sopot: Lets wait for Eric's opinion. I see no reason why findElements should not find certain elements.
When enhancing this interface, what if we include the xpath? That would allow the type to be specified and allow more targeted searches: //Part/Menu[@id='o.e.ui.views.ProblemsView'] PW
@Paul: xpath would be great, but I think that should be addressed via a separate bug.
Eric, Paul, can we set a milestone for this? I see this frequently in the forums and in 100% of the cases the expectation is that the findElements should find what they need. Also, if we don't provide it here we leave no choice but for the client of the model service to implement the traversing by itself. If we target this I may provide a patch for it (Menu, Toolbar and Handlers).
If you want to fix this you should use a TreeIterator from EMF.
How about M3 (about 5 weeks)? PW
I'm leery of changing the behaviour of findElements() to search all attributes, rather than just the children, as I think it will lead to sloppy and inefficient code. If devs know they're looking for a menu item, they should start the searxh from the menu. Given the complaints of 4.x slowness, we should try to prevent further issues from occurring! So I'd vote for changing the javadoc.
The problem is that is uses the interface MElementContainer to determine the children instead of the EMF children.
I'm leery about changing the current behavior as well but for different reasons. In general we have many, many more Menu / Tool items than we do ui elements. In the IDE it's 1000's (sometimes 3-4K) which would *severely* degrade the performance of these methods. I intentionally did not include them in the general search. I'm certainly not against augmenting the 'find' API set with ones optimized towards finding Commands related info. I'd even buy an uber-search that called both iterators but we should allow optimized versions of both. It's not really about using the regular containment vs the EMF eContainer() since the current code already does that in specific instances. The iteration code is also aware of the various 'logical' children of some containers (specifically the windows sub-windows and the trim). Certainly it makes sense to update the JDoc to reflect the current state. Is there a particular use-case here that we've identified where someone would want to do this (as opposed to working using the CommandService) ? Knowing what we expect it to be used for could drive the new API's def.
Thx Eric, that makes the current design decision much clearer. I think that in any case changing the JavaDoc would be huge improvement. IHMO, the main problem is in fact that developers, including me, would have espected any model element to be found within the tree.
Jonas, no real argument here...the API name 'findElements' doesn't give any indication of its limitations. Is there *any other* element type that we don't find using the regular call ? The only one I know about is that we likely won't find 'sharedElements' unless it's through an MPlaceholder's refs.
Please remember that our EMF-Model is extendable - I can buy the performance argument but we need to have a method that search through all and everything - if it is a new one I'm fine with that.
Eric, it is not an argument, it is a fact, that me and other people got it wrong. The decision whether this is due to our stupidity or due to a missleading method name or unexspected behavior leaves a lot room for interpretation. As often, the truth is probably somewhere in between :-) I would agree with Tom and offer a new method, i.e "findAllElements", which also mentions in the Javadoc, that it might be slow, depending on the model size.
One more note. The findElements gets a Class which we can inspect to see which type the user is searching for. Base on this information we actually know better which EMF-features to search through and which not.
IMHO the model service should really be able to search through all elements, especially as we allow to extend the model as Tom said. If we have optimized methods for special purposes that fine but we should not force our user to develop their own version of the model service.
Tom, I really like this idea, the potential search paths can very easily be retrieved from the meta-model, even in a generic way robust to meta-model change and/or adaption.
There are also a number of already defined constants that can be used to refine 'findElements' (such as IN_ANY_PERSPECTIVE...) perhaps adding 'IN_MENUS' and 'IN_TOOLBARS' would help at least at the implementation level. This is OK for 'findElements' but not so much for 'find' (should it also search the complete model? Should it have a version that takes in the 'searchFlags' ?). Tom, can the EMF tree iterator be 'tuned' to provide optimized search (i.e. to *not* search particular containers) ? If not then we have no way to optimize the search should it become necessary.
@Eric, if the EMF TreeIterator is only used internally you could replace it with optimized version if the need arrises.
I'd be all for this except that the time where we'd need optimization is already here in the IDE, we're working on a number of performance issues already (not saying that they're related to using these api's but it'd certainly be a step backwards IMO. I realize that there is never likely to be a stand-alone RCP app that is so complex that this would become an issue, what do you think of my suggestion to just add more search 'locations' (IN_MENUS...) to the existing call ? If useful we could even re-implement 'GLOBAL' to use the tree iterator if we want...
+1 for the option to search locally and for a global search.
Setting the target MS to 4.2.2 so I'll see it in my bug search...
Has anyone considered adding a find method that is asynchronous? That way even if it took a long time to search, the system would not be held up. It would also possibly allow the async callback to be delayed until the rendering engine finished creating the parts. This would enable a processor to retrieve a part from a fragment, even if it was called before the fragment.
Steven, that's an interesting take but we'll need a real world scenario where it would be necessary in order to focus the design, do you have one ? Lars, we're right at the end of the 4.2.2 cycle so I'm swamped with such real world issues right now. I expect that this will not get addressed before SR2 ships but *will* be addressed before EclipseCon (since it's part of the API I'll be presenting...;-).
What brought me to this thread was a very simple need: I created a menu Item in a fragment. During startup, I needed to dynamically set the menu item as selected or unselected I first tried to create a post fragment Processor to get the menu item from the injected EModelService Then I tried to create an AddOn and access the EModelService Then I tried to get the item in PostContextCreate from the application plugin Of course, as it turns out the EModelService would not provide me with the menu item ever. In such a robust system as e4, I can't understand why it should be so difficult to do such a basic thing. Surly the menu items are a part of the application model. My workaround turned out to be as simple as using the EModel service to get my main window, and using that window as the context in find(...) to get the menu item. If the reason EModelService is ignoring some items is because of performance issues, I would think one of these approaches would fit the bill: 1. Asynchronous callback from a slow search 2. Index the resources better to speed up searching 3. Provide parameters in the find method to include Resource-Type and/or defining Bundle (and possibly even have a FragmentId parameter along with the URI when defining the fragment to target specific contributions)
Steve, for your problem please also consider Bug 397249
Although this async stuff might sound interesting it brings up many problems. We'd access the model from a thread and hence need to deal with changes happening in between beside that EMF-Models are NOT thread safe and so we'll run into many problems. I think we should add another method which does the search the complete model and does not skip parts of it.
I also think we should not have asynchronous method calls in the services. Someone can always call this method in a thread and deal with the issues directly which may arise. Also this bug was specifically opened for the situation that the EModelService cannot find all methods. I agree with Tom that we should add a method which allows to find all model elements.
I agree that an async call is not the best solution, but there are a couple of other issues that this thread brings to the surface. The reason the find method was intentionally ignoring all of the model elements was because the amount of items to iterate the many contributions. I know that optimizations can be made to speed this up, but as the system grows, and/or if the plugins are ever distributed or proxied and located on the network for instance, the initialization will ultimately suffer performance issues. A problem I run into repeatedly is not knowing when resources are available during startup. The defining application has a pretty good shot using the LifeCycle handlers, but other contributing plugins might benefit from some additional LifeCycle like annotations during the model initialization, especially when their fragment contributions have been rendered. This would effectively be 'async', yes?
@Steven: if you want to discuss async processing I suggest you open a new big report or discuss it in the Eclipse forum. A bug report should discuss only one issue at the and I opened this one for the missing search of creation elements.
@Lars: I entered bug Bug #398036; However, my suggestion there does offer a specific ideas pertaining to the resolution of this bug.
I expect to be getting to this shortly after 4.2.2 ships using the approach outlined in comment #18 initially...comments welcome on what we should do with the basic 'find' though (my initial idea would be to offer one that takes the flags and have the simple one call it with the flags set to only iterate the model as we currently do...).
Sounds good Eric. My personal preference would be that without flag the findElements() method searches through all elements and that in performance critical code you could use a new IN_CHILDREN flag to avoid searching menus if not needed. I think that would be simple to use for the average RCP case and still offer the potential to avoid performance bottlenecks in the Eclipse IDE.
*** Bug 405093 has been marked as a duplicate of this bug. ***
Today I needed to local an MMenu contributed by a fragment. So the EModelService would be the first thing to try. After about an hour of trying and debugging, I realized it was simply not supported. (Using the MApplication as the search root). It also popped to mind, the EMenuService could offer such a service. If 'visiting' a big EMF hierarchy is a performance concern, why not create specializations for specifics objects, and instruct users to use the proper 'visitor' for the target object. Just a thought. So I implemented this for my use case (Yes, it's a hack but works for now). private MMenuElement findMenu(MUIElement uiElement, String id) { if (uiElement instanceof MMenuElement && uiElement.getElementId().equals(id)) { return (MMenuElement) uiElement; } if (uiElement instanceof MTrimmedWindow) { MMenuElement findMenu = findMenu( ((MTrimmedWindow) uiElement).getMainMenu(), id); if (findMenu != null) { return findMenu; } } else if (uiElement instanceof MPart) { List<MMenu> menus = ((MPart) uiElement).getMenus(); for (MMenu mm : menus) { MMenuElement findMenu = findMenu(mm, id); if (findMenu != null) { return findMenu; } } } else if (uiElement instanceof MMenu) { List<MMenuElement> children = ((MMenu) uiElement).getChildren(); for (MMenuElement me : children) { System.out.println(me.getElementId()); MMenuElement findMenu = findMenu(me, id); if (findMenu != null) { return findMenu; } } } return null; }
BTW, from experience programming against EMF, I know visiting hierarchies is so 'common' why not just say. Look, here is the model. Go and write your own visitors. I personally would vote against a service which is named "EModelService" but visits only parts of it. Call it "EPartialModelService" :-)
Ed, do you have any performance data for searching in large EMF models? Any idea what the performance overhead would be to search in an application model with approx. 1000 tools and menu entries?
Visiting entire trees is very common. Doubt visiting a tree with 1000 elements would be a big performance concern unless you did it one per element and hence had some kind of n^2 algorithm.
What about setting up a test case with a model and comparing the numbers? My vote would be that we make the default search going through the complete model and add a new method which does the optimized search, if the numbers we get indicate that we really need it.
Quote Tom: "My vote would be that we make the default search going through the complete model and add a new method which does the optimized search, if the numbers we get indicate that we really need it." +1
(In reply to comment #39) > What about setting up a test case with a model and comparing the numbers? > > My vote would be that we make the default search going through the complete > model and add a new method which does the optimized search, if the numbers > we get indicate that we really need it. +1 for that. findElements(..) should find all elements. Who is taking this? @Christophe do you care to write a patch since you are already in the middle of it?
If linear search performs well then we should change the default behavior IMO. If not, we could have the EMS maintain a sorted-by-id linear list (.eAllContents may help here? )of all nodes with a non-null id (or a binary tree) and their reference to the MUIElement and do a binary search in logN average which is lightning fast - for 1 billion elements you would do ~30 O(1) comparisons (supposing that the String comparison of the ID is O(1)). It would add a little bit of code and potentially O(n) space to maintain this list (also adding & removing elements would require logN time) but the find-by-id query time would be close to nothing. If you agree I could prototype this.
before making modification lets please get some numbers. It should be really easy to create a model with many many many elements (say 5.000 ToolItems, 5.000 Handlers, ...). Ideally we'd could somehow control the search order - process children containments first - wild guess is that it is the order of containment features in the .ecore, Ed?
Hi, I can help out a bit, perhaps writing the performance case. It's in there a large generated Application.e4xmi in the test suite already? Cheers Christophe
Yes a content tree iterator generally follows the order of features in the EClass.eAllStructuralFeatures (and all the derived subsets follow that order as well).
OK, we need to do something and I suspect that we could easily optimize the existing implementation to search the presentation first, then TrimBars... The reason for searching the presentation first is that there are far fewer elements involved in the presentation (even with multiple perspectives open0 than there are menu / tbs and their items. I'm guessing we should also augment the existing 'location' constants to include more constants such as MAIN_MENU and MAIN_TOOLBAR.
What about maintaining 3 indexes on demand via an EContentAdapter which help to find the 3 main search criteria (id, class, taglist) a little bit faster: Index on ID: id --> List of elements with the same ID Index on Class class --> List of elements represented by the same class Index On Tag tag --> List of elements which contain the same tag The search could afterwards use those indexes to elect a smaller list of elements to search through, e.g.: (the following order is taken from the ModelServiceImpl#match(MUIElement element, String id, Class clazz, List<String> tagsToMatch)) id given: --------- take the list of elements with this ID and if required filter by class and tag list id is not given but class is given: ----------------------------------- take the list of elements with the same class and if required filter by the tag list id and class not given but a tag list: -------------------------------------- take one tag out of the search tag list and retrieve the indexed by tag list of elements and filter out the ones which have all tags in common. In the end the returned (reduced) list must only remove those elements which are not a child of the "searchRoot". The only problem I see is the usage of those searchFlags. Are they really required?
Created attachment 237830 [details] a generated large application model I've also generated a large application model (~1MB) by taking the Live-Model of my running Eclipse and duplicate some reasonable parts (like Menu-Items, Perspectives, Parts, ...). It's not a perfect model but it is a beginning for some tests. (I even have one with 120MB, but thats already hard to open in the editor and harder to upload here ;-) )
(In reply to Christophe Bouhier from comment #35) > Today I needed to local an MMenu contributed by a fragment. > So the EModelService would be the first thing to try. After about an hour of > trying and debugging, I realized it was simply not supported. (Using the > MApplication as the search root). It also popped to mind, the EMenuService > could offer such a service. If 'visiting' a big EMF hierarchy is a > performance concern, why not create specializations for specifics objects, > and instruct users to use the proper 'visitor' for the target object. Just > a thought. So I implemented this for my use case (Yes, it's a hack but works > for now). > > > > private MMenuElement findMenu(MUIElement uiElement, String id) { > if (uiElement instanceof MMenuElement > && uiElement.getElementId().equals(id)) { > return (MMenuElement) uiElement; > } > > if (uiElement instanceof MTrimmedWindow) { > MMenuElement findMenu = findMenu( > ((MTrimmedWindow) uiElement).getMainMenu(), id); > if (findMenu != null) { > return findMenu; > } > > } else if (uiElement instanceof MPart) { > List<MMenu> menus = ((MPart) uiElement).getMenus(); > for (MMenu mm : menus) { > MMenuElement findMenu = findMenu(mm, id); > if (findMenu != null) { > return findMenu; > } > } > } else if (uiElement instanceof MMenu) { > List<MMenuElement> children = ((MMenu) uiElement).getChildren(); > for (MMenuElement me : children) { > System.out.println(me.getElementId()); > MMenuElement findMenu = findMenu(me, id); > if (findMenu != null) { > return findMenu; > } > } > } > return null; > > } For legacy code one can registered a customized EModelService in a ModelAddon (or processor) to keep client code free workarounds. Simply create a ModelAddon and run something like: @Inject private void modelAddon(final IEclipseContext ctx) { EModelService eModelService = ctx.get(EModelService.class); ctx.set(EModelService.class, new EModelServiceWrapper(eModelService)); } EModelServiceWrapper then looks similar to: public class EModelServiceWrapper implements EModelService { private final EModelService eModelService; public EModelServiceWrapper(EModelService originalModelService) { eModelService = originalModelService; } //TODO overwrite find methods and use Christophe's EMF visitor to search the complete model with fallback to original EMS } Once this bug is fixed one can just remove the bundle with the ModelAddon/EModelServiceWrapper.
Gerrit review from Wojciech Sudol https://git.eclipse.org/r/#/c/19311/
In the gerrit there is an initial implementation based on previous comments and Eric's suggestions. Comments: 1. I added tool bars and menus as new places to search in the findElements(*) methods. They can be enabled by flags IN_MAIN_MENU and IN_PART 2. The findElements(*) methods are intended to handle MUIElement objects. As the MHandler does not extend this interface, I added a new method - findHandler(handlerContainer, id). 3. Performance - Eric suggested to improve it in a separate bug. +1 from me. 4. I am working now on tests for the new code. Will be added soon.
Simple unit tests added.
M4 is done...
Committed (for Wojciech): http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=5ed75b1ca32405897472e499d5734f9d95aed127 Wojciech, we might want to open a separate defect against the 'getElementLocation'...the javadoc for the new constants indicates that that's where they'll show up but we haven't extended the implementation to be able to set the new flags... BTW, the bug should also mention that we should fix the javadoc for all of these constants to highlight that they are dual purpose (control where to look in the 'find' methods and where an element *is* in 'getElementLocation'.
If I read the new source correctly, findElements find now everything except handlers. Can handlers also be included in the findElements method?
(In reply to Eric Moffatt from comment #54) > Committed (for Wojciech): > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=5ed75b1ca32405897472e499d5734f9d95aed127 > > Wojciech, we might want to open a separate defect against the > 'getElementLocation'...the javadoc for the new constants indicates that > that's where they'll show up but we haven't extended the implementation to > be able to set the new flags... > > BTW, the bug should also mention that we should fix the javadoc for all of > these constants to highlight that they are dual purpose (control where to > look in the 'find' methods and where an element *is* in 'getElementLocation'. Thank you Eric, I have opened a new bug according to your comment. BTW. What about the performance bug. Should I open a new one, or maybe I should only mention this issue in a general performance bug? (In reply to Lars Vogel from comment #55) > If I read the new source correctly, findElements find now everything except > handlers. Can handlers also be included in the findElements method? Hi Lars, I already mentioned it: > 2. The findElements(*) methods are intended to handle MUIElement objects. As > the MHandler does not extend this interface, I added a new method - > findHandler(handlerContainer, id).
> I already mentioned it: > > > 2. The findElements(*) methods are intended to handle MUIElement objects. As > > the MHandler does not extend this interface, I added a new method - > > findHandler(handlerContainer, id). Can you check with Paul. I read this comment from Paul on the e4 mailing list. See http://dev.eclipse.org/mhonarc/lists/e4-dev/msg08206.html --------------- n Fri, Dec 13, 2013 at 9:02 AM, Laurent PETIT <laurent.petit@gmail.com> wrote: This lets room for MKeybinding, MHandler, MCommand. A method like findByTags(theApp, MHandler.class, List<String> tags) would work for the first part of my usecase. I would expect that we would provide a more generic version of org.eclipse.e4.ui.workbench.modeling.EModelService.findElements(MUIElement, String, Class<T>, List<String>, int) Something that could be used: List<MHandler> elements = service.find(root, null, MHandler.class, tags, 0); But I would still be out of luck concerning the second part of my use case: removing the MHandler instance from its parent MHandlerContainer. If we have a way to provide a parent, I think that we'd probably expose another EModelService method: Object parent = service.getParent(handlerModel); -----------------------
(In reply to Lars Vogel from comment #57) > > I already mentioned it: > > > > > 2. The findElements(*) methods are intended to handle MUIElement objects. As > > > the MHandler does not extend this interface, I added a new method - > > > findHandler(handlerContainer, id). > > Can you check with Paul. I read this comment from Paul on the e4 mailing > list. See http://dev.eclipse.org/mhonarc/lists/e4-dev/msg08206.html findHandlers(*) is fine for now. Wojtek and Eric are working on a more general search, and findHandlers(*) would probably be subsumed by that (before M6 ends, of course :-) PW
Verified in I20140120-2000.
Was this ever integrated? It's May 2017 now, Eclipse Neon.SP3, and EModelService still cannot find any menus!? Am I doing something wrong? @Inject EModelService modelService; @Inject MApplication application; @Inject MPart thisPart; List<?> x = modelService.findElements(application, null, MPopupMenu.class, null); // returns empty list List<?> x = modelService.findElements(application, null, MMenu.class, null); // returns empty list // and I do have couple of menus in the the model on several levels List<?> x = modelService.findElements(application, "org.bitbucket.espinosa.efm.part.dircontent.ctxmenu", null, null); // returns empty list // the ID definitively exists List<?> x = modelService.findElements(thisPart, null, MPopupMenu.class, null); // returns empty list List<?> x = modelService.findElements(thisPart, "org.bitbucket.espinosa.efm.part.dircontent.ctxmenu", null, null); // returns empty list List<?> x = thisPart.getMenus(); // this finally returns my menus including "org.bitbucket.espinosa.efm.part.dircontent.ctxmenu"
Sorry, perhaps this is not the best place to discuss it. I have filed a new bug report for it, please see bug 516261.
