Community
Participate
Working Groups
This idea was generated from the discussion from Laurent Petit regarding his requirements for managing Clojure changes to the model... Laurent says: "I need to find all MApplicationElements by tags, and then I inspect them: if they have some specific transient property set, with the right value, then it's ok. If they miss this transient property, I want to remove them." Now that we've addressed bug 383403 and are looking at bug 424345 perhaps we should take the time to review our approach and see if we can come up with something more generic (a la what we did for the 'createModelElement' API. I'll add some comments based on some experiments I've done but really need input from EMF experts on what we can / should do...
We seem to have two different issues: 1) We need to be able to ultimately do a *complete* iteration over the model. Preferably this will be applicable to extended models (one where the UI model has extensions in it) as well as being performant. While the API itself is TBD we can likely require that the caller provide the type of the elements it wants. Perhaps we can use that info and the model's structure to cull search paths that can't lead to this type. 2) We need to be able to extend this iterator's 'match' filter to include the ability to define a 'custom' filter (which laurent could use to do his whole operation in one step..;-). Here I have what I think is a reasonable solution (at least for Java). We could create a version of the 'findElements' API that has an 'Object' CF parameter to define a custom filter. This CF object would be expected to contain a method annotated with '@MatchElement' (a new annotation) that returns a boolean (true meaning that it 'passed' the test. I tested this and you *can* make an anonymous inner class directly in the call to the api (paraphrase of laurent's case): List<MPart> kids = ms.findElements(theApp.getChildren().get(0), null, MPart.class, magicClojureTag, EModelService.PRESENTATION, new Object() { @MatchElement boolean matchElement(MUIElement elementToTest) { return elementToTest.getTransientData().get("Clojure Data) == <value>; } }); The test for the tags is done with the default matcher and if the element passes that test then the custom matcher is invoked (so the custom filter doesn't have to re-check for the tag...).
What about providing an API which implements the visitor pattern, so we can allow people to more generally walk our model-tree?
I'd like to replace this in the context of the initial need, so that reviewers can ponder whether the proposed solution(s) best solves it (maybe there are better ways to do it). The need is to manipulate the MApplication at runtime. It's not specific to Clojure. I'd say it's of interest of every person willing to to live manipulation e.g. in ModelProcessors. The initial missing point is the ability to get any kind of MApplicationElement, not just MUIElement. A generic Visitor for the MApplication would probably fit the bill. More specialized methods, such as the existing findXXX methods on the EModelService, may then only be necessary if they provide additional value in e.g. performance (are they optimized ? Are Elements indexed on Tags, ids, etc. ?) To be specific, the MApplicationElements that I see missing in the Kepler API are MCommand, MHandler, MKeybinding. But it's just because I've only investigated this part of the API. Probably some more may be missing I'm unaware of, such as Command Categories, Key Binding Tables, etc., etc.
One thing we'll certainly need is a full Visitor pattern; it's as requirement for the type of thing that Laurent is suggesting. The question I have here is whether we're best off to use the EMF generated one or do our own. I suspect that the EMF generated one will be akin to the generated editor, complete but clunky. It will have the advantage of working with extended models though. I think it's worth at least investigating whether we can come up with a more optimal one (perhaps even contribute it back to EMF if it's generic enough...;-). We have a couple of advantages if we assume the model's containment remains type-specific (including any extensions) and if we require that the caller know the type of what they're looking for. If we know these two things then we can cull the search tree by identifying branches that can't lead to the desired type (i.e. you'll never find an MPart in an MMenu/MToolBar...). It's worth pointing out that this analysis is static and need not be done more than once for a given model. My agenda here is really to see if we can *reduce* the number of methods in the API rather than following the current pattern of adding new ones on a case-by-case basis (i.e. for MKeyBinding...). Not only will they bulk up the EModelService's API but they also don't work for extended models. By (my) preference the best result would be to define a single new visitor based API along the lines of: visit(MApplicationElement toSearch, Class toFind, Object visitor)
Laurent, you'll have noticed that the proposed API still uses an 'Object' as the incoming filter. If most folks think that it's more appropriate to use an interface fine but we should support both approaches IMO (i.e. we'll check the incoming 'Object' to see if it implements the 'IVisitor' interface and if not try the DI way). I have a *very* string dislike now for framework based 'API'; our 3.x 'framework' has lead us to a place where we have ~50 man *YEARS* of infrastructure implementation (views, editors, dialogs, commands...) that has been so tied up in the 'framework' that it's effectively inextricable (without more man years of work) and thus unusable by e4 RCP, we must not go down this road again. Now what would be really cool is to see if we can write an 'injector' for Clojure but that's well outside the scope of this defect..;-).
Currently Add-ons can also not be found by the model service. I think we should also add the possibility to find add-ins with this service
OK, I've revamped the approach based on feedback from John Arthorne. I was trying to figure out how to handle the new API without producing yet another longer 'findElements' method... The new approach is to create a new public (API) base class 'ElementMatcher' which encapsulates the original parameters used to filter the elements; 'id', 'clazz' and 'tagsToMatch' as well as owning the base implementation of 'matchElement'. Now we can add a new EModelService method 'findElements(MUIElement root, ElementMatcher matcher, int searchFlags)' and switch the other existing flavors to use the new mechanism (i.e. create an ElementMatcher from the parameters and call the new API. This means that clients can now subclass ElementMatcher and either sublcass (or even override) the 'matchElement' method to perform whatever checks they need to perform. This means that we won't be needing any new annotations like @MatchElement...
How does the ElementMatcher look like is it a SAM Type?
Well, right now it's a class with a single non-abstract method which provides the current filtering logic. Does that count ?
(In reply to Lars Vogel from comment #6) > Currently Add-ons can also not be found by the model service. I think we > should also add the possibility to find add-ins with this service Lars, I'd prefer to keep the EModelService for stuff that aren't trivial for a client to implement. For the same reason both Paul and I think we should remove the current 'findHandler' method; since the user has to locate the MBindingContainer themselves surely they are capable of writing a for loop that checks the id... We're also going to change the 'searchRoot' to be an MApplicationElement. This is mostly to avoid having to do this later (and to match the change we've just made to the 'getContainingContext' API). The initial implementation for iterating through an MApplication will recurse through its list of windows, we can extend this in the future if needed.
(In reply to Eric Moffatt from comment #9) > Well, right now it's a class with a single non-abstract method which > provides the current filtering logic. Does that count ? No that is not good in terms of Java8 API and lambda expressions - it has to be a functional interface so that one can use lambda expressions in future. I'd suggest something like: interface ElementMatcher { boolean match(....); public static Default implements ElementMatcher { boolean match(....) { // .... } } }
Interesting, I could also just provide the interface and have my current MatchElement base class extend it. I'm admittedly a noob as far as lambas go...can you provide an example of where someone might want to use one in this scenario ? Just because we have a new hammer (lambdas) doesn't mean that all API should look like a nail...
(In reply to comment #12) > Interesting, I could also just provide the interface and have my current > MatchElement base class extend it. > > I'm admittedly a noob as far as lambas go...can you provide an example of where > someone might want to use one in this scenario ? > > Just because we have a new hammer (lambdas) doesn't mean that all API should > look like a nail... I don't quite understand the question, but this is what such a lambda could look like in use: static void searchAndProcess(String id, Class<? extends MUIElement> clazz, List<String> tagsToMatch) { Predicate<MUIElement> filter = e -> id.equals(e.getElementId()) && clazz.isInstance(e) && e.getTags().containsAll(tagsToMatch); // find elements matching the filter } That's the default implementation except written from the client's point of view. Other ones are also possible: Predicate<MUIElement> filter2 = e -> e.isVisible() && e.isToBeRendered(); Predicate<MUIElement> filter3 = e -> e.getPersistedState().containsKey(CUSTOM_KEY_NAME); // etc.
OK, here's a Gerrit patch: https://git.eclipse.org/r/22374 Bases the new API on the existing Selector SAM interface. We provide a default implementation in a new class ElementMatcher which implements Selector and provides the replacement for the current 'match' method. We've decided to leave the existing API alone due to binary compatibility issues; while opening the scope from MUIElement to MApplicationElement is code compatible it appears that it's not binary compatible. There's a specific hack in here where the 'findElementsRecursive' method was calling the old API so I check and downcast the searchRoot. This should likely just be changed over to using the new API... Comments welcome particularly on the existing API's javadocs which have been modified quite a bit.
Looks good - we could try to push the Selector into a core package and name according to java8 Predicate and make it generic?
But legt's discuss this in an extra bug - API looks fine to me!
Some other things to consider: Should we move the Selector to the end of the parameter list of the new method ? We should likely also change 'getContainingContext' back to taking an MUIElement based on it breaking binary compatibility. Is there a case where something that is *not* an MUIElement could have anything except the MApplication as its containing context ? We should remove the 'findHandler' method since it's trivial and it hasn't been released yet.
Why does it break binary compat, John?
(In reply to Thomas Schindl from comment #18) > Why does it break binary compat, John? https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods "Change type of a formal parameter - Breaks compatibility" My guess is that binary bundles would have the method signature with MUIElement and so won't link correctly (can't find) at runtime with the new method signature. PW
Yes any change to the arguments of a method is not binary compatible. In class files the method signature has concrete type information. Changing a parameter to be a superclass is a strange case - it is usually source compatible but not binary compatible.
*** Bug 429311 has been marked as a duplicate of this bug. ***
Hmmm, looks like the original patch has already been committed... I've just pushed http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=59434046ea0ae9c5ee98a802cf9f2f94e13942ae This takes in the feedback from Paul / Lars; cleans up some of the javadoc as well as moving the 'Selector' argument to the end of the parameter list (should be clearer when making lambda 'inners'...
Verified in 4.4.0.I20140303-2000.
