-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add object factories to ISelection interface #1797
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see much added-value here, and while I understand this is an intent to make things simpler, I'm afraid it can introduce some confusion about how to handle null
or array objects or whether other types are permitted, while the current API makes it very explicit what to expect.
This also gives the impression to end-users that the IStructuredSelection is the only entry point to build a structured selection, while looking at the class hierarchy shows multiple implementations existing (eg TreeSelection), and encourages anyone who needs it to build a new implementation.
So it cannot be a 1-1 replacement and it's usually not more efficient to use it over the existing constructs which give more transparency about what is going to happen and what is possible.
bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/IStructuredSelection.java
Outdated
Show resolved
Hide resolved
Why should it be confusing? We can improve the javadoc if there is any uncertainty.
I don't think a user is encouraged to build an own one and effectively
It is a 1:1 replacement for the 90% case (2156 references to StructuredSelection versus 229 to TreeSelection versus 0 other implementations), still users are not limited to anything. |
Referencing concrete implementation is not always a bad thing. Or are you aware of a particular case where it is an issue here? |
The benefit is that I (as a user) compared to using the (only) implementation
|
974c3d1
to
b666dec
Compare
This also makes some trivial performance / memory optimizations possible, e.g. for empty arrays/collections this always return StructuredSelection.EMPTY instead of creating a new object + a copy of the array. |
b666dec
to
39b6049
Compare
39b6049
to
56df319
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reason mentioned above, more importantly the one that tells that ISelection is a publicly extensible type and not just a facade and that IMO factories on interfaces don't fit in that case as those lead users to false impressions of a single constrained entry point, I cannot give a positive review here.
I suggest we ask project leads to take a decision here.
@eclipse-platform/eclipse-platform-project-leads can you please take actions? |
Currently creating a IStructuredSelection requires to reference a concrete implementing class, also handling null is explicitly required and only List or Array are supported. This adds some new object factory methods similar to what users are used to with the Optional.of(...) / List.of(...) and similar and adds some usages of the new methods.
56df319
to
e036689
Compare
Currently creating a IStructuredSelection requires to reference a concrete implementing class, also handling null is explicitly required and only List or Array are supported.
This adds some new object factory methods similar to what users are used to with the Optional.of(...) / List.of(...) and similar and adds some usages of the new methods.