[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: RE: JDK-8177945 : Single cell selection flickers when adding data to TableView
From: Eric Bresie <ebresie () gmail ! com>
Date: 2020-03-26 13:01:11
Message-ID: ed4f3af3-88fd-42d5-959d-850706b57013 () Erics-iPhone-X
[Download RAW message or body]
Not sure if this is the cause of some of this but...
I see some similar flags like needsRecreateCells and needsRebuildCells.
In one context (around line 1025) I see a recreateOrRebuild flag based on the two.
Is it possible something similar is needed around here to only do something once if \
either is set?
Maybe it's doing the reset twice as part of recreate and rebuild.
Eric Bresie
Ebresie@gmail.com
> On March 10, 2020 at 11:48:27 AM CDT, David Grieve <david.grieve@microsoft.com> \
> wrote: The flickering is happening because of the way VirtualFlow reuses cells. The \
> selected state gets cleared and reset, which is the flicker. This change fixed the \
> flickering for me. I'm not sure why the index of a cell was being set to -1, but \
> the effect of doing so is that when the cell is pulled out of the pile, it looks \
> like a new cell that has to have its index set, selected state set, etc, etc. By \
> commenting out the updateIndex(-1), VirtualFlow can reuse the cell - setting the \
> index, selected state, etc turns into noops (more or less).
> Again, there may have been a good reason for setting index to -1 in this condition. \
> More investigation needs to be done.
> diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java \
> b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java \
> index 728e7c5513..a58859bd05 100644
> --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
>
> +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> @@ -991,9 +991,9 @@ public class VirtualFlow<T extends IndexedCell> extends Region \
> { lastWidth = -1;
> lastHeight = -1;
> releaseCell(accumCell);
> - for (int i = 0, max = cells.size(); i < max; i++) {
> - cells.get(i).updateIndex(-1);
> - }
> +// for (int i = 0, max = cells.size(); i < max; i++) {
> +// cells.get(i).updateIndex(-1);
> +// }
> addAllToPile();
> releaseAllPrivateCells();
> } else if (needsReconfigureCells) {
>
> > -----Original Message-----
> > From: openjfx-dev <openjfx-dev-bounces@openjdk.java.net> On Behalf Of
> > David Grieve
> > Sent: Friday, March 6, 2020 9:29 AM
> > To: Danny Gonzalez <danny.gonzalez@screamingfrog.co.uk>
> > Cc: openjfx-dev@openjdk.java.net
> > Subject: RE: JDK-8177945 : Single cell selection flickers when adding data to
> > TableView
> >
> > This might fix the issue, but I'd like to understand better what the root of the
> > problem is. My concern is that this fix might cause a performance regression.
> > I'm using the code in JDK-8177945. I want to look at what TableView does
> > when it adds a cell. Is there something about the selected state that
> > changes? Or is this just the CSS implementation recalculating styles and un-
> > necessarily clearing old values? Some debugging is in order.
> >
> > From: Danny Gonzalez <danny.gonzalez@screamingfrog.co.uk>
> > Sent: Wednesday, March 4, 2020 4:34 AM
> > To: David Grieve <David.Grieve@microsoft.com>
> > Cc: openjfx-dev@openjdk.java.net
> > Subject: [EXTERNAL] Re: JDK-8177945 : Single cell selection flickers when
> > adding data to TableView
> >
> > Hi David,
> >
> > Just wanted to add some more information.
> >
> > The cell selection flashing issue goes away If I put back the following code in
> > the layout function in Parent.java:
> >
> > //
> > // One idiom employed by developers is to, during the layout pass,
> > // add or remove nodes from the scene. For example, a ScrollPane
> > // might add scroll bars to itself if it determines during layout
> > // that it needs them, or a ListView might add cells to itself if
> > // it determines that it needs to. In such situations we must
> > // apply the CSS immediately and not add it to the scene's queue
> > // for deferred action.
> > //
> > // Note that we only call processCSS if the flag is update. If the
> > // flag is DIRTY_BRANCH, then the whatever children need UPDATE
> > // will be visited during this layout anyway. On the next pulse,
> > // doCSSPass will clean up the DIRTY_BRANCH nodes.
> > //
> > if (cssFlag == CssFlags.UPDATE) {
> > processCSS();
> > }
> >
> > This code was originally added in in the following commit:
> >
> > commit e76b5755d4d2752037cc95eb75cb2615a740cc30
> > Author: David Grieve
> > <dgrieve@openjdk.org<mailto:dgrieve@openjdk.org>>
> > Date: Thu Apr 10 15:40:34 2014 -0400 (tel:34%202014%20-0400)
> >
> > RT-36559: Some css optimizations: 1 - on impl_reapplyCSS, do not reapply
> > css to child nodes if nothing has changed. 2 - on applyCss, only look for
> > ancestor node with css state = UPDATE. 3 - only recalculate checksum of css
> > file if the file has been removed from a scene or parent
> >
> >
> > It was reverted out in this commit:
> >
> > commit 05afad6b528e871d607b76aea2642cf788b417fe
> > Author: David Grieve
> > <dgrieve@openjdk.org<mailto:dgrieve@openjdk.org>>
> > Date: Tue Apr 15 11:51:38 2014 -0400 (tel:38%202014%20-0400)
> >
> > RT-36672: move code to process css during layout back to impl_reapplyCSS,
> > which is where it was priort to RT-36559
> >
> >
> >
> >
> > This was the point at which the cell selection flashing appeared.
> >
> >
> > Thanks
> >
> >
> > Danny
> >
> >
> >
> > On 3 Mar 2020, at 16:50, David Grieve
> > <David.Grieve@microsoft.com<mailto:David.Grieve@microsoft.com>>
> > wrote:
> >
> > The importance of 05afad6 Is there in the commit itself:
> >
> > + //
> > + // One idiom employed by developers is to, during the layout pass,
> > + // add or remove nodes from the scene. For example, a ScrollPane
> > + // might add scroll bars to itself if it determines during layout
> > + // that it needs them, or a ListView might add cells to itself if
> > + // it determines that it needs to. In such situations we must
> > + // apply the CSS immediately and not add it to the scene's queue
> > + // for deferred action.
> > +
> >
> > If you revert 05afad6, you'll break this.
> >
> > This is the first time I've become aware of this flickering issue. I'll have to \
> > look at it.
> > I wonder if the fix for https://bugs.openjdk.java.net/browse/JDK-
> > 8193445<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-
> > 8193445&data=02%7C01%7CDavid.Grieve%40microsoft.com%7C9148c860ff5
> > 24a32ac3208d7c01f2523%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%
> > 7C637189112753556661&sdata=mZrjldSw%2Fjbd0ita58XMU%2BaKM2GlvObq
> > 3aaQmQb5Poc%3D&reserved=0> has any impact on this.
> >
> >
> > -----Original Message-----
> > From: openjfx-dev <openjfx-dev-
> > bounces@openjdk.java.net<mailto:openjfx-dev-
> > bounces@openjdk.java.net>> On Behalf Of Danny Gonzalez
> > Sent: Tuesday, March 3, 2020 11:14 AM
> > To: openjfx-dev@openjdk.java.net<mailto:openjfx-dev@openjdk.java.net>
> > Subject: [EXTERNAL] JDK-8177945 : Single cell selection flickers when adding
> > data to TableView
> >
> >
> > There is currently an open bug to do with the issue of selection flickering
> > when using single cell selection and when adding data to a TableView.
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> > .java.com%2Fbugdatabase%2Fview_bug.do%3Fbug_id%3D8177945&da
> > ta=02%7C01%7Cdavid.grieve%40microsoft.com%7C91a16d9ab05340719f3008
> > d7bf8e3410%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63718848
> > 9816399816&sdata=wdPRh3VnN%2BfJw%2BatKOyBi9%2Ba2%2FidMJJb
> > 8AwcRXVrfLo%3D&reserved=0
> >
> > This bug seems to be low priority because it hasn't been looked at since I
> > submitted it at the start of 2017.
> >
> > This is quite a major issue for us so I have narrowed down when the issue
> > was first introduced.
> >
> > Here is the changeset:
> >
> > commit 05afad6b528e871d607b76aea2642cf788b417fe
> > Author: David Grieve
> > <dgrieve@openjdk.org<mailto:dgrieve@openjdk.org<mailto:dgrieve@open
> > jdk.org%3cmailto:dgrieve@openjdk.org>>>
> > Date: Tue Apr 15 11:51:38 2014 -0400 (tel:38%202014%20-0400)
> >
> > RT-36672: move code to process css during layout back to impl_reapplyCSS,
> > which is where it was priort to RT-36559
> >
> >
> > I can't search the bug database for this bug ID as I believe it was submitted
> > when a previous bug tracking system was in use.
> >
> > Does anyone have any knowledge as to why this fix was needed and what
> > the repercussions would be if I reverted it out for our local OpenJFX build.
> >
> > Alternatively I would be glad to receive any suggestions of how to fix the
> > flickering issue if this changeset is important to leave in.
> >
> > Thanks
> >
> > Danny
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic