[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8193800: TreeTableView selection changes on sorting [v7]
From:       Ajit Ghaisas <aghaisas () openjdk ! java ! net>
Date:       2020-06-29 4:47:20
Message-ID: -cU6oxwM2C1GiKkopoMiZ8tI8EIVdtPDQ3-grqekMOc=.c2327610-988c-4c36-ba96-0dfe78b10eb4 () github ! com
[Download RAW message or body]

On Fri, 26 Jun 2020 09:22:53 GMT, Ambarish Rapte <arapte@openjdk.org> wrote:

> > Issue:
> > In TreeTableView, in case of Multiple selection mode, if nested items are \
> > selected, then TreeTableView does not retain/update the selection correctly when \
> > the tree items are permuted(either by `sort()` or by reordering using \
> > `setAll()`).  Cause: 
> > 1. For permutation, the current implementation uses `TreeModificationEvent` to \
> > update the selection. 2. The indices from these TreeModificationEvents are not \
> > reliable. 3. It uses the non public `TreeTablePosition` constructor to create \
> > intermediate `TreeItem` positions, this constructor results in another unexpected \
> > TreeModificationEvent while one for sorting is already being processed. 4. In \
> > case of sorting, there can be multiple intermediate TreeModificationEvents \
> > generated, and for each TreeModificationEvent, the selection gets updated and \
> > results in selection change events being generated. 5. Each time a TreeItem is \
> > expanded or collapsed, the selection must be shifted, but shifting is not \
> > necessary in case of permutation. All these issues combine in wrong update of the \
> > selection.  Fix: 
> > 1. On each TreeModificationEvent for permutation, for updating the selection, use \
> > index of TreeItem from the TreeTableView but not from the TreeModificationEvent. \
> > 2. Added a new non public TreeTablePosition constructor, which is almost a copy \
> > constructor but accepts a different row. 3. In case of sorting, send out the set \
> > of selection change events only once after the sorting is over. 4. In case of \
> > setAll, send out the set of selection change events same as before.(setAll \
> > results in only one TreeModificationEvent, which effectively results in only one \
> > set of selection change events). `shiftSelection()` should not be called in case \
> > of permutation i.e. call `if (shift != 0)` Verification:
> > The change is very limited to updating of selection of TreeTableView items when \
> > the TreeItems are permuted, so the change should not cause any other failures. \
> > Added unit tests which fail before and pass after the fix.
> 
> Ambarish Rapte has updated the pull request incrementally with one additional \
> commit since the last revision: 
> Correct the bug-id for ignored tests

Marked as reviewed by aghaisas (Reviewer).

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

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


[prev in list] [next in list] [prev in thread] [next in thread]