Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
On Sat, 24 Feb 2024 17:38:11 GMT, Marius Hanl wrote: >> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all >> other implementations of the cell. >> >> This also means that the `TreeTableRow` always updates the item, although it >> should not, resulting in a performance loss as a `TreeTableRow` will always >> call `updateItem(..)`. >> >> It looks like that this was forgotten in >> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). >> >> Checking the whole history, it looks like the following was happening: >> 1. There was a check if the item is the same in all cell implementations >> (with `.equals(..)`) >> 2. Check was removed as it caused bugs >> 3. Check was readded, but instead we first check the index (same index) and >> then if we also have the same item (this time with `.isItemChanged(..)`, to >> allow developers to subclass it) >> 4. Forgotten for `TreeTableRow` inbetween this chaos. >> >> Added many tests for the case where an item should be changed and where it >> should not. >> Improved existing tests as well. Using `stageLoader`, as before the tests >> only created a stage when doing the assert. >> >> NOTE: The update logic is now the same as for all other 5 cell >> implementations. Especially `TreeCell` (since it is for a tree structure as >> well). > > Marius Hanl has updated the pull request incrementally with two additional > commits since the last revision: > > - JDK-8325402: remove labeled if > - JDK-8325402: Unit test improvements LGTM, tested in Mac 14.3.1. - Marked as reviewed by kpk (Committer). PR Review: https://git.openjdk.org/jfx/pull/1360#pullrequestreview-1916024806
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
On Wed, 14 Feb 2024 20:53:46 GMT, Marius Hanl wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
>> line 421:
>>
>>> 419: if (oldIndex == newIndex) {
>>> 420: if (!isItemChanged(oldValue, newValue)) {
>>> 421: // RT-37054: we break out of the if/else code
>>> here and
>>
>> could we also add (or replace?) the new reference for RT-37054: JDK-8096969
>
> same as the other comment, I would prefer to do that for all cell
> implementations in one go. If that is okay for you as well. :)
okay
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1508074943
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
On Sat, 24 Feb 2024 17:38:11 GMT, Marius Hanl wrote: >> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all >> other implementations of the cell. >> >> This also means that the `TreeTableRow` always updates the item, although it >> should not, resulting in a performance loss as a `TreeTableRow` will always >> call `updateItem(..)`. >> >> It looks like that this was forgotten in >> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). >> >> Checking the whole history, it looks like the following was happening: >> 1. There was a check if the item is the same in all cell implementations >> (with `.equals(..)`) >> 2. Check was removed as it caused bugs >> 3. Check was readded, but instead we first check the index (same index) and >> then if we also have the same item (this time with `.isItemChanged(..)`, to >> allow developers to subclass it) >> 4. Forgotten for `TreeTableRow` inbetween this chaos. >> >> Added many tests for the case where an item should be changed and where it >> should not. >> Improved existing tests as well. Using `stageLoader`, as before the tests >> only created a stage when doing the assert. >> >> NOTE: The update logic is now the same as for all other 5 cell >> implementations. Especially `TreeCell` (since it is for a tree structure as >> well). > > Marius Hanl has updated the pull request incrementally with two additional > commits since the last revision: > > - JDK-8325402: remove labeled if > - JDK-8325402: Unit test improvements Tested in MT on macOS 14.3.1 Looks good, thank you for making the changes! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1360#pullrequestreview-1909680339
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all > other implementations of the cell. > > This also means that the `TreeTableRow` always updates the item, although it > should not, resulting in a performance loss as a `TreeTableRow` will always > call `updateItem(..)`. > > It looks like that this was forgotten in > [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). > > Checking the whole history, it looks like the following was happening: > 1. There was a check if the item is the same in all cell implementations > (with `.equals(..)`) > 2. Check was removed as it caused bugs > 3. Check was readded, but instead we first check the index (same index) and > then if we also have the same item (this time with `.isItemChanged(..)`, to > allow developers to subclass it) > 4. Forgotten for `TreeTableRow` inbetween this chaos. > > Added many tests for the case where an item should be changed and where it > should not. > Improved existing tests as well. Using `stageLoader`, as before the tests > only created a stage when doing the assert. > > NOTE: The update logic is now the same as for all other 5 cell > implementations. Especially `TreeCell` (since it is for a tree structure as > well). Marius Hanl has updated the pull request incrementally with two additional commits since the last revision: - JDK-8325402: remove labeled if - JDK-8325402: Unit test improvements - Changes: - all: https://git.openjdk.org/jfx/pull/1360/files - new: https://git.openjdk.org/jfx/pull/1360/files/947b4153..a7584d81 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1360=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1360=00-01 Stats: 29 lines in 10 files changed: 11 ins; 12 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1360.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1360/head:pull/1360 PR: https://git.openjdk.org/jfx/pull/1360
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Thu, 15 Feb 2024 12:08:27 GMT, Marius Hanl wrote: >> on second thought, this is ok as is - I asked because the code is not >> testing a specific requirement. >> The second `assertEquals(18)` might be testing a specific requirement - that >> the count has not changed, right? > > Yes, as far as I can see. so in theory, we could just test the fact that the count has not changed: int ct = rt_31200_count; ... assertEquals(ct, rt_31200_count); this will make the test independent of the first number (which might change) and only test what's important (that the count does not change later). what do you think? - PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1491304604
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 20:57:14 GMT, Andy Goryachev wrote: >> Yes! I did it this way to be consistent with all other cell implementations. >> I really think that this should be refactored, I can also create a ticket >> for this. My preference would be to be consistent now, but change it for all >> cell implementations in one go. >> What is your opinion? > > my opinion: the code is not broken per se. Since this block is edited, I > would have changed it just in this file to make the logic clearer (in other > places, like TreeTableCell:685 it is functional because we have more code in > line 692 and on). > I don't think another ticket is necessary because, technically speaking, the > code is not broken. > (just my opinion) yes, but other cells have the exact same pattern, although it is not needed. And technically speaking, this is not even needed in `TreeTableCell`, tbh I never even needed a labeled if ever. But sure, can also change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490919877
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 21:01:59 GMT, Andy Goryachev wrote: >> It is the count, how often update item was called. >> That it got smaller shows, that we call it less often, as the item was not >> changed (which is a good thing). >> That also matches with your observation, which is the expected output here. >> >> I saw that this was changed for some tests in >> https://github.com/openjdk/jfx/pull/863, we also can do something similar >> here as well. > > on second thought, this is ok as is - I asked because the code is not testing > a specific requirement. > The second `assertEquals(18)` might be testing a specific requirement - that > the count has not changed, right? Yes, as far as I can see. - PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490914400
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 20:52:44 GMT, Marius Hanl wrote: >> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java >> line 2639: >> >>> 2637: StageLoader sl = new StageLoader(treeTableView); >>> 2638: >>> 2639: assertEquals(18, rt_31200_count); >> >> magic number... should we explain why exactly 18 ? > > It is the count, how often update item was called. > That it got smaller shows, that we call it less often, as the item was not > changed (which is a good thing). > That also matches with your observation, which is the expected output here. > > I saw that this was changed for some tests in > https://github.com/openjdk/jfx/pull/863, we also can do something similar > here as well. on second thought, this is ok as is - I asked because the code is not testing a specific requirement. The second `assertEquals(18)` might be testing a specific requirement - that the count has not changed, right? - PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490064468
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 20:48:33 GMT, Marius Hanl wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java >> line 424: >> >>> 422: // proceed with the code following this, so that >>> we may >>> 423: // still update references, listeners, etc as >>> required. >>> 424: break outer; >> >> I understand neither the comment nor this `break outer;`: there is no code >> that follows `outer: if`, so we might as well `return`, right? > > Yes! I did it this way to be consistent with all other cell implementations. > I really think that this should be refactored, I can also create a ticket for > this. My preference would be to be consistent now, but change it for all cell > implementations in one go. > What is your opinion? my opinion: the code is not broken per se. Since this block is edited, I would have changed it just in this file to make the logic clearer (in other places, like TreeTableCell:685 it is functional because we have more code in line 692 and on). I don't think another ticket is necessary because, technically speaking, the code is not broken. (just my opinion) - PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490058786
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 19:21:39 GMT, Andy Goryachev wrote:
>> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all
>> other implementations of the cell.
>>
>> This also means that the `TreeTableRow` always updates the item, although it
>> should not, resulting in a performance loss as a `TreeTableRow` will always
>> call `updateItem(..)`.
>>
>> It looks like that this was forgotten in
>> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593).
>>
>> Checking the whole history, it looks like the following was happening:
>> 1. There was a check if the item is the same in all cell implementations
>> (with `.equals(..)`)
>> 2. Check was removed as it caused bugs
>> 3. Check was readded, but instead we first check the index (same index) and
>> then if we also have the same item (this time with `.isItemChanged(..)`, to
>> allow developers to subclass it)
>> 4. Forgotten for `TreeTableRow` inbetween this chaos.
>>
>> Added many tests for the case where an item should be changed and where it
>> should not.
>> Improved existing tests as well. Using `stageLoader`, as before the tests
>> only created a stage when doing the assert.
>>
>> NOTE: The update logic is now the same as for all other 5 cell
>> implementations. Especially `TreeCell` (since it is for a tree structure as
>> well).
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
> line 421:
>
>> 419: if (oldIndex == newIndex) {
>> 420: if (!isItemChanged(oldValue, newValue)) {
>> 421: // RT-37054: we break out of the if/else code here
>> and
>
> could we also add (or replace?) the new reference for RT-37054: JDK-8096969
same as the other comment, I would prefer to do that for all cell
implementations in one go. If that is okay for you as well. :)
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java
> line 1150:
>
>> 1148: super.updateItem(item, empty);
>> 1149:
>> 1150: counter.incrementAndGet();
>
> very, very minor, and probably not important: shouldn't 'incrementAndGet()`
> be called before `super.updateItem()`?
Hm, I see no reason for either one or the other. So not strong preference from
my side here.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
> line 2639:
>
>> 2637: StageLoader sl = new StageLoader(treeTableView);
>> 2638:
>> 2639: assertEquals(18, rt_31200_count);
>
> magic number... should we explain why exactly 18 ?
It is the count, how often update item was called.
That it got smaller shows, that we call it less often, as the item was not
changed (which is a good thing).
That also matches with your observation, which is the expected output here.
I saw that this was changed for some tests in
https://github.com/openjdk/jfx/pull/863, we also can do something similar here
as well.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490054700
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490056222
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490053457
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Wed, 14 Feb 2024 19:25:27 GMT, Andy Goryachev wrote:
>> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all
>> other implementations of the cell.
>>
>> This also means that the `TreeTableRow` always updates the item, although it
>> should not, resulting in a performance loss as a `TreeTableRow` will always
>> call `updateItem(..)`.
>>
>> It looks like that this was forgotten in
>> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593).
>>
>> Checking the whole history, it looks like the following was happening:
>> 1. There was a check if the item is the same in all cell implementations
>> (with `.equals(..)`)
>> 2. Check was removed as it caused bugs
>> 3. Check was readded, but instead we first check the index (same index) and
>> then if we also have the same item (this time with `.isItemChanged(..)`, to
>> allow developers to subclass it)
>> 4. Forgotten for `TreeTableRow` inbetween this chaos.
>>
>> Added many tests for the case where an item should be changed and where it
>> should not.
>> Improved existing tests as well. Using `stageLoader`, as before the tests
>> only created a stage when doing the assert.
>>
>> NOTE: The update logic is now the same as for all other 5 cell
>> implementations. Especially `TreeCell` (since it is for a tree structure as
>> well).
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
> line 424:
>
>> 422: // proceed with the code following this, so that we
>> may
>> 423: // still update references, listeners, etc as
>> required.
>> 424: break outer;
>
> I understand neither the comment nor this `break outer;`: there is no code
> that follows `outer: if`, so we might as well `return`, right?
Yes! I did it this way to be consistent with all other cell implementations. I
really think that this should be refactored, I can also create a ticket for
this. My preference would be to be consistent now, but change it for all cell
implementations in one go.
What is your opinion?
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
> line 1777:
>
>> 1775: }
>> 1776:
>> 1777: @Test public void testSetChildrenShouldUpdateTheCells() {
>
> we probably should have `@Test` on its own line
sure, just copied from above.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490048165
PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490049249
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
On Thu, 8 Feb 2024 18:55:54 GMT, Marius Hanl wrote:
> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all
> other implementations of the cell.
>
> This also means that the `TreeTableRow` always updates the item, although it
> should not, resulting in a performance loss as a `TreeTableRow` will always
> call `updateItem(..)`.
>
> It looks like that this was forgotten in
> [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593).
>
> Checking the whole history, it looks like the following was happening:
> 1. There was a check if the item is the same in all cell implementations
> (with `.equals(..)`)
> 2. Check was removed as it caused bugs
> 3. Check was readded, but instead we first check the index (same index) and
> then if we also have the same item (this time with `.isItemChanged(..)`, to
> allow developers to subclass it)
> 4. Forgotten for `TreeTableRow` inbetween this chaos.
>
> Added many tests for the case where an item should be changed and where it
> should not.
> Improved existing tests as well. Using `stageLoader`, as before the tests
> only created a stage when doing the assert.
>
> NOTE: The update logic is now the same as for all other 5 cell
> implementations. Especially `TreeCell` (since it is for a tree structure as
> well).
Tried the path with the TreeTableView page in the MonkeyTester on macOS 14.3,
see no ill effects - though the page is somewhat limited to what it can
exercise.
The SCCE generates the following output:
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
updateItem
isItemChanged
isItemChanged
isItemChanged
updateItem
isItemChanged
isItemChanged
isItemChanged
updateItem
isItemChanged
is this expected output?
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
line 421:
> 419: if (oldIndex == newIndex) {
> 420: if (!isItemChanged(oldValue, newValue)) {
> 421: // RT-37054: we break out of the if/else code here
> and
could we also add (or replace?) the new reference for RT-37054: JDK-8096969
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
line 424:
> 422: // proceed with the code following this, so that we
> may
> 423: // still update references, listeners, etc as
> required.
> 424: break outer;
I understand neither the comment nor this `break outer;`: there is no code that
follows `outer: if`, so we might as well `return`, right?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java
line 1150:
> 1148: super.updateItem(item, empty);
> 1149:
> 1150: counter.incrementAndGet();
very, very minor, and probably not important: shouldn't 'incrementAndGet()` be
called before `super.updateItem()`?
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
line 943:
> 941: super.updateItem(item, empty);
> 942:
> 943: counter.incrementAndGet();
same here: move before super.updateItem?
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
line 961:
> 959: * is called when the index is 'changed' to the same number as the
> old one, to evaluate if we need to call
> 960: * updateItem(..).
> 961: */
thank you so much for providing clear explanations of what all these tests are
trying to accomplish!
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 176:
> 174: super.updateItem(item, empty);
> 175:
> 176: counter.incrementAndGet();
and here. the reason is that if super.updateItem() throws an exception which
gets eaten (it shouldn't but we should not be assuming that) we still get the
counter incremented.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java
line 1078:
> 1076: super.updateItem(item, empty);
> 1077:
> 1078: counter.incrementAndGet();
...
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
line 1255:
> 1253: super.updateItem(item, empty);
> 1254:
> 1255: counter.incrementAndGet();
...
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableRowTest.java
line 960:
> 958: super.updateItem(item, empty);
> 959:
> 960: counter.incrementAndGet();
...
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
line 1777:
> 1775: }
> 1776:
> 1777: @Test public void testSetChildrenShouldUpdateTheCells() {
we probably should have `@Test` on its own line
