Bug 175737 - [DataBinding] ListDetailObservableList
Summary: [DataBinding] ListDetailObservableList
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.5 M5   Edit
Assignee: Matthew Hall CLA
QA Contact:
URL:
Whiteboard:
Keywords: bugday, helpwanted
Depends on:
Blocks:
 
Reported: 2007-02-27 14:53 EST by Peter Centgraf CLA
Modified: 2010-01-13 12:13 EST (History)
7 users (show)

See Also:


Attachments
proposed patch along with some unit tests (22.38 KB, patch)
2008-07-19 15:14 EDT, Ovidio Mallo CLA
no flags Details | Diff
second approach to implementing the LDOL (6.42 KB, patch)
2008-10-18 13:29 EDT, Ovidio Mallo CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Centgraf CLA 2007-02-27 14:53:38 EST
I want to use DataBinding APIs to implement a form-style editor that acts on a viewer multiple-selection.  I've just submitted a possible solution (in bug 124683) for observing the multiple-selection.  The next stage in the pipeline is to translate an IObservableList of masters into a IObservableList of details.  Finally, a DuplexingObservableValue (bug 175735) should translate an IObservableList of separate detail IObservableValues into a single IObservableValue that can be bound to an SWTObservableValue target.

ListDetailObservableList would perform the same function as DetailObservableValue, but for each item in an IObservableList.  This could be implemented as a subclass of ObservableList in which wrappedList contains DetailObservableValues.  LDOL would respond to ListChangeEvents on the source list by creating or destroying DetailObservableValues in the wrappedList.

If the source list does not contain IObservableValues, LDOL should wrap each item in a static, unmodifiable implementation of IObservableValue, so that DetailObservableValue can use them as masters.  This new type of IObservableValue would act as a write-once box, providing a real getValue() but stubs for everything else.  For this use case, the meaningful change events are generated by the source list, not the values it contains.
Comment 1 Ovidio Mallo CLA 2008-07-19 15:14:19 EDT
Created attachment 107883 [details]
proposed patch along with some unit tests

I liked Peter's idea so I implemented it :).

What I have not implemented so far is the support for a master list whose elements are not of type IObservableValue. I think this part should be implemented as an additional ObservableList which wraps another's list elements into IObservableValues and I was wondering whether having such a wrapping (and accordingly an unwrapping) list would be useful enough in practice to make such a list accessible to clients?

In any case, I think the wrapping part could be tracked in a separate bug if others agree.

By the way, one of the unit tests is currently failing but merely due to bug #241318.
Comment 2 Matthew Hall CLA 2008-07-21 13:33:44 EDT
I was thinking about this too and was considering using IValueProperty from bug 194734 to accomplish it.
Comment 3 Ovidio Mallo CLA 2008-07-21 15:56:51 EDT
Matthew, could you maybe further explain what you intended to use the IValueProperty class for? Maybe in place of the DetailObservableValues?
Comment 4 Matthew Hall CLA 2008-07-21 16:06:39 EDT
I think a code example would explain best:

class Person {
  private String name;
  public Person(String name) {
    setName(name);
  }
  public void setName(String name) {
    firePropertyChange("name", this.name, this.name = name);
  }
  public String getName() {
    return name;
  }
  /* addPropertyChangeListener and removePropertyChangeListener methods implied */
}

IValueProperty personName = BeanProperties.valueProperty(Person.class, "name");
IObservableList people = new WritableList();
people.add(new Person("Tom"));
people.add(new Person("Jerry"));

IObservableList peopleNames = new ListDetailObservableList(people, personName);
assertEquals(Arrays.asList("Tom", "Jerry), peopleNames);
Comment 5 Ovidio Mallo CLA 2008-07-22 05:29:40 EDT
Thanks for the piece of code, Matthew. I'm now getting the point.

Your usage of the IValueProperty class is interesting since it essentially makes it unnecessary for the master list to contain IObservableValues and you are furthermore directly getting a list of detail attributes and not of detail observable values containing the actual attributes, so the whole wrapping/unwrapping story I was mentioning would become obsolete.

However, a drawback may be that the IValueProperty kind of introduces a "new concept" since it handles part of the logic otherwise located in the IObservableValue (get/set/listen to value) and it completely replaces the otherwise used detail IObservableFactory so the pattern for clients would be somewhat different from the one used for plain DetailObservableValues. But beside that I very much like your approach as it does not involve that many observables as in my implementation.
Comment 6 Ovidio Mallo CLA 2008-07-22 05:40:32 EDT
By the way, my current implementation could also be adapted to make the LDOL contain the actual model detail attributes as elements instead of DetailObservableValues by allowing the user to write to list elements (but not do any structural changes to the LDOL). Internally, you would of course still need the DetailObservableValues but there is maybe no need to expose them to clients. I think that would definitely be an improvement.
Comment 7 Matthew Hall CLA 2008-07-22 10:26:08 EDT
If the IProperty APIs become good enough, my hope is to retrofit the majority of our internal classes to use properties and eliminate many of the custom observable implementations.  Thankfully most of our observables are hidden in internal classes so this change would be mostly transparent.
Comment 8 Ovidio Mallo CLA 2008-10-18 13:23:38 EDT
I've been thinking about this one again and I think an easy way to equally support IObservableValues as well as plain Objects in the source (master) list would be by letting the passed-in IObservableFactory making the distinction between those two cases. This means that if e.g. the source list contains normal Objects, you would do something like:

  new ListDetailObservableList(
      masterList, // contains plain Objects
      BeansObservables.valueFactory("name"), // factory for normal bean
      String.class);

For the case in which the source list contains IObservableValues, we could offer a new observable factory as API (or else, clients could do so):

  public static IObservableFactory BeansObservables#detailValueFactory(
      final String propertyName,
      final Class propertyType) {
    return new IObservableFactory() {
      public IObservable createObservable(Object target) {
        IObservableValue master = (IObservableValue) target;
        return MasterDetailObservables.detailValue(
            master,
            valueFactory(realm, propertyName),
            propertyType);
      }
    };
  }

Then, you could simply use that factory instead the original one:

  new ListDetailObservableList(
      masterList, // contains IObservableValue masters
      BeansObservables.detailValueFactory("name", String.class), // factory for masters
      String.class);

I'm attaching a draft implementation of the LDOL (it's not much different from the old one) which:
* does not anymore assume that the source list always contains IObservableValues
* has the actual detail attributes as list elements (and not anymore the IObservableValues which should be more handy)
I think that this way the usage for clients would be very simple in both cases.

Please note that the implementation is untested yet and is mainly thought to capture the idea. Feedback is welcome.
Comment 9 Ovidio Mallo CLA 2008-10-18 13:29:24 EDT
Created attachment 115503 [details]
second approach to implementing the LDOL
Comment 10 Matthew Hall CLA 2009-01-21 19:10:56 EST
This can be achieved in a single statement using the new properties API (bug 194734), which was just released to HEAD and will be in 3.5M5:

IObservableList namesOfFriends = BeanProperties
    .list(Person.class, "friends").values("name").observe(person);

It can also be used with existing observables:

IObservableList friends = BeansObservables.observeList(person, "friends");
IObservableList namesOfFriends = BeanProperties
    .value(Person.class, "name").observeDetail(friends);
Comment 11 Matthew Hall CLA 2009-02-04 19:33:54 EST
Ovidio, have you looked at the properties API yet?  Does IListProperty.values(IValueProperty).observe() satisfy your use case?
Comment 12 Matthew Hall CLA 2009-02-05 14:11:06 EST
(In reply to comment #5)
> However, a drawback may be that the IValueProperty kind of introduces a "new
> concept" since it handles part of the logic otherwise located in the
> IObservableValue (get/set/listen to value) and it completely replaces the
> otherwise used detail IObservableFactory so the pattern for clients would be
> somewhat different from the one used for plain DetailObservableValues. But
> beside that I very much like your approach as it does not involve that many
> observables as in my implementation.

Indeed, that is quite the point of IValueProperty: to be able to extract the get/set/listen parts from the observable into a strategy, so that clients can get observables of their custom properties without having to conform to all the special case rules that apply to observables.

As for IObservableFactory, we still use it with properties whenever the property source is a single value.  IObservableFactory is a good pattern here so there's no reason to stop using it.  However when you're talking about projecting some kind of property over a whole list, I think IObservableFactory breaks down.

I've reviewed the thread all over just to make sure I haven't missed any specific use cases.  This one:

(In reply to comment #8)
> new ListDetailObservableList(
> masterList, // contains plain Objects
> BeansObservables.valueFactory("name"), // factory for normal bean
> String.class);

..can be done with properties like this:

IObservableList list = BeanProperties.value("name", String.class).observeDetail(masterList);

Closing as fixed since all use cases mentioned appear to be satisfied by the new IProperty API.  If I'm wrong please reopen or file a new bug.
Comment 13 Matthew Hall CLA 2009-02-05 14:13:23 EST
IListProperty was released in 3.5M5
Comment 14 Ovidio Mallo CLA 2009-02-07 08:21:25 EST
(In reply to comment #11)
> Ovidio, have you looked at the properties API yet?  Does
> IListProperty.values(IValueProperty).observe() satisfy your use case?
Matthew, I still need to have a closer look at the new API but from what I have seen so far it does support the use cases of this bug and it does so in a very elegant way! Thanks for the above explanations and great work!
Comment 15 Matthew Hall CLA 2009-04-29 02:28:05 EDT
Verified in I20090428-0100