Community
Participate
Working Groups
The classes and methods in org.eclipse.jface.databinding should get type parameters, in the same manner as the other databinding bundles: org.eclipse.core.databinding org.eclipse.core.databinding.observable org.eclipse.core.databinding.property
I have started to work on this, and plan to push a change in a week or a few.
(In reply to Jens Lideström from comment #1) > I have started to work on this, and plan to push a change in a week or a few. Any progress, Jens?
*** Bug 537918 has been marked as a duplicate of this bug. ***
(In reply to Lars Vogel from comment #2) > Any progress, Jens? Yes, I'm working on it. I have an almost finished prototype and a text with observations and reflections. I will publish it here in a few days so we can discuss the solution and problems I'm having. I also did org.eclipse.databinding.beans in the same time; updating the tests requires both plugins to be generified.
New Gerrit change created: https://git.eclipse.org/r/127805
I have worked on updating the org.eclipse.jface.databinding and org.eclipse.core.databinding.beans bundles with type parameters on relevant classes, most importantly on property and observable types. An unfinished prototype has been pushed to Gerrit. Most of the two bundles are updated, and some relevant tests and snippets. (I did org.eclipse.core.databinding.beans at the same time because the two plug-ins work in a similar way have have many similar difficulties.) In the following text I describe some observations, complications and open questions that I would like to discuss. There are quite a few problems that are hard to solve in a perfect way. The design decision often boils down to this: Is it okay that this update causes compilation errors for some clients, in order to get a better API? ### 1. Assignment of raw types to parameterized types This is a problem that seems hard to solve. Adding type parameters to the property factories (ex WidgetProperties) will probably cause compile errors in some code written since the introduction of type parameters in org.eclipse.core.databinding.observable and org.eclipse.core.databinding.property. People can use raw return type from WidgetProperties to assign the result to any property type. When type parameters are added that code do no longer type check if the types differ. For example, this code from SnippetSideEffectConditionalBinding.java: IObservableValue<Boolean> showDescription = WidgetProperties.selection().observe(showDescriptionButton); This snippet code has been updated after the addition of type parameters to IObservableValue, but before the current update of org.eclipse.jface.databinding. The raw IObservableValue return value from IValueProperty#observe is assigned to a IObservableValue<Boolean>. This code compiles when WidgetProperties#selection returns a raw type, but when WidgetProperties#selection is updated to have the return type IWidgetValueProperty<Widget, Object> the code no longer type checks. The problem is that SnippetSideEffectConditionalBinding uses Boolean as the value type of IObservableValue, but the correct type is Object. WidgetProperties#selection must return a property with value type Object, since that method is used for the selection of many different widgets, with different selection types. This problem makes it impossible to add specific type arguments to methods that return IValueProperty without introducing compile errors in some client code. I don't see any way to solve this. QUESTION: Can we accept that this update causes compile errors for people who assign raw IValueProperty and IObservableValue to parameterized versions of these types? (See also the section about WidgetProperties#selection below.) ### 2. The IWidgetXXXProperty#observe(Widget) methods The IWidgetValuesProperty class (and the other variants) have the method observe(Widget). Up until now this method has worked fine alongside the method IValuesProperty#observe(Object), but when that method is updated to have the signature observe(S), then the two methods can potentially collide. This has the following consequences: * It gets impossible to have a class implementing IWidgetValuesProperty<Widget, T>. Doing that results in an "ambiguous method" compile error. * It gets impossible to call either observe method on an IWidgetValuesProperty<Widget, T>. Doing that also results in an "ambiguous method" compile error. The point of observe(Widget) (I think) is that it returns ISWTObservableValue instead of IObservableValue. Since Java now-days allow covariant method return types there is no longer any need to keep the overloaded observe(Widget) to return ISWTObservableValue. (While fiddling around with this problem I ran into a type checking bug in ECJ, which is reported here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=536911) Potential solution: Because of above I think that the best solution is to remove the IWidgetValuesProperty#observe(Widget) method, and make IWidgetValuesProperty#observe(S) return ISWTObservableValue. The question is whether it is possible to do that in a backwards compatible way. I think it would not be a problem, but I'm not sure. (This is not implemented right now.) ### 3. Use of getClass as class argument to BeanProperties factory methods Many of the JFace snippets use bean properties in this pattern: ModelClass modelObj = ... IObservableValue modelProp = BeanProperties.value(modelObj.getClass(), "propName").observe(modelObj); This is hard to get to work nicely with a when BeanProperties#value has got a generic type. The most natural generic type for that method looks like this: <T> IBeanValueProperty<Object, T> value(Class<T> beanClass, String propertyName) In the example above the type T is inferred to be `? extends ModelClass`. This leads to a compilation error at `observe` because the wildcard type is not assignable from the type `ModelClass`. The only solution I can think of is to give `BeanProperties#value` and its comrades the following pretty useless type instead of the one above: IBeanValueProperty<Object, Object> value(Class<?> beanClass, String propertyName) QUESTION: Is it okay to cause some compilation errors for clients in this case? Or do we have to use the latter, overly lax type for the BeanProperties factory methods? See Snippet034ComboViewerAndEnum.java for an example of this. ### 4. WidgetProperties#XXXprop return value type QUESTION: Should the return value of the property factory methods be 1. IValueProperty<Control, T>, 2. or <S extends Control> IValueProperty<S, T>? That is, should the result be a property that works on a specific, caller-selected control type, or a property that works on all controls? * IValueProperty<Control, T> is simpler and easier to understand. * The result of <S extends Control> IValueProperty<S, T> can be assigned to different kinds of controls, for example to IValueProperty<Button, T>. This is useful if the user has to work with an API where the S parameter of IValueProperty is invariant. Right now alternative 1. is used for `WidgetProperties#background`; alternative 2. is used for `WidgetProperties#bounds`. I think alternative 1 is best. ### 5. BeanProperties#XXXprop return value type Analogous to the previous section above. Should the return value of the untyped property factory methods be (1) IValueProperty<Object, Object> or (2) IValueProperty<S, T>? ### 6. WidgetProperties#selection WidgetProperties#selection is used for widgets that have different kinds of selection. For example, Button has a boolean selection value, Combo has Object and Scale has int. So WidgetProperties#selection must return `IWidgetValueProperty<S, Object>`. I think the best way to handle this is to introduce widget specific versions of `WidgetProperties#selection`: `WidgetProperties#buttonSelection`, `WidgetProperties#comboSelection` etc. See for example SnippetSideEffectConditionalBinding.View#bindData. ### 7. Adding new methods for interfacing with non-generic code There is much code that is related to the databinding API which are not using generic types. This code instead uses Object for types that can contain different kinds of values. This is the way the databinding API worked before its generification. Such related code includes the JFace viewer API and the EMF databinding API. To make it easier and prettier to interface with such non-generic code it would be useful to introduce some util and convince methods to convert untyped databinding objects to object with more specific types. Here is a proposal for the addition of some such methods: IValueProperty would get the methods `castValue(Class<T>)` and `castSource(Class<T>)` These methods return a IValueProperty with value/source type T. They are added to the IValueProperty interface and implemented in ValueProperty. (It should not be be a technical problem to add methods to the IValueProperty interface, since it is @noimplement. Otherwise default methods could be used.) It could be implemented as a property decorator which checks the type of values when they are set. Such cast methods has been implemented in the prototype for IValueProperty and IListProperty.
Jens, you have a compile error, do you know how to examine the builds that were triggered by Gerrit? https://ci.eclipse.org/platform/job/eclipse.platform.ui-Gerrit/15675/console
(In reply to Wim Jongman from comment #9) > Jens, you have a compile error, do you know how to examine the builds that > were triggered by Gerrit? Yes, I've seen it, and I know how to examine the build log, but thanks for trying to help me out. :) Since I only work on this on my spare time I'm sometimes a bit unresponsive. I will fix it during the weekend. Anyway, since the current implementation is kind of a work-in-progress prototype the important thing is to get some feedback on the design and discuss the problems I have listed above, and a compilation error does not hider that.
I have put some though into how to handle the problems I listed in comment 6. This is how will do it (unless anyone have any other opinions): ### 1. Assignment of raw types to parameterized types I don't think there is anything to do to avoid this. (Apart from leaving *a lot* of method with raw types.) We just have to swallow it. Some clients will have compile errors because of this change. ### 2. The IWidgetXXXProperty#observe(Widget) methods I'll remove these methods and see if it works. ### 3. Use of getClass as class argument to BeanProperties factory methods So many of the examples and snippets use this pattern so I think I would be unwise to cause compile errors for clients who do that. Solution: Create the new class BeanPropertiesTyped, with all the same methods as BeanProperties, but will return types that are linked to the argument types. That is, the methods in BeanPropertiesTyped will look like this: <S> IBeanValueProperty<S, Object> value(Class<S> beanClass, String propertyName) The methods in BeanProperties will look like this: IBeanValueProperty<Object, Object> value(Class<?> beanClass, String propertyName) ### 4. WidgetProperties#XXXprop return value type (and BeanProperties) I will use IValueProperty<Control, T> for now. ### 6. WidgetProperties#selection I will make WidgetProperties#selection return IValueProperty<Control, Object> and add the methods WidgetProperties#buttonSelection, WidgetProperties#scaleSelection etc, that have Boolean and Integer as value types. ### 7. Adding new methods for interfacing with non-generic code I will add the methods `cast`, `castSource`, `castValue` and `castElement` to relevant property and observable classes.
(In reply to Jens Lideström from comment #11) > I have put some though into how to handle the problems I listed in comment > 6. This is how will do it (unless anyone have any other opinions): > > ### 1. Assignment of raw types to parameterized types > > I don't think there is anything to do to avoid this. (Apart from leaving *a > lot* of method with raw types.) We just have to swallow it. Some clients > will have compile errors because of this change. I agree, assigning a raw type to a typed variable is a programming error. If we respect such code, no generification can ever be done. > ### 2. The IWidgetXXXProperty#observe(Widget) methods > > I'll remove these methods and see if it works. ok. Should work. > ### 3. Use of getClass as class argument to BeanProperties factory methods > > So many of the examples and snippets use this pattern so I think I would be > unwise to cause compile errors for clients who do that. > > Solution: Create the new class BeanPropertiesTyped, with all the same > methods as BeanProperties, but will return types that are linked to the > argument types. > > That is, the methods in BeanPropertiesTyped will look like this: > > <S> IBeanValueProperty<S, Object> value(Class<S> beanClass, String > propertyName) > > The methods in BeanProperties will look like this: > > IBeanValueProperty<Object, Object> value(Class<?> beanClass, String > propertyName) > I agree, that <Object, Object> is not very nice, but where do we an advantage with <S, Object>? Do you have a code example, that gets nicer with the more specific types? > ### 4. WidgetProperties#XXXprop return value type (and BeanProperties) > > I will use IValueProperty<Control, T> for now. > ok. > ### 6. WidgetProperties#selection > > I will make WidgetProperties#selection return IValueProperty<Control, > Object> and add the methods WidgetProperties#buttonSelection, > WidgetProperties#scaleSelection etc, that have Boolean and Integer as value > types. > Is it possible to also name the methods selection, but these take a more specific type than Widget (e.g. Button, Scale)? > ### 7. Adding new methods for interfacing with non-generic code > > I will add the methods `cast`, `castSource`, `castValue` and `castElement` > to relevant property and observable classes. I don't understand, what this is good for. If it makes the internal code easier, we should not make it public, because public code needs to be maintained for years. And for me it sounds like temporary code.
Hello Conrad! Nice to get someone to discuss these things with! Answers to Conrad's remarks in comment 12: ### 2. The IWidgetXXXProperty#observe(Widget) methods >> I'll remove these methods and see if it works. > > ok. Should work. I came to think of a probably cause of problems with this: binary compatibility. Although this change probably compiles for all clients it will probably cause crashes for class files that call observe(Widget) if that method disappears. Probably with a NoSuchMethodError. I don't think that is acceptable. Although I really want to get rid of the observe(Widget) method to make the API cleaner, I don't think it's acceptable to break old code like that. I will read more about compatibility policy and binary compatibility. ### 3. Use of getClass as class argument to BeanProperties factory methods > I agree, that <Object, Object> is not very nice, but where do we an advantage > with <S, Object>? Do you have a code example, that gets nicer with the more > specific types? Almost every use of this API gets nicer with a more specific type! :) A typical usage example looks like this: // With specific types: IValueProperty<SomeBean, String> propTyped = BeanPropertiesTyped.value(SomeBean.class, "someName", String.class); // With unspecific types: IValueProperty<Object, Object> propUntyped = BeanProperties.value(someBean.getClass(), "someName", String.class); The type of propTyped makes the all code that uses it clearer, and some bugs are caught by the type checker. An alternative to creating the new class BeanPropertiesTyped is to make clients recover the specific type with a case: IValueProperty<SomeBean, String> propTyped = (IValueProperty<SomeBean, String>) BeanProperties.value(someBean.getClass(), "someName", String.class); But that is really long and ugly and unintuitive and verbose for clients. (I have also come to think about another complication with the type of the property factory methods, see below for more about that.) ### 6. WidgetProperties#selection >> ### 6. WidgetProperties#selection >> >> add the methods WidgetProperties#buttonSelection, WidgetProperties#scaleSelection etc > > Is it possible to also name the methods selection, but these take a more > specific type than Widget (e.g. Button, Scale)? No, they are zero-argument factory methods. No overloading is possible. ### 7. Adding new methods for interfacing with non-generic code >> I will add the methods `cast`, `castSource`, `castValue` and `castElement` >> to relevant property and observable classes. > > I don't understand, what this is good for. If it makes the internal code > easier, we should not make it public, because public code needs to be > maintained for years. And for me it sounds like temporary code. It's particularly nice to discuss this subject, because it would be a big change! > public code needs to be maintained for years I agree, we should take this seriously and think carefully about it before doing anything These cast methods are added purely for the sake of the clients to be able to conveniently work with typed databinding classes. The motivation is this: I believe that there will be many places where clients of the databinding API will have to continue to work with more or less untyped databinding classes, such as IValueProperty<Object, Object> or IValueProperty<?, ?>. Clients will want to convert from these types to the more specific types which they know that the objects have. It is possible to convert the types with normal Java casts, but that is verbose and ugly, to the point where the resulting code is more complicated, verbose and ugly compared to the situation before type parameters were added. I think it would be a good idea to provide a better way for clients to perform this task. Example: // With normal Java cast: IValueProperty<ISelectionProvider, SomeEmfObject> prop = (IValueProperty<ISelectionProvider, SomeEmfObject>) ViewerProperties.selection(); // With new castValue method: IValueProperty<ISelectionProvider, SomeEmfObject> prop = ViewerProperties.selection().castValue(SomeEmfObject.class); There are several reasons for why all databinding code can't have specific type parameters: * Code that uses libraries that are (and will remain) untyped. For example: GUI widgets, the JFace viewer API or the EMF databinding code. * Some things are simply hard or impossible to make generic. It might for example be necessary to store property objects of mixed types in a container. * Much client code has been written without types in mind, because there are been no types for so long. It is not realistic to update all such code with type parameters, and it is desirable to be able to I think we should do more than just add type parameters to the databinding API. We should also make it convenient to work with the typed API. These new casting methods help with that.
I have performed another iteration of work which I have pushed as patch set 6. Updates: * All code is updated, and should be close to a finished state. * All snippets are updated. * Added WidgetProperties#selection variants for specific controls. * Observable content providers and related classes have got type parameters. * Property factory methods have consistently got universally qualified return types. Remaining work: * Update Javadoc. * Update tests. * Find an Eclipse project which makes heavy use of databinding and update that one to get experience with some real code. ## The trickiest problem: Backwards compatibility The trickiest problem in this change is to decide how much backwards compatibility that should be broken and how much to preserve. I have studied the documents on backwards compatibility on the Eclipse wiki[1] on this issue. My conclusion is that it is okay to sacrifice some source compatibility, but that we should really try to have full binary compatibility. The implies that I can make a better solution for some of the problems that I've posted (#1, #3) but have to accept sub-optimal solutions in other cases (#2). [1]: https://wiki.eclipse.org/Evolving_Java-based_APIs#API_Prime_Directive ===================== ### 8. New question: Type parameters for the deprecated XXXObservables classes? Should I add type parameters to the deprecated classes SWTObservables, BeansObservables and PojoObservables? I think we should not touch the deprecated classes, just leave them as they are. ### 9. New question: Should observable content and label providers get type parameters? The normal JFace content and label providers are untyped. Type parameters seems somewhat useful to me. I added them in the latest changeset, but I am not sure that they should stay. ===================== ## Status of old issues ### 2. The IWidgetXXXProperty#observe(Widget) methods I'll keep these method for binary backwards compatibility. Even if that makes a worse API. ### 3. Use of getClass as class argument to BeanProperties factory methods > Solution: Create the new class BeanPropertiesTyped, with all the same methods as BeanProperties, but will return types that are linked to the argument types. I changed my mind! According to the "API Prime Directive" [1] it might be okay to break source compatibility in limited ways, so I think this break is justified. I've given BeanProperties specific types, and I won't create BeanPropertiesTyped. [1]: https://wiki.eclipse.org/Evolving_Java-based_APIs#API_Prime_Directive ### 4. WidgetProperties#XXXprop return value type (and BeanProperties) ### 5. BeanProperties#XXXprop return value type > QUESTION: Should the return value of the property factory methods be > > 1. IValueProperty<Control, T>, > 2. or <S extends Control> IValueProperty<S, T>? > >> I will use IValueProperty<Control, T> for now. I changed my mind! I think it is better to instead use alternative 2 (<S extends Control> IValueProperty<S, T>). Because that pattern is already used in org.eclipse.core.databinding.property.Properties. (I think that choice was made by Stefan Xanos.) ### 6. WidgetProperties#selection > I will make WidgetProperties#selection return IValueProperty<Control, Object> and add the methods WidgetProperties#buttonSelection, WidgetProperties#scaleSelection etc, that have Boolean and Integer as value types. This has been implemented. ### 7. Adding new methods for interfacing with non-generic code > I will add the methods `cast`, `castSource`, `castValue` and `castElement` to relevant property and observable classes. These methods are present on property classes in the latest changeset. The following snippet is an example of when generification is hard and cast methods will come in handy: Snippet026AnonymousBeanProperties.java The code is written in a way that mixes unrelated object types in the same variables and collections. To add generic types to that kind of code it will be necessary to cast property and observable types. I fear that making clients cast property values with normal Java casts will cause much irritation with the generified API, because it is extremely verbose. Adding cast methods makes it a little prettier for clients to solve this.
Patch set 12 updated: https://git.eclipse.org/r/#/c/127805/12 Updates: ### 7. Adding new methods for interfacing with non-generic code I've removed the cast methods in this patch set. Although I still thing they would be a good idea I think it's best to treat them in a separate change. I'll create a separate bug report for that.
Why hasn't the ViewersObservables classes been deprecated after the introduction of the ViewerProperties? In the same way as SWTObservables has been deprecated after the introduction of WidgetProperties.
With Patch set 20 this thing is getting close to completion. :) I don't think there is any more code to update or more decisions to make. I've postponed updating the tests. Since in a lot of places that involves switching from XxxObservables to XxxProperties updating the tests involves more than adding type parameters, which makes it a big enough change to warrant its own ticket. I plan to do the following before it is time to merge this change: * Look though all the changes and make small corrections. * Find another Eclipse project which makes extensive use of databinding and update it to use type parameters, to get experience with how this change works with real code. It is also time to start with the review process by another developer.
@Lars Vogel, @Conrad Groth, @Wim Jongman, and others: I want to find an Eclipse projects which makes extensive use of databinding. My plan is to update it to use the parametrized databinding classes, to get experience of how the solution works out in real code. Do your have any suggestion for a suitable project?
e4 tooling does but it uses EMF-Databinding so I doubt it would help you in this regard. But it can help you to make sure that: * did not break binary compability (very important) * did not break source compability (important but not as much as binary) The "problem" we have with breaking changes in Databinding from EMF-Db point of view is that we still support old versions of Eclipse hence we rely on Eclipse-Db evolved binary compatible.
Interesting! Much of this work has already been done for the e4 databinding project: https://git.eclipse.org/c/e4/org.eclipse.e4.databinding.git/tree/bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/swt https://git.eclipse.org/c/e4/org.eclipse.e4.databinding.git/tree/bundles/org.eclipse.core.databinding.beans/src/org/eclipse/core/databinding/beans Well, these things happens...! Now I can compare their solution to mine, that's good. It's like these separate redundant implementation that they sometimes do for safety critical aircraft systems. As I understand it the e4 databinding fork was an experiment that where never fully finished and taken into production. Is that correct?
(In reply to Jens Lideström from comment #20) > Interesting! Much of this work has already been done for the e4 databinding > project: > > https://git.eclipse.org/c/e4/org.eclipse.e4.databinding.git/tree/bundles/org. > eclipse.jface.databinding/src/org/eclipse/jface/databinding/swt > https://git.eclipse.org/c/e4/org.eclipse.e4.databinding.git/tree/bundles/org. > eclipse.core.databinding.beans/src/org/eclipse/core/databinding/beans > > Well, these things happens...! Now I can compare their solution to mine, > that's good. It's like these separate redundant implementation that they > sometimes do for safety critical aircraft systems. > > As I understand it the e4 databinding fork was an experiment that where > never fully finished and taken into production. Is that correct? Correct
### 2. The IWidgetXXXProperty#observe(Widget) methods > I'll keep these method for binary backwards compatibility. Even if that makes a worse API. It turns out that it is possible to change these methods and still preserve backwards compatibility. The methods that are generated when observe(Widget) is removed and observe(S) is given the return type ISWTXXObservable include methods with the same signature and the old method. Hence this change is backwards compatible! In Patch set 22 I have made that change for the Widget properties: https://git.eclipse.org/r/#/c/127805/22 It is not possible to do this for the Viewer properties however, because they work for also for classes that don't extend Viewer. Anyone how is interested in the details can have a look below at the generated code for IWidgetValueProperty after the change: $ javap -s IWidgetValueProperty.class Compiled from "IWidgetValueProperty.java" public interface IWidgetValueProperty<S extends Widget, T> extends IValueProperty<S, T> { // This method is generated because of the covarienat return type: public abstract ISWTObservableValue<T> observe(S); descriptor: (Lorg/eclipse/swt/widgets/Widget;)Lorg/eclipse/jface/databinding/swt/ISWTObservableValue; // This is the normal override, without covariant return type public IObservableValue observe(Object); descriptor: (Ljava/lang/Object;)Lorg/eclipse/core/databinding/observable/value/IObservableValue; public abstract ISWTObservableValue<T> observeDelayed(int, S); descriptor: (ILorg/eclipse/swt/widgets/Widget;)Lorg/eclipse/jface/databinding/swt/ISWTObservableValue; }
Created attachment 276463 [details] Jface usage of stock eclipse Jens see the attachment for usage of databinding. Please also take a look at this: https://git.eclipse.org/c/windowbuilder/org.eclipse.windowbuilder.git/tree/org.eclipse.wb.rcp.doc.user/html/features/swt/data_binding
(In reply to Wim Jongman from comment #23) > Jens see the attachment for usage of databinding. Please also take a look at > this: > > https://git.eclipse.org/c/windowbuilder/org.eclipse.windowbuilder.git/tree/ > org.eclipse.wb.rcp.doc.user/html/features/swt/data_binding Thanks! Here I have much to work with. Someone should update Window Build to use type parameters at some point...
Status report: I have not had much time to do work on this. I have done a little work adding type arguments to the E4 tools. Here are some observations: * The generified databinding classes seem to work fine. * It is super ugly and awkward to use the gentrified databinding classes together with EMF databinding. * It is simple to generify code if you do the simplest and minimalest solution possible. But code that uses databinding often uses Object and EObject in a lot of places, and it would be nice to add type parameters in many places. This however is tricky and requires much effort. So I'll do the minimal solution from now on. * There are compile errors in a few places as a result of the addition of type parameters. I will slowly continue to work on this.
Thanks for the update Jens.
I made a separate ticket for the work with adding type arguments to the e4 tools code: https://bugs.eclipse.org/bugs/show_bug.cgi?id=544303
(In reply to Jens Lideström from comment #25) > I will slowly continue to work on this. Jens, it is realistic to plan this for early 4.12? It would be great if you work can finally find its way into the official code line.
(In reply to Lars Vogel from comment #28) > Jens, it is realistic to plan this for early 4.12? It would be great if you > work can finally find its way into the official code line. I don't know when 4.12? Is that "month.day"? The work in this in this ticket is basically complete. I think it's only a couple of hours work left. I think I can squeeze that in during the coming week or so. The following is on my plan of things to do: * Test the binary compatibility of the changes. * Write an entry for "News and Noteworthy". * Review the examples. I think that is it...
Note: These changes give 19 compile errors in the examples, and 3 compile errors in E4 Tools. (Curiously, no compile error were cause in the tests.)
(In reply to Jens Lideström from comment #30) > Note: These changes give 19 compile errors in the examples, and 3 compile > errors in E4 Tools. (Curiously, no compile error were cause in the tests.) Can you provide more details about the nature of these errors. It would be pretty annoying (and bad in my opinion), if downstream consumers end up with errors when they use the last version of the platform with these changes. It seems to me in the worst case they should see only raw type warnings... Also, is there some way to provide a build with these changes so that consumers and anticipate/analyze the impact of these changes?
(In reply to Ed Merks from comment #31) > Can you provide more details about the nature of these errors. The errors are mostly related to the issues that is described in section 1. and 3. in my comments above. Here are some example: IObservableValue<Boolean> showDescription = WidgetProperties.selection().observe(showDescriptionButton); Previously `observe` returned a raw type, `IObservableValue`, which could be assigned to `IObservableValue<Boolean>`. After the update however it returns `IObservableValue<T>`, which gets inferred as `IObservableValue<Object>`, which is not assignable to `IObservableValue<Boolean>`. The only way I can think of to avoid that clients get to suffer from such errors is to abstain from updating `WidgetProperties` (and related classes), and instead create new factory classes that return typed properties, for example `WidgetPropertiesTyped`. > It would be pretty annoying (and bad in my opinion), if downstream consumers end up with errors when they use the last version of the platform with these changes. It seems to me in the worst case they should see only raw type warnings... I agree that it is annoying. It's a good thing that we get to discuss the issue again. The question is this: what is more annoying? To accept some compile errors for clients now, or to forever after live with two versions of the property factory classes? (These classes include WidgetProperties, ViewerProperties, BeanProperties and PojoProperties.) The discussion earlier in this bug landed in that it is better to accept a few compile errors now. Note that the functionality has not changed anywhere, and that the compile errors should be easy to resolve. Also not that the changes are binary compatible. > Also, is there some way to provide a build with these changes so that > consumers and anticipate/analyze the impact of these changes? That might be good idea. I don't know how to do that in the best way. This was not done for the previous additions of type parameters, I think. Suggestions and advice about how to do this are welcome!
I have tried to verify the binary compatibility of the changes. They seem binary compatible to me. That is, a compiled version of the new updated databinding bundles will work at runtime with code that is compiled for an old version of the bundles. The bundles and their versions before and after the update are these: org.eclipse.jface.databinding Old-version: 1.8.500, Updated-version: 1.9.0 org.eclipse.core.databinding.beans Old-version: 1.4.400 Updated version: 1.5.0 Old version source: https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/ac93f51cfc5352c24baa84b54713f911ff1d250f Updated version source: Gerrit https://git.eclipse.org/r/#/c/127805/28 I have tested this is the following ways: ## 1. Using the e4 fragment editor in with new versions of the bundles I used an existing Eclipse installation (Eclipse IDE for RCP and RAP Developers 2018-12), and replaced the Beans and JFace databinding jars with updated versions. (I had to create updated databinding jars with the original version numbers to make this work.) ## 2. Running the databinding tests with updated bundles I compiled the non-updated tests in org.eclipse.jface.tests.databinding against the old version of the databinding bundles. Then I run the tests with the updated version of the databinding bundles. The tests worked fine in this configuration, with no failures. It was I little tricky to compile and run the tests against a different versions of the databinding bundles. To manage that I did this: 1. I compiled the tests to a jar file, using the non-updated databinding code. 2. Then I compiled the updated databinding bundles to jar files. 3. Then I created a new test project, in which I included the above-mentioned jars. 4. Using that test project I could use JDTs JUnit feature to run the tests class files. ## 3. Using compatibility analysis tools I have run the Revapi[1] and JAPICC[2] analysis tools to check new updated against the non-updated versions of the databinding jars. These tools accept the Beans jar, but they report problems for the JFace jar. But judging from the problem description texts it seems like the tools can not correctly handle some of the changes that are made. For example, JAPICC reports that the IWidgetValuePropty interface has been removed from the WidgetValuePropty class, when in fact type arguments have been added to that interface. After examining these reports, it is my conclusion that the the compatibility warnings they report are bogus, and I think that the changes are compatible. I have attached the reports from these tools to this bug report as the following files: org.eclipse.core.databinding.beans_JAPICC_report.html org.eclipse.core.databinding.beans_revapi_report.txt org.eclipse.jface.databinding_JAPICC_report.html org.eclipse.jface.databinding_revapi_report.txt [1]: https://revapi.org/ [2]: https://lvc.github.io/japi-compliance-checker/
Created attachment 277982 [details] Report from checking the org.eclipse.core.databinding.beans bundle with the JAPICC tool.
Created attachment 277983 [details] Report from checking the org.eclipse.jface.databinding bundle with the JAPICC tool.
Created attachment 277984 [details] Report from checking the org.eclipse.jface.databinding bundle with the Revapi tool.
Created attachment 277985 [details] Report from checking the org.eclipse.core.databinding.beans bundle with the Revapi tool
Created attachment 277986 [details] Report from checking the org.eclipse.jface.databinding bundle with the JAPICC tool.
Created attachment 277987 [details] Report from checking the org.eclipse.core.databinding.beans bundle with the Revapi tool
I created a News and Noteworthy entry: https://git.eclipse.org/r/#/c/140937/
New Gerrit change created: https://git.eclipse.org/r/141106
Gerrit change https://git.eclipse.org/r/127805 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0475aa4a1e912f18eec6b857bfb17b9e46b15350
Thanks Jens, looks very good to me. Examples, e4 tools and tests still compile also the customer project compiles without issues. Test are still passing. I merge this so that we can ask more people to test this. I plan to merge the update of the tests and examples tomorrow so that the nightly build has a chance to identify more issues. Again many thanks for this sisyphus work.
(In reply to Lars Vogel from comment #43) > Again many thanks for this sisyphus work. Thanks Jens. Well done.
(In reply to Wim Jongman from comment #44) > (In reply to Lars Vogel from comment #43) > > > Again many thanks for this sisyphus work. > > Thanks Jens. Well done. :)
This causes API Tools errors (can be seen by simply starting off a new workspace). Why did you not notice those??? Description Resource Path Location Type The minor version should be the same for version 1.7.0, since no new APIs have been added since version 1.6.400 MANIFEST.MF /org.eclipse.core.databinding.observable/META-INF line 5 Version Numbering Problem The minor version should be the same for version 1.7.0, since no new APIs have been added since version 1.6.400 MANIFEST.MF /org.eclipse.core.databinding.property/META-INF line 5 Version Numbering Problem Please fix.
(In reply to Dani Megert from comment #46) > Why did you not notice those??? We have you! ;)
(In reply to Wim Jongman from comment #47) > (In reply to Dani Megert from comment #46) > > Why did you not notice those??? > > We have you! ;) ;-)
OK, I am currently puzzled. I have two workspace where one reports the errors and one does not :-(
(In reply to Dani Megert from comment #49) > OK, I am currently puzzled. I have two workspace where one reports the > errors and one does not :-( Clear is, there was no need to increase the minor version as there was not API change.
(In reply to Dani Megert from comment #48) > (In reply to Wim Jongman from comment #47) > > (In reply to Dani Megert from comment #46) > > > Why did you not notice those??? > > > > We have you! ;) > > ;-) Finally, Dani see the same API tools issues as I do. :-)
(In reply to Dani Megert from comment #50) > (In reply to Dani Megert from comment #49) > > OK, I am currently puzzled. I have two workspace where one reports the > > errors and one does not :-( > Clear is, there was no need to increase the minor version as there was not > API change. I think our wiki say that generification works requires update if the minor version. I can check tomorrow.
See generic_types_and_methods_into_generic_ones and https://wiki.eclipse.org/Version_Numbering#When_to_change_the_minor_segment
(In reply to Lars Vogel from comment #53) > See generic_types_and_methods_into_generic_ones and > https://wiki.eclipse.org/Version_Numbering#When_to_change_the_minor_segment https://wiki.eclipse.org/Evolving_Java-based_APIs_2
(In reply to Dani Megert from comment #49) > OK, I am currently puzzled. I have two workspace where one reports the > errors and one does not :-( I see this too, but once I refresh/rebuild/restart it usually goes away. A new magic capability of the PDE *.target editor, Bug 546183, is to corrupt the active target platform definition. This gives wonderfully obscure behaviors.
(In reply to Dani Megert from comment #50) > (In reply to Dani Megert from comment #49) > > OK, I am currently puzzled. I have two workspace where one reports the > > errors and one does not :-( > Clear is, there was no need to increase the minor version as there was not > API change. I think that statement was wrong, see the links I provided.
(In reply to Lars Vogel from comment #51) > (In reply to Dani Megert from comment #48) > > (In reply to Wim Jongman from comment #47) > > > (In reply to Dani Megert from comment #46) > > > > Why did you not notice those??? > > > > > > We have you! ;) > > > > ;-) > > Finally, Dani see the same API tools issues as I do. :-) No I don't. It was a setup issue on my side: The corresponding preference is taken from the workspace. In my "normal" workspace it is set to error but in the new one it uses the default, which is 'Warning'. So, I see the problems in all workspace but just not as as errors in all of them. Please fix the problems by adding API problem filters. Do not decrease the version since the new one is already out by now. The latest build results confirm it too: https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/apitools/analysis/html/org.eclipse.core.databinding.observable/report.html https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/apitools/analysis/html/org.eclipse.core.databinding.property/report.html
(In reply to Lars Vogel from comment #56) > (In reply to Dani Megert from comment #50) > > (In reply to Dani Megert from comment #49) > > > OK, I am currently puzzled. I have two workspace where one reports the > > > errors and one does not :-( > > Clear is, there was no need to increase the minor version as there was not > > API change. > > I think that statement was wrong, see the links I provided. It's a corner case and increasing is also OK, but API Tools can't deal with that specific case and hence reports a problem which needs to be filtered.
(In reply to Dani Megert from comment #58) > > I think that statement was wrong, see the links I provided. > It's a corner case and increasing is also OK, but API Tools can't deal with > that specific case and hence reports a problem which needs to be filtered. AFAICS the increase of the minor version was necessary not only OK. API tools do not catch them but our documentation requires it.
(In reply to Dani Megert from comment #57) > Please fix the problems by adding API problem filters. Do not decrease the > version since the new one is already out by now. I can have a look at this during the weekend. I'll add problem filters for the version increments and try to figure out how the handle the "implements non-API interface" warnings.
Note that this also causes new compile warnings in the official build: https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/compilelogs/plugins/org.eclipse.e4.tools_4.8.200.v20190211-1025/@dot.html https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/compilelogs/plugins/org.eclipse.tools.layout.spy_1.0.200.v20180827-1331/@dot.html
(In reply to Dani Megert from comment #57) > The latest build results confirm it too: > > https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/ > apitools/analysis/html/org.eclipse.core.databinding.observable/report.html > > https://download.eclipse.org/eclipse/downloads/drops4/I20190425-1800/ > apitools/analysis/html/org.eclipse.core.databinding.property/report.html We already have API filter, e.g., org.eclipse.core.databinding.observable and I also do not see any API tool filter in my IDE setup. <?xml version="1.0" encoding="UTF-8" standalone="no"?> <component id="org.eclipse.core.databinding.observable" version="2"> <resource path="META-INF/MANIFEST.MF"> <filter comment="Inc version to 1.7.0 after adding wildcards to parameters. Bug 531748." id="926941240"> <message_arguments> <message_argument value="1.7.0"/> <message_argument value="1.6.300"/> </message_arguments> </filter> </resource> </component> Maybe another API Tools issue? The other messages, e.g., org.eclipse.core.databinding.observable.AbstractObservable implements non-API interface org.eclipse.core.databinding.observable.IObservable) seem wrong. Reopened https://bugs.eclipse.org/bugs/show_bug.cgi?id=546670 Dani, please try the API tools quickfix locally and post the difference to our existing filter into this bug. In several cases this fix my error locally for me without changing actually the real filter. Please do a full clean ws build before that.
Just to be clear: only the version warnings need to be fixed. The other ones are there for a long time. To spare us all time I've fixed it via https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f124bd18f674508afa7bddb2da6b2da67cbb648a The compile warnings from comment 61 still need to be addressed.
(In reply to Dani Megert from comment #63) > To spare us all time I've fixed it via > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=f124bd18f674508afa7bddb2da6b2da67cbb648a Thanks. API tools still driving me crazy. If I revert your change I now also get a API tools warning, something was not able to see earlier.
Gerrit change https://git.eclipse.org/r/141106 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c2459deea0d6625b246c02119084c747fa6566da
New Gerrit change created: https://git.eclipse.org/r/141273
New Gerrit change created: https://git.eclipse.org/r/141272
Gerrit change https://git.eclipse.org/r/141272 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9c1bbf836b5faa57710a0de115d2110172c95338
Gerrit change https://git.eclipse.org/r/140937 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=342d489f09ed3f70b170bd011d5bbd13885d98cd
Marking as fixed, remaining work with be done via new bug reports.
New Gerrit change created: https://git.eclipse.org/r/146791
Gerrit change https://git.eclipse.org/r/146791 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9e99f10da80dd0ca2418d82913bae2d98405a39d
Jens, this bug is closed. Please open new ones.
(In reply to Wim Jongman from comment #73) > Jens, this bug is closed. Please open new ones. Ops, I but the wrong bug number in the commit message, sorry. There is a ticket for the problem that is fixed by that commit, bug 549659.
