On Wed, 4 Nov 2020 17:25:16 GMT, Jose Pereda <jper...@openjdk.org> wrote:
> As discussed in the JBS > [issue](https://bugs.openjdk.java.net/browse/JDK-8177945), there are some > inconsistencies in the use of `VirtualContainerBase::requestRebuildCells` > from `VirtualContainerBase::updateItemCount()`, which is implemented in the > different skin classes for virtualised controls `TableViewSkinBase`, > `ListViewSkin` or `TreeTableViewSkin`. > > The latter already commented out this call (related to JDK-8155798 and > JDK-8147483). > > This PR removes now the calls to `VirtualContainerBase::requestRebuildCells` > from `TableViewSkinBase` (except for the case `itemCount = 0` based on > JDK-8118897 and JDK-8098235) and `ListViewSkin`. > > A test is provided for TableView, that verifies that the `selected` > pseudo-class state remains set for the selected cell while adding more items. > Without this fix, as the cells are rebuilt, the pseudo-class states are clean > and set all over again, thus the flickering. > > For ListView, the test rt_35395 (JDK-8091726) is updated, as now there are > way less calls to updateItem. The fix looks good with one question and one minor suggestion. modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java line 359: > 357: > 358: updatePlaceholderRegionVisibility(); > 359: if (newCount == oldCount) { Does this also need the same `else if (oldCount == 0)` test that you added to `TableViewSkinBase`? modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 5482: > 5480: > 5481: @Test > 5482: // see JDK-8177945 Minor: It seems best to not have the comment between the annotation and the method being annotated. I see this pattern also in `VirtualFlowTest`, but you might want to move the comment above the annotation. ------------- PR: https://git.openjdk.java.net/jfx/pull/348
