Bug 566751 - [Databinding 2.0] Collections inheritance is inconsistent
Summary: [Databinding 2.0] Collections inheritance is inconsistent
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.14   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 566750
  Show dependency tree
 
Reported: 2020-09-08 00:30 EDT by Christoph Laeubrich CLA
Modified: 2020-09-18 10:07 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Laeubrich CLA 2020-09-08 00:30:20 EDT
As discussed in the gerrit here [1] currently the collections inheritance of the databinding frameweork is inconsistent and includes a lots of code duplication.

 Current inconsitencies: 
 - AbstractObservableSet is read-only by default while AbstractObservableList is read/write
 - ObservableSet (what is an abstract class!) is read-only as-well and duplicates most of AbstractObservableSet instead of the constructor and the ElementType addition
 - ObservableList (what is an abstract class!) is in contrast read-only and duplicates most of AbstractObservableList instead of the constructor and the ElementType addition
 - AbstractObservableSet has a method getWrappedSet() and delegates to this, AbstractObservableList extends AbstractList and delegate to super-methods
 - ObservableSet has a setWrappedSet but does not fire a change event, ObservableList has a updateWrappedList and does fire change events
 - AbstractObservableList does currently not implement getSubList(int,int)
 - DecoratingObservableCollection/List/Set that mostly behaves like modifiable ObservableList/Set but are modifiable and delegate ti underlying collections duplicating much of the code again.
- then there is WritableList/Set that also duplicates code from DecoratingObservableList/Set again but extends ObservableList/Set and not the abstract ones...

Suggested new structure:
- there should be an AbstractCollection where AbstractObservableSet/List can inherit from most methods
 - AbstractObservableSet/List simply delegates all calls to getWrappedSet/List() regardless for read/write (if someone need the read-only behaviour he simply can return an unmodifiable Collection)
 - ObservableSet/List extends their abstract counterparts instead of duplicate all the code and do offer updateWrappedList/Set that does update and fire events, also both classes should become non abstract (they both don't declare any abstract methods...)


[1] https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/168855
Comment 1 Christoph Laeubrich CLA 2020-09-08 02:52:29 EDT
As disccused in Bug 566755 it is not really possible to do breaking changes and major versions has to remain the same forever, I'll thus propose to add a new package named 

> org.eclipse.core.databinding.observable.collections

that includes cleaned up versions of the collection implementations with new semantics and old classes can the be removed via the 2-yers removal process afterwards.
Comment 2 Jens Lideström CLA 2020-09-13 09:40:03 EDT
Hello Christoph,

It is nice that you want to improve the databinding framework. I agree that the design would be better if it were the way you describe. I too think it is frustrating to be forced to keep old, defect API:s for backwards compatibility.

But breaking backwards compatibility has a very high cost for library API:s like this. If we were to remove old classes even after 2 years it might break useful plugins that are no longer maintained but have been working fine for years.

My feeling is that even if your suggested changes would be useful improvements, they would not be useful enough to justify the cost.

In any case, that not a decision I can make myself, someone in a responsible position within the project would have to get involved.
Comment 3 Jens Lideström CLA 2020-09-13 09:41:58 EDT
Is there *anything* we could do about the code duplication between ObservableList, AbstractObservableList and DecoratingObservableList without breaking backwards compatibility?
Comment 4 Jens Lideström CLA 2020-09-18 10:07:58 EDT
An example of removed API elements that cased problems turned up on the platform-dev mailing list recently:

https://www.eclipse.org/lists/platform-dev/msg02397.html