Community
Participate
Working Groups
MUIElement.setParent(MElementContainer<MUIElement>) currently requires an MElementContainer<MUIElement> but actually should accept MElementContainer<? extends MUIElement>. Otherwise it is not possible to link a placeholder to a part stack for example because MPartStack is of type MElementContainer<MStackElement> ...
This might be currently not possible with the code generator, we should have the following signatures: MUIElement.setParent(MElementContainer<? extends MUIElement>) MUIElemet.getParent(MElementContainer<MUIElement>) Currently I was only able to either generate the existing: MUIElement.setParent(MElementContainer<MUIElement>) MUIElemet.getParent(MElementContainer<MUIElement>) Or this alternative: MUIElement.setParent(MElementContainer<? extends MUIElement>) MUIElemet.getParent(MElementContainer<? extends MUIElement>) In which the 'getParent' is broken, because an element does (and cannot) know its parent generic type. @Ed, are there any alternative available? Note that in the code-base there are a few cases where this workaround is used: // double cast because we're bad people (MElementContainer<MUIElement>) ((Object) value)
(In reply to Rolf Theunissen from comment #1) > Or this alternative: > MUIElement.setParent(MElementContainer<? extends MUIElement>) > MUIElemet.getParent(MElementContainer<? extends MUIElement>) > In which the 'getParent' is broken I don't think this is broken but correct if you state that > because an element does (and cannot) > know its parent generic type. It might be an API break though... > Note that in the code-base there are a few cases where this workaround is > used: > // double cast because we're bad people > (MElementContainer<MUIElement>) ((Object) value) I use something similar atm, but that's not really a workaround but simply silencing the compiler about a valid incompatibility as per API a MUIElement is only allowed to have MElementContainer<MUIElement> as parent... I just haven't found any interface that actually implements this ;-)
It's always possible to use container.getChildren().add(child) rather than using child.setParent(container). Moreover, because the parent reference is the opposite of the children reference, it necessarily must be possible to add the child to the container's children because both things must (and do) happen regardless of whether you express it as child.setParent(container) versus container.getChildren().add(child). And of course the latter add is also not possible (expressible) if the type of the container is MElementContainer<? extends MUIElement>. So I don't think you ought to expect differently...
So you say that essentially MUIElement.setParent(..) should better be removed/never used? For me it still reads as if getParent() has to return 'MElementContainer<? extends MUIElement>' as a child might be compatible with multiple parents but not vice-versa...?
(In reply to Christoph Laeubrich from comment #4) > So you say that essentially MUIElement.setParent(..) should better be > removed/never used? > > For me it still reads as if getParent() has to return 'MElementContainer<? > extends MUIElement>' as a child might be compatible with multiple parents > but not vice-versa...? Only wildcards should not be used in return return types (hence by statement about breaking, the API becomes useless), solarlint output: """ Generic wildcard types should not be used in return types (java:S1452) It is highly recommended not to use wildcard types as return types. Because the type inference rules are fairly complex it is unlikely the user of that API will know how to use it correctly. Let's take the example of method returning a "List<? extends Animal>". Is it possible on this list to add a Dog, a Cat, ... we simply don't know. And neither does the compiler, which is why it will not allow such a direct use. The use of wildcard types should be limited to method parameters. """
(In reply to Rolf Theunissen from comment #5) > (In reply to Christoph Laeubrich from comment #4) > > So you say that essentially MUIElement.setParent(..) should better be > > removed/never used? > > > > For me it still reads as if getParent() has to return 'MElementContainer<? > > extends MUIElement>' as a child might be compatible with multiple parents > > but not vice-versa...? > > Only wildcards should not be used in return return types (hence by statement > about breaking, the API becomes useless) But as a matter of fact, the parent is not a MElementContainer<MUIElement> and will either throw a RTE or produce unexpected behavior. > , solarlint output: > > """ > Generic wildcard types should not be used in return types (java:S1452) > > It is highly recommended not to use wildcard types as return types. Because > the type inference rules are fairly complex it is unlikely the user of that > API will know how to use it correctly. > > Let's take the example of method returning a "List<? extends Animal>". Is it > possible on this list to add a Dog, a Cat, ... we simply don't know. And > neither does the compiler, which is why it will not allow such a direct use. > The use of wildcard types should be limited to method parameters. > """ That's a good example why I'm personally very reluctant about the usefulness of 'generic' Rulsets like SonarLint/Checkstyle/... and so on that claim to know the only truth... This hint assumes that I (as the user of an API) like to (or are allowed to) add things to the collection returned here, but modifying collections is an optional operation. declaring List<? extends Animal> makes perfectly sense if the list is not meant to be added things as the compile will never allow me to add items to it (where today Stream<Animal> would be a better choice here). Also limit this hint to return values does not make sense, as I (as a user) would also not able to add Dog, Cats, ... whatever if it would be a method parameter either. So consequently one should not use Wildcards all together ;-)
You should also note that when the Javadoc contains "@generated", whatever you change manually will be replaced by exactly what you see now. This includes the implementation method where you can see (indirectly) that in the end, setting the parent adds to the parent's children: /** * <!-- begin-user-doc --> * <!-- end-user-doc --> * @generated */ @Override public void setParent(MElementContainer<MUIElement> newParent) { if (newParent != eInternalContainer() || (eContainerFeatureID() != UiPackageImpl.UI_ELEMENT__PARENT && newParent != null)) { if (EcoreUtil.isAncestor(this, (EObject) newParent)) throw new IllegalArgumentException("Recursive containment not allowed for " + toString()); //$NON-NLS-1$ NotificationChain msgs = null; if (eInternalContainer() != null) msgs = eBasicRemoveFromContainer(msgs); if (newParent != null) msgs = ((InternalEObject) newParent).eInverseAdd(this, UiPackageImpl.ELEMENT_CONTAINER__CHILDREN, MElementContainer.class, msgs); msgs = basicSetParent(newParent, msgs); if (msgs != null) msgs.dispatch(); } else if (eNotificationRequired()) eNotify(new ENotificationImpl(this, Notification.SET, UiPackageImpl.UI_ELEMENT__PARENT, newParent, newParent)); } I'm not saying setParent should never be used, I'm just saying that if that method didn't exist (which would be the case if the model had MUIElement.parent.changeable set to false), you would still be able to change the parent by changing the parent's children instead. But of course an add is not possible if the parent is of type MElementContainer<? extends MUIElement>, so you end up with the same problem on the other side of the coin... So whatever logical path you take, the bottom line is that the child will be added to the parent, despite any signature change you might desire which would imply that's not possible.
(In reply to Ed Merks from comment #7) > So whatever logical path you take, the bottom line is that the child will be > added to the parent, despite any signature change you might desire which > would imply that's not possible. So even if they are incompatible types? e.g > MApplication extends MElementContainer<MWindow> I would get compile error if I try to > application.getChildren().add(MMenuElement) but say I have a Window that is a child of the application > window.getParent().add(MMenuElement) would work...? That's why I'm confused, because currently I can (per API) add *any* child via the getParent() method while I actually should check if the parent is a MMenu? > But of course an add is not possible if the parent is of type MElementContainer<? extends > MUIElement>, so you end up with the same problem on the other side of the > coin... I don't think this is really completely interchangeable. While I can't (without some dirty type erasure) call MMenuElement#setParent(MMenu), I can perfectly safe do the following: MElementContainer<? extends MUIElement> newparent = .... if (newparent instanceof MMenu) { ... } else if (...) { ... } else { //something is really wrong or unsupported parent type } That's why (for me) the consequence is, instead of really try to call #setParent it should check for the type and call getChildren().add(....) instead.
Trying to make the problem more clear. We have 1. The ElementContainer interface contains (generic) elements that extend UIElement, this interface has only an abstract implementation. Interfaces that extend the ElementContainer all have a sub-type of UIElement as element type, when they have concrete implementations. The container knows which sub-types are allowed. 2. UIElement that have a parent. From the child, it cannot be known which specific sub-types are allowed in the parent. The parent can contain any subtype of UIElement or some specific ones. Moreover, this can change when the UIElement is re-parented. So when adding child elements to a parent, the generics will prevent of adding incorrect types to the parent. Compile time errors will be generated. As Christoph also noticed, when setting the parent from the child, no such checks are there. There are no compile time warnings, even run-time will probably generate no errors, until there is some unexpected class cast exception somewhere. MApplication application; MWindow window; MMenu menu; application.getChildren().add(window); application.getChildren().add(menu); // Compiler error MElementContainer<MUIElement> parent = window.getParent(); menu.setParent(parent); // No warning or error So looking at it closely, I think that the current API is quite dangerous. I am not able to predict what the behavior of the code will be, will the inverse add throw an exception, or will the getChildren() return elements that are of incorrect type? If that is even possible to see after generic type erasures. Also bugs like Bug 518248 are real world examples that things do go wrong. Changing setParent to setParent(MElementContainer<? extends MUIElement>) would make the method more convenient to use, but the dangers of the code are still there. So this would in practice make things worse. Some alternatives: 1. Remove the setParent method 2. Make the setParent method container type aware. At least this would require that an generic is added to UIElement. Though, in general the element cannot know what is the container type. So I doubt this is feasible. 3. Make the return type of getParent MElementContainer<? extends MUIElement>. This would make clear that the element doesn't know its parent type. Though this breaks much existing code that calls 'element.getParent().setSelectedElement(element)', this call also needs the container type to match. The extra checks (selected element should be a child) ensure that this code is safe. But any of these alternatives could result in API breakage. Are there any other alternatives, or isn't there a problem?
(In reply to Christoph Laeubrich from comment #8) > (In reply to Ed Merks from comment #7) > > So whatever logical path you take, the bottom line is that the child will be > > added to the parent, despite any signature change you might desire which > > would imply that's not possible. > > So even if they are incompatible types? e.g > > > MApplication extends MElementContainer<MWindow> > This is kind of a context-free question. > I would get compile error if I try to > > > application.getChildren().add(MMenuElement) > > but say I have a Window that is a child of the application > > > window.getParent().add(MMenuElement) > > would work...? > You can try it. You'll find there are many overrides of org.eclipse.e4.ui.model.application.ui.impl.ElementContainerImpl.getChildren() that will prevent at runtime things that are not correct. > That's why I'm confused, because currently I can (per API) add *any* child > via the getParent() method while I actually should check if the parent is a > MMenu? > This is how the e4 designers chose to model it. > > But of course an add is not possible if the parent is of type MElementContainer<? extends > > MUIElement>, so you end up with the same problem on the other side of the > > coin... > > I don't think this is really completely interchangeable. While I can't > (without some dirty type erasure) call MMenuElement#setParent(MMenu), I can > perfectly safe do the following: > > MElementContainer<? extends MUIElement> newparent = .... > if (newparent instanceof MMenu) { > ... > } else if (...) { > ... > } else { > //something is really wrong or unsupported parent type > } > > That's why (for me) the consequence is, instead of really try to call > #setParent it should check for the type and call getChildren().add(....) > instead. I'm not sure where this is headed, but of course with erasure not all type information is available... The question is where does this line of reasoning go? What must change? What will be broken as a result? And what will be so much better as a result? Certainly allowing setParent to be more easily called is not going to make things more likely to be correct, but maybe you haven't reached that conclusion yet.
(In reply to Rolf Theunissen from comment #9) > Changing setParent to setParent(MElementContainer<? extends MUIElement>) > would make the method more convenient to use, but the dangers of the code > are still there. So this would in practice make things worse. From the discussion and insights here (thanks Ed for the details) I think MElementContainer#setParent could not be made reliable, and maybe be deprecated with a warning that it is inherently unsafe to call this method and getChilds().add(...) have to be used instead to get the same effect. > Some alternatives: > 1. Remove the setParent method or deprecate it as mentione above > 2. Make the setParent method container type aware. At least this would > require that an generic is added to UIElement. I think this would not work well as then I'm probably lost with e.g. a MMenu<?> so how do I know the type? > 3. Make the return type of getParent MElementContainer<? extends > MUIElement>. This would make clear that the element doesn't know its parent > type. Though this breaks much existing code that calls > 'element.getParent().setSelectedElement(element)', this call also needs the > container type to match. The extra checks (selected element should be a > child) ensure that this code is safe. I think that setSelectedElement/getSelectedElement should not require <T> as a type but should accept/return MUIElement instead (but again --> API break :-\) anyways. > > But any of these alternatives could result in API breakage. Are there any > other alternatives, or isn't there a problem?
(In reply to Ed Merks from comment #10) > The question is where does this line of reasoning go? What must change? > What will be broken as a result? And what will be so much better as a > result? I just wrote a ModelAddon and noticed that "setParent" can't be applied, neither generic, nor if I even kown the actual type.. so I first assumed that this is simply missing in the API... > Certainly allowing setParent to be more easily called is not going > to make things more likely to be correct, but maybe you haven't reached that > conclusion yet. ... but as you pointed out setParent might simply be not suitable (and even dangerous) here anyways. So for me I can go ahead now and use getChilds().add(...) instead, but the issue itself remains that setParent is actually not defined in a consitent way.
