6465404: some problems in CellEditor related API docs #6608

TejeshR13 · 2021-11-30T08:49:19Z

JListBox is invalid component. CellTableEditor Interface doesn't support JComboBox and JTree components. Hence its removed from Documentation.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-6465404: some problems in CellEditor related API docs
JDK-8279421: some problems in CellEditor related API docs (CSR)

Reviewers

Prasanta Sadhukhan (@prsadhuk - Reviewer) ⚠️ Review applies to fb13cfd
Alexey Ivanov (@aivanov-jdk - Reviewer)
Alexander Zuev (@azuev-java - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6608

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

Using diff file

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

TejeshR13 · 2021-11-30T08:49:49Z

/covered

bridgekeeper · 2021-11-30T08:49:54Z

Hi @TejeshR13, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user TejeshR13" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

bridgekeeper · 2021-11-30T08:49:58Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

openjdk · 2021-11-30T08:52:02Z

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

mlbridge · 2021-12-06T15:58:27Z

Webrevs

openjdk · 2021-12-06T16:12:43Z

⚠️ @TejeshR13 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout branch_6465404
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-12-06T16:12:47Z

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

6465404: some problems in CellEditor related API docs

Reviewed-by: psadhukhan, aivanov, kizune, serb, prr

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 736 new commits pushed to the master branch:

b0496b0: 8279970: two AppCDS tests fail after JDK-8261455
4eb4f94: 8279956: Useless method Scheduling::ComputeLocalLatenciesForward()
4f4da3b: 8275318: loaded_classes_do may see ArrayKlass before InstanceKlass is loaded
3a421e4: 8280122: SupportedGroupsExtension should output "named groups" rather than "versions"
1a20628: 8248404: AArch64: Remove uses of long and unsigned long
46fd683: 8176567: nsk/jdi/ReferenceType/instances/instances002: TestFailure: Unexpected size of referenceType.instances(nsk.share.jdi.TestInterfaceImplementer1): 11, expected: 10
e314a4c: 8280124: Reduce branches decoding latin-1 chars from UTF-8 encoded bytes
bdfa15d: 8250801: Add clhsdb "threadcontext" command
fd9fb9a: 8279194: Add Annotated Memory Viewer feature to SA's HSDB
848b16a: 8272746: ZipFile can't open big file (NegativeArraySizeException)
... and 726 more: https://git.openjdk.java.net/jdk/compare/bd92674be563ad291990216b7cdf061c498f5dd3...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prsadhuk, @aivanov-jdk, @azuev-java, @mrserb, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

aivanov-jdk

Please also update the copyright year.

aivanov-jdk · 2021-12-06T16:29:03Z

src/java.desktop/share/classes/javax/swing/table/TableCellEditor.java

+ * an editor of values for the component <code>JTable</code> that
 * needs to implement.


Suggested change

* an editor of values for the component <code>JTable</code> that

* needs to implement.

* an editor of values for the {@code JTable} component needs to implement.

‘that’ after JTable shouldn't be there.

However, I would suggest rephrasing the Javadoc to make it easier to understand: the sentence has two levels of dependent clauses.

I'm not sure this fix is going in the right direction. Isn't the doc saying that if you want to use one of these components as the cell of a JTable it should implement this interface ?
And the problem with JListBox is that it is actually just called JList ?

As I read it, the interface TableCellEditor adds the getTableCellEditorComponent method to the generic CellEditor interface.

There's also TreeCellEditor interface which adds getTreeCellEditorComponent specifically for JTree. This one has a cleaner Javadoc:

Adds to CellEditor the extensions necessary to configure an editor in a tree.

The TableCellEditor interface should say something similar.

Currently, the Javadoc says, “…that would like to be an editor of values for components such as JListBox, JComboBox, JTree, or JTable…” Yet the interface adds a method which is relevant for JTable only. JList has no cell editor. JComboBox uses its own ComboBoxEditor interface, and JTree uses TreeCellEditor.

So, I think we're in the right direction.

Yeah, update the PR.

After going on through the example, what I felt is "Adds to CellEditor the extensions necessary to configure an editor in a tree" is little generic description than the updated PR Definition.

The interface is generic, so the generic description fits well.

I still find the proposed version quite hard to parse. In my first comment, I suggested rephrasing the Javadoc to make it clearer and easier to understand.

No, they don't. At the same time TableCellEditor interface can't be used to return a cell editor for JComboBox and for JTree components. These components use ComboBoxEditor and TreeCellEditor correspondingly.

Right the TableCellEditor should not be used as a JComboBox/JTree editor, it is only for JTable. But the objects implemented TableCellEditor interface can be used to modify the values stored as the JComboBox/JTree when the user edits the cell of the table. So the user selects the cell -> the TableCellEditor is activated for editing that cell -> if the data for the cell may be stored as a list then the JComboBox could be shown -> the user may change/edit the current value of the cell using JComboBox and its values? And the list of components in the spec is just an example of what components can be used by the TableCellEditor.

At least this is how I read it, might be wrong.

I agree it probably referred to components which can be used as an editor. DefaultEditorCell supports three components: JCheckBox, JComboBox, and JTextField.

But the objects implemented TableCellEditor interface can be used to modify the values stored as the JComboBox/JTree when the user edits the cell of the table.

And this just adds to the confusion rather than clarifies the purpose of the interface.

In fact, the CellEditor interface describes how the interface can be used for JTable and JTree and gives JTextField, JCheckBox, JComboBox as the examples of specific editors.

Hi, I have updated the PR as per latest discussions and Input, any other suggestions or Inputs......?

victordyakov · 2021-12-06T16:29:21Z

@azuev-java please review

aivanov-jdk

Please also expand the wildcard import at line 30:

import javax.swing.*;

to a specific class import. In fact, this interface imports only JTable with the wildcard since CellEditor is already imported explicitly.

aivanov-jdk · 2021-12-06T16:43:38Z

/reviewers 2

openjdk · 2021-12-06T16:44:16Z

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

aivanov-jdk

Please also update the copyright year to 2021.

TejeshR13 · 2021-12-09T05:24:42Z

@aivanov-jdk , updated the PR.

aivanov-jdk

Looks good to me.

The only question left: Do we keep the Javadoc as it is, or do we update it to be similar to TreeCellEditor?

src/java.desktop/share/classes/javax/swing/table/TableCellEditor.java

TejeshR13 · 2021-12-17T10:52:09Z

/CSR

openjdk · 2021-12-17T10:52:47Z

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

prrace · 2021-12-17T22:19:46Z

src/java.desktop/share/classes/javax/swing/table/TableCellEditor.java

- * an editor of values for components such as <code>JListBox</code>,
- * <code>JComboBox</code>, <code>JTree</code>, or <code>JTable</code>
- * needs to implement.
+ * an editor of values for the {@code JTable} component needs to implement.


"This interface must be implemented to provide an editor of cell values for a {@code JTable}" ?

Clear and to the point.

I wonder if it makes sense to update the Javadoc for TreeCellEditor for consistency.

Yeah, this description is clear and to the point. Will updated the PR.

aivanov-jdk · 2021-12-21T21:26:53Z

Should we add @see javax.swing.JTable to the Javadoc?

prrace · 2022-01-03T19:27:34Z

Should we add @see javax.swing.JTable to the Javadoc?

I don't think that is necessary. We are in JTable documentation already.

prrace · 2022-01-03T19:28:54Z

I don't see the CSR yet. Create it from the "More" menu of the JBS issue.

TejeshR13 · 2022-01-04T06:29:19Z

I don't see the CSR yet. Create it from the "More" menu of the JBS issue.

Yes, CSR is created from JBS..

openjdk · 2022-01-04T06:29:24Z

@TejeshR13 usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

TejeshR13 · 2022-01-04T06:37:41Z

Please Review the csr https://bugs.openjdk.java.net/browse/JDK-8279421

TejeshR13 · 2022-01-19T05:30:04Z

/integrate

openjdk · 2022-01-19T05:31:28Z

@TejeshR13
Your change (at version 9face9e) is now ready to be sponsored by a Committer.

prsadhuk · 2022-01-19T05:32:09Z

/sponsor

openjdk · 2022-01-19T05:33:33Z

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

b0496b0: 8279970: two AppCDS tests fail after JDK-8261455
4eb4f94: 8279956: Useless method Scheduling::ComputeLocalLatenciesForward()
4f4da3b: 8275318: loaded_classes_do may see ArrayKlass before InstanceKlass is loaded
3a421e4: 8280122: SupportedGroupsExtension should output "named groups" rather than "versions"
1a20628: 8248404: AArch64: Remove uses of long and unsigned long
46fd683: 8176567: nsk/jdi/ReferenceType/instances/instances002: TestFailure: Unexpected size of referenceType.instances(nsk.share.jdi.TestInterfaceImplementer1): 11, expected: 10
e314a4c: 8280124: Reduce branches decoding latin-1 chars from UTF-8 encoded bytes
bdfa15d: 8250801: Add clhsdb "threadcontext" command
fd9fb9a: 8279194: Add Annotated Memory Viewer feature to SA's HSDB
848b16a: 8272746: ZipFile can't open big file (NegativeArraySizeException)
... and 726 more: https://git.openjdk.java.net/jdk/compare/bd92674be563ad291990216b7cdf061c498f5dd3...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-19T05:33:53Z

@prsadhuk @TejeshR13 Pushed as commit 44fe958.

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

6465404: some problems in CellEditor related API docs

fb13cfd

bridgekeeper bot added the oca label Nov 30, 2021

bridgekeeper bot added the oca-verify label Nov 30, 2021

openjdk bot added the client label Nov 30, 2021

bridgekeeper bot removed oca oca-verify labels Dec 6, 2021

openjdk bot added the rfr label Dec 6, 2021

prsadhuk approved these changes Dec 6, 2021

View reviewed changes

openjdk bot added the ready label Dec 6, 2021

aivanov-jdk suggested changes Dec 6, 2021

View reviewed changes

openjdk bot removed the ready label Dec 6, 2021

TejeshR13 added 2 commits December 8, 2021 11:54

6465404: some problems in CellEditor related API docs

5e3d9ea

6465404: some problems in CellEditor related API docs

0e4e8bf

aivanov-jdk suggested changes Dec 8, 2021

View reviewed changes

6465404: some problems in CellEditor related API docs

84551ed

aivanov-jdk reviewed Dec 9, 2021

View reviewed changes

aivanov-jdk suggested changes Dec 9, 2021

View reviewed changes

src/java.desktop/share/classes/javax/swing/table/TableCellEditor.java Outdated Show resolved Hide resolved

6465404: some problems in CellEditor related API docs

d411b90

openjdk bot added the csr label Dec 17, 2021

prrace reviewed Dec 17, 2021

View reviewed changes

6465404: some problems in CellEditor related API docs

9face9e

aivanov-jdk approved these changes Jan 7, 2022

View reviewed changes

azuev-java approved these changes Jan 9, 2022

View reviewed changes

mrserb approved these changes Jan 11, 2022

View reviewed changes

openjdk bot added ready and removed csr labels Jan 14, 2022

prrace approved these changes Jan 18, 2022

View reviewed changes

openjdk bot added the sponsor label Jan 19, 2022

openjdk bot added the integrated label Jan 19, 2022

openjdk bot closed this Jan 19, 2022

openjdk bot removed ready rfr sponsor labels Jan 19, 2022

		* an editor of values for the component <code>JTable</code> that
		* needs to implement.

	* an editor of values for the component <code>JTable</code> that
	* needs to implement.
	* an editor of values for the {@code JTable} component needs to implement.

6465404: some problems in CellEditor related API docs #6608

6465404: some problems in CellEditor related API docs #6608

Conversation

TejeshR13 commented Nov 30, 2021 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

TejeshR13 commented Nov 30, 2021

bridgekeeper bot commented Nov 30, 2021

bridgekeeper bot commented Nov 30, 2021

openjdk bot commented Nov 30, 2021

mlbridge bot commented Dec 6, 2021 • edited Loading

Webrevs

openjdk bot commented Dec 6, 2021

openjdk bot commented Dec 6, 2021 • edited Loading

aivanov-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victordyakov commented Dec 6, 2021 • edited Loading

aivanov-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

aivanov-jdk left a comment

Choose a reason for hiding this comment

TejeshR13 commented Dec 9, 2021

aivanov-jdk left a comment

Choose a reason for hiding this comment

TejeshR13 commented Dec 17, 2021

openjdk bot commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk commented Dec 21, 2021

prrace commented Jan 3, 2022

prrace commented Jan 3, 2022

TejeshR13 commented Jan 4, 2022

openjdk bot commented Jan 4, 2022

TejeshR13 commented Jan 4, 2022

TejeshR13 commented Jan 19, 2022

openjdk bot commented Jan 19, 2022

prsadhuk commented Jan 19, 2022

openjdk bot commented Jan 19, 2022

openjdk bot commented Jan 19, 2022

TejeshR13 commented Nov 30, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 6, 2021 •

edited

Loading

openjdk bot commented Dec 6, 2021 •

edited

Loading

aivanov-jdk Dec 6, 2021 •

edited

Loading

victordyakov commented Dec 6, 2021 •

edited

Loading