RFR: 8265669: AccumCell should not be visible

[Johan Vos]

On Fri, 23 Apr 2021 13:48:16 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> The change as such is fine, but, I have two questions at a higher level -

Thanks. The answers are higher level as well, as they are more about the expectations of updateItem()
> 
> > (from JBS description) - "This will lead to a call to Cell.updateItem(). That was already happening before, but it is happening more often now in case the gradual caching goes faster than the scrolling. "
> 
> Question - 1) How did you find out it is happening more often? Do you have any test that you feel is worth adding?

The fix for JDK-8089589 gradually adds more information about the size of the items that are not yet displayed, in order to be able to gradually estimate the correct position of the scrollbar thumb. In order to do so, the entries need to be calculated, which includes a call to Cell.updateItem(). The current implementation will add 2 new cells per re-layout into the calculations. 
I don't think a test would add value here, as there is no binary right or wrong. 
The conceptual issue is more about the contract of Cell.updateItem(). That method is called when the contents of a cell change, but this does not imply that the considered cell is visible. Indeed, the VirtualFlow has the concept of an accumCell that is not visible in the control, but it is used for a number of calculations. 

> 
> > (from JBS bug description) - "In order to allow this logic to still be possible, I suggest the accumCell should always made be invisble after it has done its work. That way, the `updateItem` can clearly decide what to do, based on not only the index, but also the visibility of the considered item. "
> 
> Question - 2) Does this mean any followup work is needed to tweak `updateItem` after this change ?

No, the JavaFX implementations don't need to change. However, developers can now check in their updateItem method whether the considered cell is visible or invisible (in which case it is most likely the accumCell). Nothing in the `Cell.updateItem` specifies that the considered cell is visible, and the implementations should do what is needed to layout the cell. However, there implementations in third-party code that depend on the cell index of the considered cell to do some (non UI-related) logic, and if that logic falsely assume that the considered cell is visible, the results are not what the developer expects. Since the contract doesn't say the cell is visible, this is not a spec error, but if the cell.isVisible() returns true (as could have been the case before this fix), developers have no way to find out if the cell is shown or not (even if the accumCell was visible, it was still not shown as the code in VirtualFlow prevents this)

-------------

PR: https://git.openjdk.java.net/jfx/pull/474