-
Notifications
You must be signed in to change notification settings - Fork 473
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
Remove final from TransformationList (Filtered and Sorted) #278
Conversation
👋 Welcome back tobiasdiez! A progress list of the required criteria for merging this PR into |
internal review ID : 9066289 |
thanks :) |
In addition to needing a JBS issue, since this is an enhancement request, with an associated API change, a PR is not the right starting point. It will need prior discussion on the openjfx-dev mailing list. See the Before submitting a pull request section of the CONTRIBUTING guidelines, specifically the part that says: If this is a feature request, please note the additional requirements and expectations in the New features / API additions section of the Code Review Policies doc. Many of the JavaFX classes are final by design, and in any case, it isn't as simple as "just remove the final keyword". You will need to provide motivation as to why this is needed. Our general position would be to not accept such a proposed change, but go ahead and start the discussion. |
Thanks for the feedback. I wasn't sure if this really falls as a "feature" or "API" change. I just send a message to the dev mailing list. |
@kevinrushforth I think my email is stuck in the moderation queue. Can you please have a look? |
@kevinrushforth Is there any update on this? My mail to the dev list only got one response (https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-August/027395.html), which was rather general. I agree that porting some of the functionality of EasyBind back to JavaFX would be desirable in the long term, but that's a rather huge project. |
@tobiasdiez This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@kevinrushforth @kleopatra Any feedback on how to proceed/what is missing is highly appreciated. |
There seems to be insufficient motivation to do this. |
@tobiasdiez This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
It's a shame that simple changes like this one here, that would simplify implementations in downstream libraries, are not appreciated. |
I would also like to see this PR merged. Perhaps it should be mandated that if a class such as this is marked as final, then it be accompanied with a reason why. Otherwise, you get "justifications" like "well, I'm sure it was marked final for a reason...", 15 years later. Nothing bugs me more than a "well, that's the way it's always been..." reason for anything. |
Right now it is hard to extend the FilteredList and the SortedList as these classes are marked final. This makes it practically impossible to add convenient helper methods, or change the behavior of these classes. I agree that normally you don't need to extend them, but I also don't see any reason why you shouldn't be allowed to do so.
Progress
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/278/head:pull/278
$ git checkout pull/278
Update a local copy of the PR:
$ git checkout pull/278
$ git pull https://git.openjdk.org/jfx pull/278/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 278
View PR using the GUI difftool:
$ git pr show -t 278
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/278.diff