Community
Participate
Working Groups
In my applications of data binding I have required in two places an observable class for a static, constant value. I think this should be in core. Contribution (patch) attached.
Created attachment 84944 [details] Immutable static value observable
I agree that this would be good in core, however I'd rather this class was internal and its functionality available through static factory methods in the Observables class: IObservableValue staticObservableValue(Object value); IObservableValue staticObservableValue(Object value, Object valueType); IObservableValue staticObservableValue(Realm realm, Object value); IObservableValue staticObservableValue(Realm realm, Object value, Object valueType); This fits well with the other staticObservable(List|Set) methods already present in Observables.
(In reply to comment #2) > I agree that this would be good in core, however I'd rather this class was > internal and its functionality available through static factory methods in the > Observables class Agreed.
Created attachment 85041 [details] Static/constant value observable
Created attachment 85043 [details] Constant/Static value observable type. Proper contributor comments in this patch, forgot those.
A few things that need to be checked: * Checking for null realm in constructor * Checking whether we're running within the realm. * Calling getterCalled() in accessors (see IObservableValue, every method with @TrackedGetter in the javadoc must call ObservableTracker.getterCalled(this) before proceeding). * valueType parameter should be an Object argument, not Class. This is to support other concepts of type such as EMF's EClass, Groovy's MetaClass and so on. I'll start by attaching a test case for the first three points above. IObservableValue.setValue states that an UnsupportedOperationException will be thrown if setValue is not allowed. Personally I would prefer that ConstantValue threw this instead of BindingException. Which brings up a more general topic, should IObservables throw BindingException at all? I see this all over the place in JDB but it doesn't seem appropriate. Observables can be used without bindings, and getting a BindingException in those situations is confusing. If the Perhaps the observables should throw more generic runtime exceptions, and if a Binding catches an exception, it should a BindingException which wraps the observable's thrown exception. Just thought I'd bring it up.
Created attachment 85094 [details] Tests for IObservableValue contract
Created attachment 85095 [details] Tests for IObservableValue contract - updated contribution comments Whoops, forgot to update contribution comments :)
Created attachment 85100 [details] Test for null realm check in constructor
Created attachment 85334 [details] Immutable constant/static value observable
(In reply to comment #6) > A few things that need to be checked: > * Checking for null realm in constructor > * Checking whether we're running within the realm. > * Calling getterCalled() in accessors (see IObservableValue, every method with > @TrackedGetter in the javadoc must call ObservableTracker.getterCalled(this) > before proceeding). > * valueType parameter should be an Object argument, not Class. This is to > support other concepts of type such as EMF's EClass, Groovy's MetaClass and so > on. > New patch attached with IObservableValue contract checked and all the above points from Matt Hall all sorted. Thank Matt, this was helpful diligence and thanks for adding the test. Seems this bug needed a bit more work than the daft little class I initially thought it was :) Two more points for input if anyone has any: - ConstantValue could alternatively have been called StaticValue in line with *static*() methods in Observables. Static here however, such as in the Static case means "does not fire events" and "does not need disposing", the underlying list content is not immutable. I called it ConstantValue instead. If we prefer StaticValue it can be renamed. - WritableValue is not an internal class but we have made ConstantValue an internal one. Both can be constructed directly in client code. Why was WritableValue made API without the indirection through Observables? (Note: I prefer it to be directly usable)
Matt it seems to me that your third test patch replaces the previous ones unless I am mistaken. If so you might want to obsolete the last two by checking 'Obsolete' on the Details page. I can't do this for you. It can be done too on patch submission.
(Correction to comment #11) s/Static case/staticObservableList() case/
(In reply to comment #11) > Why was WritableValue made API without the indirection through Observables? To enable subclassing. Reasons for the indirection: * in future versions, implementation can be changed freely * makes it easy for clients to discover functionality * follows the Java collections framework example * clients cannot subclass (i.e. the subclass contract does not have to be specified) Reasons against it: * clients cannot subclass :)
> (In reply to comment #6) > Two more points for input if anyone has any: > - ConstantValue could alternatively have been called StaticValue in line with > *static*() methods in Observables. Static here however, such as in the Static > case means "does not fire events" and "does not need disposing", the underlying > list content is not immutable. I called it ConstantValue instead. If we prefer > StaticValue it can be renamed. I agree that "static" isn't appropriate. Some other possible names are ImmutableValue, UnmodifiableValue or FinalValue. There are two aspects that need to be considered separately that should be considered as we discuss new API: mutability and modifiability. Mutability indicates whether the object can change. Modifiability indicates whether the object may be changed through a given interface. ArrayList is mutable and modifiable. Collections.unmodifiableList(ArrayList) is mutable but not modifiable through the returned List. Collections.singletonList(T) is immutable and unmodifiable. Mutable, modifiable : WritableValue, WritableList Mutable, unmodifiable: Observables.unmodifiableObservableList() Immutable, unmodifiable: Observables.emptyObservableList() Immutable, modifiable: Not possible (modifiable implies mutable) I'm starting to wonder if the use of "static" is appropriate even for collections. It has nothing to do with the traditional meaning of the word in Java and doesn't really convey what the object does (I read the meaning of the method differently than you described). Perhaps it would be better to rename/redirect the staticObservable* methods to unmodifiableObservable*? Another thing that came up while writing the tests is that the base ObservableListContractTest assumes mutability, so I had to subclass the contract test just to disable all the tests that failed because of it. Sorry this is so disjointed, I hope all my points came across clearly.
Please see bug 213145 for my proposal to fix the observable contract tests with regard to comment 15 earlier.
(In light of comment #14) Perhaps ConstantValue should be exposed as API too then, it's the logical opposite of WritableValue. (In reply to comment #15) I agree with Matt's comments on the meaning of 'static' and the better terms available in line with the Java Collections API. I had to read all the nearby comments to understand the purpose of the Observables.static*() methods. > Perhaps it would be better to rename/redirect the staticObservable* methods to unmodifiableObservable*? +1 staticObservable*() are API though, so we'd obviously have to deprecate the existing methods and create renamed ones. I think the sooner the better. I wonder how many consumers use these methods.. few? I think Boris might disapprove of renaming existing API methods like this though.
Actually.. I don't like indirection at all unless it adds value. I think Observables should contain emptySet() and accessors for similar reusable objects but should not contain factory methods for objects that could be classes constructed just as easily and more naturally using their constructor. That's my opinion though :)
Looking at the file history I see that Observables.staticObservable* methods were present in the 3.3 release so cannot be changed now. I should have checked that before making such a sweeping suggestion. The most correct approach seems to be to fill out the javadoc. I have a pending patch for bug 208322 which does just this.
Taking ownership.
Created attachment 89221 [details] Updated patch with renames and test case
Created attachment 89222 [details] mylyn/context/zip
Released to HEAD > 20080213 Thanks, Matt!
Verified by code inspection in I20080327-0100
