Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Aug 2, 2020

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

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

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

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 2, 2020

👋 Welcome back tobiasdiez! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@kleopatra
Copy link
Collaborator

#22 (comment)

@tobiasdiez
Copy link
Contributor Author

internal review ID : 9066289

@kleopatra
Copy link
Collaborator

thanks :)

@kevinrushforth
Copy link
Member

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.

@tobiasdiez
Copy link
Contributor Author

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.

@tobiasdiez
Copy link
Contributor Author

@kevinrushforth I think my email is stuck in the moderation queue. Can you please have a look?

@tobiasdiez
Copy link
Contributor Author

@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.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 31, 2023

@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!

@tobiasdiez
Copy link
Contributor Author

@kevinrushforth @kleopatra Any feedback on how to proceed/what is missing is highly appreciated.

@kevinrushforth
Copy link
Member

There seems to be insufficient motivation to do this.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 29, 2023

@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!

@tobiasdiez
Copy link
Contributor Author

It's a shame that simple changes like this one here, that would simplify implementations in downstream libraries, are not appreciated.

@tobiasdiez tobiasdiez closed this Apr 30, 2023
@tobiasdiez tobiasdiez deleted the patch-2 branch April 30, 2023 04:08
@credmond
Copy link

credmond commented Nov 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants