Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 237718 - [DataBinding] Introduce decorator observable classes
Summary: [DataBinding] Introduce decorator observable classes
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: 3.5 M2   Edit
Assignee: Matthew Hall CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 230189
Blocks:
  Show dependency tree
 
Reported: 2008-06-18 19:56 EDT by Matthew Hall CLA
Modified: 2008-12-10 00:51 EST (History)
2 users (show)

See Also:


Attachments
Prototype (26.79 KB, patch)
2008-07-10 20:26 EDT, Matthew Hall CLA
no flags Details | Diff
mylyn/context/zip (82.92 KB, application/octet-stream)
2008-07-10 20:26 EDT, Matthew Hall CLA
no flags Details
Retrofit existing API to DecoratingObservables (97.88 KB, patch)
2008-07-10 21:19 EDT, Matthew Hall CLA
no flags Details | Diff
mylyn/context/zip (147.78 KB, application/octet-stream)
2008-07-10 21:19 EDT, Matthew Hall CLA
no flags Details
Refinements to implementation and tests (108.72 KB, patch)
2008-07-24 02:21 EDT, Matthew Hall CLA
no flags Details | Diff
mylyn/context/zip (43.83 KB, application/octet-stream)
2008-07-24 02:21 EDT, Matthew Hall CLA
no flags Details
Updated patch (84.59 KB, patch)
2008-09-04 17:51 EDT, Matthew Hall CLA
no flags Details | Diff
mylyn/context/zip (42.80 KB, application/octet-stream)
2008-09-04 17:51 EDT, Matthew Hall CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Hall CLA 2008-06-18 19:56:36 EDT
API Tools is showing errors for the IBeanObservable, ISWTObservable, and IViewerObservable interfaces and their concrete implementations due to @noextend / @noimplement restrictions.  We should introduce observable decorator classes as official API to address this.

Various internal classes such as UnmodifiableObservable* should be retrofitted to subclass these classes.  The ProxyObservable* classes could probably be eliminated altogether.
Comment 1 Boris Bokowski CLA 2008-06-20 12:38:53 EDT
+1
Comment 2 Matthew Hall CLA 2008-06-20 13:07:22 EDT
Note that this will only remove the errors for the concrete classes.  The IBeanObservable, ISWTObservable, and IViewerObservable interfaces will still show the error.
Comment 3 Matthew Hall CLA 2008-07-03 10:25:10 EDT
I've been thinking that we could get around the API Tools errors I mentioned in comment #2 by introducing an IObservable subinterface that is meant to be extended / implemented:

@noimplement interface IDecoratingObservable extends IObservable {
  public IObservable getDecorated();
}

thus:

interface ISWTObservable extends IDecoratingObservable {
  public Widget getWidget();
}

interface IBeanObservable extends IDecoratingObservable {
  public Object getBean();
  public PropertyDescriptor getPropertyDescriptor();
}

However there remains the problem of proliferating combinations of interfaces (e.g. ISWTObservable + IObservableValue = ISWTObservableValue), which would still trigger an API Tools error for each subinterface.  I think the solution is to proliferate once in the core plugin:

@noimplement @noextend interface IDecoratingObservable extends IObservable {
  public IObservable getDecorated();
}

@noimplement interface IDecoratingObservableValue
    extends IDecoratingObservable, IObservableValue {}

@noimplement interface IDecoratingObservableList
    extends IDecoratingObservable, IObservableList {}

@noimplement interface IDecoratingObservableSet
    extends IDecoratingObservable, IObservableSet {}

@noimplement interface IDecoratingObservableMap
    extends IDecoratingObservable, IObservableMap {}

By tagging the subinterfaces as @noimplement but leaving them open for extension, we encourage users to subclass our DecoratingObservable(Value|Set|List|Map) classes rather than implementing the decorating interfaces directly.
Comment 4 Boris Bokowski CLA 2008-07-03 10:35:46 EDT
Haven't thought this through but - could we just remove the @noextend clause from the observable interfaces, but leave the @noimplement?
Comment 5 Matthew Hall CLA 2008-07-03 10:44:37 EDT
(In reply to comment #4)
> Haven't thought this through but - could we just remove the @noextend clause
> from the observable interfaces, but leave the @noimplement?

+1
Comment 6 Matthew Hall CLA 2008-07-10 20:26:09 EDT
Created attachment 107154 [details]
Prototype

Needs tests
Comment 7 Matthew Hall CLA 2008-07-10 20:26:12 EDT
Created attachment 107155 [details]
mylyn/context/zip
Comment 8 Matthew Hall CLA 2008-07-10 21:19:12 EDT
Created attachment 107156 [details]
Retrofit existing API to DecoratingObservables
Comment 9 Matthew Hall CLA 2008-07-10 21:19:17 EDT
Created attachment 107157 [details]
mylyn/context/zip
Comment 10 Matthew Hall CLA 2008-07-14 11:10:41 EDT
(In reply to comment #4)
> Haven't thought this through but - could we just remove the @noextend clause
> from the observable interfaces, but leave the @noimplement?

I've removed the @noextend tags but am still getting errors from API tools:

Description	Resource	Path	Location	Type
ISWTObservable illegally implements IObservable	ISWTObservable.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/swt	line 22	API Usage Problem
ISWTObservableValue illegally implements IObservableValue	ISWTObservableValue.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/swt	line 21	API Usage Problem
IViewerObservable illegally implements IObservable	IViewerObservable.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/viewers	line 22	API Usage Problem
IViewerObservableList illegally implements IObservableList	IViewerObservableList.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/viewers	line 21	API Usage Problem
IViewerObservableSet illegally implements IObservableSet	IViewerObservableSet.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/viewers	line 21	API Usage Problem
IViewerObservableValue illegally implements IObservableValue	IViewerObservableValue.java	org.eclipse.jface.databinding/src/org/eclipse/jface/databinding/viewers	line 21	API Usage Problem

It appears that @noimplement is also applied when an interface extends an interface.  This surprised me because I thought this is what @noextend would be for.  Is this a bug in API tooling?
Comment 11 Matthew Hall CLA 2008-07-14 12:00:21 EDT
There is dispute in API tools on whether clients should be allowed to extends a @noimplement interface, even if the implementing class extends a legal implementation of that superinterface.
Comment 12 Matthew Hall CLA 2008-07-24 02:21:25 EDT
Created attachment 108307 [details]
Refinements to implementation and tests

Includes some new, previously unmatched methods in Observables (proxyObservableMap, unmodifiableObservableMap, proxyObservableValue)
Comment 13 Matthew Hall CLA 2008-07-24 02:21:29 EDT
Created attachment 108308 [details]
mylyn/context/zip
Comment 14 Matthew Hall CLA 2008-07-24 02:22:41 EDT
Boris, I think we're close enough to pull the trigger on this one.  All tests pass on my machine.  I'd like a vote from you before officially introducing the new API though.
Comment 15 Matthew Hall CLA 2008-09-01 17:58:55 EDT
Planned for 3.5M2
Comment 16 Matthew Hall CLA 2008-09-04 17:51:47 EDT
Created attachment 111732 [details]
Updated patch

Added a boolean constructor argument to all DecoratingObservable classes that determines whether the decorator, when disposed, will dispose the decorated observable as well.  The UnmodifiableObservable* classes pass false to the constructor, the assumption being that these classes are going to be used to make unmodifiable copies that can be passed around to clients, so the client should not be able to dispose the original observable.

I've also omitted the BeanObservable*Decorator classes from this updated patch because there are conflicting changes in bug 194734
Comment 17 Matthew Hall CLA 2008-09-04 17:51:50 EDT
Created attachment 111733 [details]
mylyn/context/zip
Comment 18 Matthew Hall CLA 2008-09-05 14:13:30 EDT
Patch released to HEAD > 20080905
Comment 19 Matthew Hall CLA 2008-09-05 14:22:33 EDT
Some additional javadoc changes / corrections released to HEAD > 20080905
Comment 20 Matthew Hall CLA 2008-12-10 00:51:20 EST
Verified in I20081209-0100 by code inspection and by running test suite