Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

WIP: JDK-8091967: Add a MappedList Implementation #499

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

madoar
Copy link

@madoar madoar commented Jun 12, 2019

This PR adds a MappedList class (and corresponding tests) to OpenJFX.

The MappedList class takes a source ObservableList and a mapper Function as its arguments and applies the mapper function on each object inside the source list. The result is then returned as the content of the MappedList.

In case no mapper function is applied (i.e. mapper = null), the MappedList acts as an empty list with a size of zero.

Corresponding GitHub issue: #303
Corresponding JBS issue: https://bugs.openjdk.java.net/browse/JDK-8091967

The code is based on my implementation done in: https://github.com/PhoenicisOrg/javafx-collections

I'm not sure about the code standards for this project therefore I've just done some slight changes to make the classes (MappedList and MappedListTest) fit better together with the other TransformationList implementations. Therefore if you have any requirements for the code feel free to tell me so that I can make the necessary changes, otherwise I hope that my implementation finds approval.

@kevinrushforth kevinrushforth added enhancement New feature or request WIP Work In Progress labels Jun 12, 2019
@kevinrushforth
Copy link
Collaborator

Please read the CONTRIBUTING guidelines, specifically the Before submitting a pull request section. Since this is a new feature that adds new API, it will need prior discussion on the mailing list as to whether it is suitable to bring this into the core of javafx.base.

Marking this as "WIP" since it is premature to review it.

@madoar
Copy link
Author

madoar commented Jun 13, 2019

@kevinrushforth first thank you for pointing me to the CONTRIBUTING guidelines.

I'm a bit unsure how I should proceed now that you've marked the PR as WIP. I understand that I need to sign the OCA, but apart from that I don't know how to proceed. You mentioned that the new API needs prior discussion on the mailing list. When "triggering" this discussion is it sufficient to write the JBS-ID and the description text provided in this PR? Is this apart from the OCA the only point left to do for now?

I must confess that compared to other projects the barrier to provide new features to openjfx is quite difficult and time consuming for the one providing those.

@kevinrushforth
Copy link
Collaborator

Your question really boils down to the more general "How do I get a new feature into JavaFX?". You commented that the barrier seems a bit higher than you were expecting. For new features, this is intentional. The general guidelines for new features are covered here, which was linked from the CONTRIBUTING guidelines mentioned above, so I assume you've already read it.

You asked what the next steps were, which suggests that the guidelines could be clearer. I took a stab at adding some more detail that should eventually be added to the CONTRIBUTING guidelines. Please see Note: New Features in JavaFX. That note should be taken as a general guideline on feature development, and not a comment on your specific proposal. I do have some concerns about the value proposition of this proposed feature, but the right place to discuss that is the mailing list.

To answer your other question, yes we also need a signed OCA from you before we can review this or any contribution.

@madoar
Copy link
Author

madoar commented Jun 13, 2019

Thank you for clarifying the process of adding new features to JavaFX. What is still missing in my opinion is how the JBS issues are related to the whole process. The idea why I even opened this PR is mainly because of the linked JBS issue. Let me elaborate:

I'm a huge fan of map and fold operations like Java supports with its streaming API, which was introduced in Java 8. Therefore somewhere around a year ago I searched for a way to apply map and fold operations to an ObservableList. Filtering and ordering an ObservableList was already possible, therefore I searched for some kind of feature request for this and found the above mentioned JBS issue.

Because the JBS issue was already quite old and didn't have a lot of feedback, I thought about implementing my own solution to problem based on the TransformationList implementations in JavaFX. Now, some time later, I thought about sharing my solution because I discovered the JBS issue continues to be open.

Now after reading your comments I'm asking myself what do JBS issues represent, and why not discuss the value of a requested feature inside the corresponding JBS issue? Previously I thought they indicate approved abstract feature requests and and confirmed bug reports, but according to your comments this doesn't seem to be the case.

@kevinrushforth
Copy link
Collaborator

I can see your point about the open JBS Enhancement request, so it seems that there might be still something missing in our README and/or CONTRIBUTING docs that could help explain this. I can't immediately think of how to improve it, but I'll think about it.

For bugs, it's pretty straight-forward, a JBS issue of type Bug represents something that doesn't work as it should, so when we triage a Bug, we will try to reproduce it and prioritize it, putting it in the backlog if it is a valid bug that we can reproduce, closing it (or marking it incomplete) if not.

It isn't as straight-forward for enhancements. Unless something is clearly in the "no this will never make any sense" category, we don't usually close an Enhancement, even if we are unlikely to work on it. By now we have over 1800 open Enhancement requests. About the only thing we can tell from each one of them is that at least one person thought the idea was useful at the time it was filed.

As for discussing it in the JBS issue itself, the main reason this won't work is that most developers do not have write access to JBS. I suppose that an alternative approach would be to use a linked GitHub issue (I note issue #303 is the one for this bug), and have the discussion there, but even in that case, the initial high-level proposal, the "I would like to discuss implementing feature XYZ", needs to happen on the mailing list (you can certainly reference the JBS issue and the GitHub issue in that email as supporting documentation).

@kevinrushforth kevinrushforth changed the title JDK-8091967: Add a MappedList Implementation WIP: JDK-8091967: Add a MappedList Implementation Oct 1, 2019
@kevinrushforth
Copy link
Collaborator

kevinrushforth commented Oct 1, 2019

As announced in this message, the official jfx repository is moving from hg.openjdk.java.net/openjfx/jfx-dev/rt to github.com/openjdk/jfx.

This sandbox repository is being retired on 1-Oct-2019. You will need to migrate your WIP pull request to the openjdk/jfx repo if you want to continue working on it. Here are instructions for migrating your pull request. The updated CONTRIBUTING.md doc has additional information on how to proceed.

Once you have done this, it would be helpful to add a comment with a pointer to the new PR.

NOTE: since this is a feature / enhancement request it needs to be discussed on the openjfx-dev mailing list if you want it to be considered.


The new openjdk/jfx repo will be open for pull requests on Wednesday, 2-Oct-2019. I will send email to the openjfx-dev mailing list announcing this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants