On Tue, 2 Jun 2020 18:02:50 GMT, Ambarish Rapte <ara...@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:
>
> Change isSortingInProgress to package scope
The algorithm looks correct to me for sorting. How much regression testing have
you done for cases where rows or
columns are inserted?
I left a few comments, and will complete my testing in parallel.
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
line 1822:
> 1821: public void sort() {
> 1822: sortingInProgress = true;
> 1823: final ObservableList<TreeTableColumn<S,?>> sortOrder =
> getSortOrder();
I presume that sorting cannot be called recursively?
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
line 1807:
> 1806:
> 1807: boolean sortingInProgress;
> 1808: boolean isSortingInProgress() {
Minor: the field itself can be private.
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java
line 82:
> 81: // This causes issue by triggering a new TreeModificationEvent while
> one TreeModificationEvent
> 82: // is being handled currently.
> 83: // This is kind of a copy constructor with different value for row.
The first three lines of added comments don't really belong here. I would just
document the new constructor as a
copy-like constructor (with a different row) and mention that it is used by the
TreeTableView::sort method.
-------------
PR: https://git.openjdk.java.net/jfx/pull/244
