On Fri, 24 Sep 2021 06:16:55 GMT, Robert Lichtenberger <rlich...@openjdk.org> wrote:
> > First, this has the potential to hurt performance for applications that do > > a lot of setup on tables that aren't visible. > > That would be possible if the hbar changes its visibility or its value. I > can't really see how that would happen as long as the hbar isn't visible. > Well I guess it is theoretically possible that someone subclasses VirtualFlow > and adds thousands of items and changes the hbar value to some value after > each item before making the VirtualFlow visible. > > There are several places where we defer updating and the usual way we > > address any such problems is to validate when "something" changes. In this > > case, the "something" should include becoming visible or the scene going to > > non-null (or alternatively being "treeVisible", which means visible and > > part of a scene that is showing). > > If this is the "convention" of how things should be done then I will adapt my > PR to **not** remove the said line but instead make the listenerX fire also > when the VIrtualFlow's visible or scene property changes. > This will also fix the problem an keep the performance optimization in place. Good. This seems a better fix. While the performance hit is likely theoretical, it becomes a non-issue with your update fix. > > Second, the code you propose to remove was added as part of the fix for > > [JDK-8112072](https://bugs.openjdk.java.net/browse/JDK-8112072), and while > > I doubt it was added to ensure correctness, it will need to be tested. > > As posted in JDK-8274137: Tested it, found no problems. Thanks. > > Third, I prefer that tests not check a specific implementation detail such > > as ensuring that derived values are computed when the scene is null or the > > node is invisible. Rather, it is better to check that a change made while > > the node is not visible (or the scene is null), is valid _after_ making it > > visible on an active scene. > > I claim that testScrollBarClipSyncWhileInvisibleOrNoScene does just that. > > * It first checks that hBar and clipping stay in sync when the flow is > visible and has a scene. (sunshine case) > * Then it changes the hBar while the flow is invisible, makes the flow > visible again and checks if (after the flow is now visible again) hBar and > clipping are in sync again. > * Then it does the same by removing hBar from the scene. Clearly I didn't look closely enough at the test. I missed that you were already doing what I was asking for. ------------- PR: https://git.openjdk.java.net/jfx/pull/629
