JDK-6998249: Wrong behavior/Javadoc of JTable.tableChanged(TableModelEvent e) #7253
Conversation
|
👋 Welcome back honkar-jdk! A progress list of the required criteria for merging this PR into |
|
@honkar-jdk The following label will be automatically applied to this pull request:
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. |
| * the coordinate system of the model for the column and the | ||
| * coordinate system of the view for the row; the appropriate | ||
| * mapping to the view coordinate system is performed by this | ||
| * <code>JTable</code> when it receives the event. |
There was a problem hiding this comment.
A few things
- since most of the doc is changing we can update to use the more modern {@code ...} instead
- the 2nd clause is now only about the columns so should say "the appropriate mapping of the column to"
- there's no @param tag .. how come doclint isn't complaining ? But we should fix it.
- This will require a CSR.
Oh and I assume you can confirm that you verified the above is 100% true .. and there's no need to mention RowSorter - can you point to where the event is created ?
There was a problem hiding this comment.
I said most of the doc is changing which is clearly not most of the doc for JTable so it means just this method.
To the latter point the doc you are proposing is asserting that it does not apply to rows.
Now since we also know this is all supposed to be code called ny the implementation and responded to by the implementation and even so we are NOT going to change anything because some code somewhere would break, nontheless we want to be sure the new code is what actually happens and not just what someone wrote in a bug report several years ago. So go find the code that calls "new TableModelEvent" and point in this PR to how it guarantees the rows are in view order.
There was a problem hiding this comment.
@prrace After going through the code, I see that the new TableModelEvent() is called in setModel of JTable but I wasn't able to locate the code that does model-to-view coordinate mapping for rows from here. Within the tableChanged method I see sortedTableChanged method being called if sortManager != null which in turn calls convertRowIndexToView method (which is called when either sorter or model changes and sorting is enabled). I think this is the method that is responsible for mapping coordinate system from model to view for the rows.
There was a problem hiding this comment.
I was just looking at this same code too - and that's how it looks to me.
So the claim in the report AND the evaluation both look wrong to me.
But verify this with testing. I am not sure but it seems like merely doing whatever it is to install
a RowSorter will ttigger that code path and the code the submitter pointed to is only used if
the model row == the view row ..
|
/csr |
|
@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
That's worrying. So maybe it doesn't exist ? When I look at TableModelEvent docs it also says at the VERY BEGINNING of the class docs ..
So at the very least then these docs would be wrong too !?!? I think look at the code that does "new TableModelEvent(...") and it is in methods caller fireTableCellUpdated() which is defined on the TableModel. So it goes back to what I said to you earlier (somewhere) - don't just trust what is written in a bug report - verify it. So I think you should do some more looking at code and actual testing to verify. |
|
@prrace Thank you. Will look into the code and verify with actual testing as well. |
|
Unless your investigation uncovers new information, it seems like this PR will be withdrawn and the bug report closed as not an issue. What is the status of that investigation ? |
|
@prrace So far I have tested two conditions where the model to view mapping for row changes is not happening as stated. I'm checking one other scenario and will have an update (and include the details) on this issue by today. |
The current JavaDocs for JTable's tableChanged method sounds correct based on the scenarios tested below.
The JTable tableChanged method was assessed for two cases -
When table is created and a default row sorter is attached - In this case the tableChanged method is called and as the row coordinates of view are same as the model (there is no model-to-view mapping done).
When any of the columns are sorted - In this case tableChanged method is not called at all and the model-to-view mapping is carried out by using convertRowIndexToView called by the restoreSortingSelection() in JTable.
Additionally, the code that does row coordinate mapping from model-to-view is not triggered by tableChanged method.
Thus the PR is withdrawn without integrating any changes.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7253/head:pull/7253$ git checkout pull/7253Update a local copy of the PR:
$ git checkout pull/7253$ git pull https://git.openjdk.java.net/jdk pull/7253/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7253View PR using the GUI difftool:
$ git pr show -t 7253Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7253.diff