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

8270314: TreeTableCell: inconsistent naming for tableRow and tableColumn property methods #575

Closed

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jul 13, 2021

In the TreeTableCell class there is a mismatch between name of the following property method vs the getter:

    public final ReadOnlyObjectProperty<TreeTableRow<S>> tableRowProperty()
    public final TreeTableRow<S> getTreeTableRow()

The get method has "Tree" in the name while the property method does not.

By contrast, the corresponding methods for column are self-consistent, and are named without "Tree" in the name:

    public final ReadOnlyObjectProperty<TreeTableColumn<S,​T>> tableColumnProperty()
    public final TreeTableColumn<S,​T> getTableColumn()

The solution is to effectively change getTreeTableRow() to getTableRow(). In order to preserve source and binary compatibility, the method is copied rather than renamed. The existing getTreeTableRow() method is deprecated (not for removal).

Additionally, the docs for each property is on a private property field that does have tree in the name, which results in no docs being generated for either the tableRow or tableColumn property.

Finally, there is a problem with the implementation of the tableRowProperty() method in that it returns the writable property by mistake rather than the read-only property that is specified by the method's return type.

In summary, the following changes are made to TreeTableCell:

  1. Deprecate the getTreeTableRow method.
  2. Add a getTableRow method.
  3. Rename the (private) property object fields from treeTableRow and treeTableColumn to tableRow and tableColumn, including the name of the bean, so that they match the public property method names. This will allow API docs to be generated.
  4. Change the implementation of the tableRowProperty() method to return a read-only property.

In addition to changing the existing implementation and tests to call the new getTableRow method instead of the now-deprecated getTreeTableRow method, I added unit tests to validate changes 3 and 4.

NOTE: this is targeted to jfx17.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270314: TreeTableCell: inconsistent naming for tableRow and tableColumn property methods

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/575/head:pull/575
$ git checkout pull/575

Update a local copy of the PR:
$ git checkout pull/575
$ git pull https://git.openjdk.java.net/jfx pull/575/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 575

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/575.diff

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jul 13, 2021

This will need a CSR.

/csr
/reviewers 2

@kevinrushforth kevinrushforth changed the base branch from master to jfx17 Jul 13, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 13, 2021

👋 Welcome back kcr! 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 csr label Jul 13, 2021
@openjdk
Copy link

openjdk bot commented Jul 13, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@kevinrushforth please create a CSR request for issue JDK-8270314. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

openjdk bot commented Jul 13, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth marked this pull request as ready for review Jul 14, 2021
@kevinrushforth kevinrushforth requested a review from aghaisas Jul 14, 2021
@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jul 14, 2021

@kleopatra would you be willing to be a second reviewer?

@openjdk openjdk bot added the rfr label Jul 14, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 14, 2021

Webrevs

@kleopatra
Copy link
Collaborator

kleopatra commented Jul 16, 2021

@kleopatra would you be willing to be a second reviewer?

yeah sure - will do at the weekend.

A quick question: why didn't you rename the updateTreeTableRow/Column as well? Pretending that the properties were writable and we had setters, these would follow the new names - regarding those update methods as expert setters, I would tend to rename them also.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jul 16, 2021

A quick question: why didn't you rename the updateTreeTableRow/Column as well? Pretending that the properties were writable and we had setters, these would follow the new names - regarding those update methods as expert setters, I would tend to rename them also.

I didn't look at it too closely, but my thinking was to minimize the changes to those that were necessary. You are right, though, that they should follow the same naming convention since they are effectively expert-mode setters for those properties. I'll take a closer look at those, but it does seem consistent to rename them as well.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jul 16, 2021

As suggested, I also renamed updateTreeTable{Row,Column} to updateTable{Row,Column} (by deprecating the existing methods and adding new methods).

I'll update the CSR to match after those new changes have been reviewed.

* Note: This function is intended to be used by experts, primarily
* by those implementing new Skins. It is not common
* for developers or designers to access this function directly.
* @param tv the TreeTableView associated with this TreeTableCell
* @param tv the {@code TreeTableView} associated with this {@code TreeTableCell}
*/
public final void updateTreeTableView(TreeTableView<S> tv) {
Copy link
Member Author

@kevinrushforth kevinrushforth Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the docs for this method were just formatting changes to match the corresponding changes to row and column methods (I did the same thing earlier for the property methods).

Copy link
Collaborator

@kleopatra kleopatra left a comment

Looks good: verified failing/passing tests and that the deprecated methods are no longer used internally.

left a single comment inline

@openjdk openjdk bot removed the csr label Jul 20, 2021
@openjdk
Copy link

openjdk bot commented Jul 20, 2021

@kevinrushforth 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:

8270314: TreeTableCell: inconsistent naming for tableRow and tableColumn property methods

Reviewed-by: aghaisas, fastegal

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 15 new commits pushed to the jfx17 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 jfx17 branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jul 20, 2021
@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jul 20, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 20, 2021

Going to push as commit 748f464.
Since your change was applied there have been 15 commits pushed to the jfx17 branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 20, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 20, 2021
@openjdk
Copy link

openjdk bot commented Jul 20, 2021

@kevinrushforth Pushed as commit 748f464.

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

@kevinrushforth kevinrushforth deleted the 8270314-TreeTableCell branch Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
3 participants