Skip to content

8313403: Remove unused 'mask' field from JFormattedTextField #15088

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

Closed

Conversation

aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jul 31, 2023

The private field mask is never used in JFormattedTextField.

I couldn't find any usages of the field, including JNI. Therefore, it is safe to remove.


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

Issue

  • JDK-8313403: Remove unused 'mask' field from JFormattedTextField (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15088/head:pull/15088
$ git checkout pull/15088

Update a local copy of the PR:
$ git checkout pull/15088
$ git pull https://git.openjdk.org/jdk.git pull/15088/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15088

View PR using the GUI difftool:
$ git pr show -t 15088

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15088.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 31, 2023

👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 31, 2023
@openjdk
Copy link

openjdk bot commented Jul 31, 2023

@aivanov-jdk The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Jul 31, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 31, 2023

Webrevs

@mrserb
Copy link
Member

mrserb commented Aug 1, 2023

It was left there since the JFormattedTextField class is Serializable, and it is a kind of "public" API because it is mentioned in the serializable form of this class(I think so). See an example where I had to add the spec to the private fields via CSR: https://bugs.openjdk.org/browse/JDK-8252544

But the JFormattedTextField is part of the Swing where we intentionally added a statement "Serialized objects of this class will not be compatible with future Swing releases" So we can delete it but probably CSR will be needed, I am not sure.

@aivanov-jdk
Copy link
Member Author

It was left there since the JFormattedTextField class is Serializable, and it is a kind of "public" API because it is mentioned in the serializable form of this class(I think so). See an example where I had to add the spec to the private fields via CSR: https://bugs.openjdk.org/browse/JDK-8252544

But the JFormattedTextField is part of the Swing where we intentionally added a statement "Serialized objects of this class will not be compatible with future Swing releases" So we can delete it but probably CSR will be needed, I am not sure.

None of the Swing components is present in the Serialized Form for Java 17.

However, the field is still part of a serialized object. If a CSR is required, it may not be worth the effort.

@mrserb
Copy link
Member

mrserb commented Aug 1, 2023

It is better to check, if CSR is not necessary anymore at least for Swing, we can do some small cleanup here and there.

@openjdk
Copy link

openjdk bot commented Aug 17, 2023

@aivanov-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8313403: Remove unused 'mask' field from JFormattedTextField

Reviewed-by: prr, honkar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 252 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 17, 2023
@aivanov-jdk
Copy link
Member Author

aivanov-jdk commented Aug 18, 2023

@prrace could you confirm, please? No CSR is required to remove an unused private field from classes in Swing because the specification for Swing classes states, “Serialized objects of this class will not be compatible with future Swing releases.”

I believe your approval means just that. I still want an additional confirmation to avoid any confusion.

It is to resolve @mrserb's concern raised above.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 19, 2023

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

@aivanov-jdk
Copy link
Member Author

@prrace could you confirm, please? No CSR is required to remove an unused private field from classes in Swing because the specification for Swing classes states, “Serialized objects of this class will not be compatible with future Swing releases.”

I believe your approval means just that. I still want an additional confirmation to avoid any confusion.

It is to resolve @mrserb's concern raised above.

I haven't gotten any reply…

The PR has two approvals now. If don't hear any objections, I'll integrate it in a day or two.

@prrace
Copy link
Contributor

prrace commented Sep 20, 2023

@prrace could you confirm, please? No CSR is required to remove an unused private field from classes in Swing because the specification for Swing classes states, “Serialized objects of this class will not be compatible with future Swing releases.”
I believe your approval means just that. I still want an additional confirmation to avoid any confusion.
It is to resolve @mrserb's concern raised above.

I haven't gotten any reply…

The PR has two approvals now. If don't hear any objections, I'll integrate it in a day or two.

Swing isn't serializable across releases, so I see no need for a CSR.

@aivanov-jdk
Copy link
Member Author

Swing isn't serializable across releases, so I see no need for a CSR.

Thank you. That's what I thought.

@aivanov-jdk
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 25, 2023

Going to push as commit b65f4f7.
Since your change was applied there have been 698 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 25, 2023
@openjdk openjdk bot closed this Sep 25, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 25, 2023
@openjdk
Copy link

openjdk bot commented Sep 25, 2023

@aivanov-jdk Pushed as commit b65f4f7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aivanov-jdk aivanov-jdk deleted the 8313403-formattedField-mask branch October 2, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants