Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Thu, 4 Jun 2020 11:16:50 GMT, Ambarish Rapte wrote: > In the above case, The selection updates correctly but the received change > event on SelectedItems is incorrect. As of > now it looks like the selection is updated correctly with this fix, but the > event generated from sort() method is > always incorrect. Looking in to fix the change event Please take a look at the updated changes. The `GenericAddRemoveChange` event generated from `TreeTableView.sort()` method does not result in correct selection change events. @kevinrushforth , Thanks for the guidance on this in offline discussion. The solution is to sort the tree of selected items upfront before generating the `GenericAddRemoveChange` event. Rest of the TreeItems can be left to be sorted lazily whenever accessed. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Thu, 4 Jun 2020 11:23:42 GMT, Ambarish Rapte wrote: > > > > The other selectionModels for virtualized controls try to keep the > > selectedItem/Index only. > > Keeping the selection after sort/permutation seems more correct from a user's > perspective. I did not look into how > other controls handle this. Looking at your observation seems like we need to > fix this in other controls too, > preferably each control in a separate issue :) well, I don't think so (except for tree which is doing the-wrong-thingy-always): they do keep all state on sort, but not on replace-in-different-sequence - which is correct, I think: it's the semantic of the change notification that must rule, not some assumption of the view. If a replace-in-different-sequence is required to be interpretated as a permutation in a particular use-case, the place to implement it would be a custom list that checks for that corner case and sends a notification of type wasPermutated (instead of a wasReplaced). - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 12:03:43 GMT, Jeanette Winzenburg wrote: > The other selectionModels for virtualized controls try to keep the > selectedItem/Index only. Keeping the selection after sort/permutation seems more correct from a user's perspective. I did not look into how other controls handle this. Looking at your observation seems like we need to fix this in other controls too, preferably each control in a separate issue :) - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 13:45:12 GMT, Kevin Rushforth wrote: > Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 > number of children in each level > respectively. The fix works fine till level 3. But can observe issue with > level 4 and further. Shall debug this more > and update. In the above case, The selection updates correctly but the received change event on SelectedItems after sort is incorrect. As of now it looks like the selection is updated correctly with this fix, but the event generated from sort() method is always incorrect. Looking in to fix the change event - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 13:45:12 GMT, Kevin Rushforth wrote: >>> The algorithm looks correct to me for sorting. How much regression testing >>> have you done for cases where rows or >>> columns are inserted? >> >> I have tested with a small TreeTableView of 15 rows of 3 levels and 3 >> columns. >> Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 >> columns ? >> >> Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 >> number of children in each level >> respectively. The fix works fine till level 3. But can observe issue with >> level 4 and further. Shall debug this more >> and update. > >> I have tested with a small TreeTableView of 15 rows of 3 levels and 3 >> columns. >> Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 >> columns ? >> >> Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 >> number of children in each level >> respectively. The fix works fine till level 3. But can observe issue with >> level 4 and further. Shall debug this more >> and update. > > OK, thanks. In addition to making sure that insertion, deletion, and sorting > work, are you also checking that the > events being sent are as expected? Unrelated to this fix I see two pain points (which were not introduced here :) A. replaced vs. permutated These are types of change as decided by the list (aka: model) - treeModificationEvent (aka: view) must follow the lead (vs. spreading its own interpretation). Doing so is a bug, IMO. B. treeTableView.sort changing internals of the selectionModel that's a no-no-no-go, always. Instead let the selectionModel handle the permutated state correctly (didn't dig why it was deemed necessary to do so in sort - though it has the side-effect of leaving out selectionModels that are not of the expected type) - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 05:15:34 GMT, Ambarish Rapte wrote: > I have tested with a small TreeTableView of 15 rows of 3 levels and 3 columns. > Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 > columns ? > > Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 > number of children in each level > respectively. The fix works fine till level 3. But can observe issue with > level 4 and further. Shall debug this more > and update. OK, thanks. In addition to making sure that insertion, deletion, and sorting work, are you also checking that the events being sent are as expected? - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 12:03:43 GMT, Jeanette Winzenburg wrote: >>> The algorithm looks correct to me for sorting. How much regression testing >>> have you done for cases where rows or >>> columns are inserted? >> >> I have tested with a small TreeTableView of 15 rows of 3 levels and 3 >> columns. >> Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 >> columns ? >> >> Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 >> number of children in each level >> respectively. The fix works fine till level 3. But can observe issue with >> level 4 and further. Shall debug this more >> and update. > > hmm .. TreeModificationEvent seems to have a different interpretation (than a > list change) of _permutated_: its > wasPermutated may return true even on a replace - that's probably why some > tests are throwing a IllegalStateException > before the fix. The other way round: do we really want to introduce a > singularity in selection handling after > replace? The other selectionModels for virtualized controls try to keep the > selectedItem/Index only. need a break ;) Will come back later with an example comparing the behavior of multiple selection state across virtualized controls - preliminary results Sort: - ListView/TableView keep selection (corrrectly) on all selectedItems, adjust indices as needed - TreeView: ?? - TreeTableView: throws IllegalState before this fix, keeps selection (correctly) on all selectedItems, adjusts indices as needed after this fix Replace with same items in different sequence - ListView/TableView: keep only selectedItem, adjust selectedIndex as needed - TreeView: keeps selectedIndices (?) - TreeTableView: throws IllegalState before this fix, keeps selectedItems, adjusts selectedIndices as needed after this fix - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 12:03:43 GMT, Jeanette Winzenburg wrote: >>> The algorithm looks correct to me for sorting. How much regression testing >>> have you done for cases where rows or >>> columns are inserted? >> >> I have tested with a small TreeTableView of 15 rows of 3 levels and 3 >> columns. >> Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 >> columns ? >> >> Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 >> number of children in each level >> respectively. The fix works fine till level 3. But can observe issue with >> level 4 and further. Shall debug this more >> and update. > > hmm .. TreeModificationEvent seems to have a different interpretation (than a > list change) of _permutated_: its > wasPermutated may return true even on a replace - that's why some tests are > throwing a IllegalStateException before the > fix. The other way round: do we really want to introduce a singularity in > selection handling after replace? The other > selectionModels for virtualized controls try to keep the selectedItem/Index > only. ahh .. was wrong (note to self: run examples for all controls before commenting ;) - TableView also tries to keep all selected state on replace with the same items in changed sequence, ListView doesn't (which then is the singularity which might need a fix?). - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Wed, 3 Jun 2020 05:15:34 GMT, Ambarish Rapte wrote: >> 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. > >> The algorithm looks correct to me for sorting. How much regression testing >> have you done for cases where rows or >> columns are inserted? > > I have tested with a small TreeTableView of 15 rows of 3 levels and 3 columns. > Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 > columns ? > > Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 > number of children in each level > respectively. The fix works fine till level 3. But can observe issue with > level 4 and further. Shall debug this more > and update. hmm .. TreeModificationEvent seems to have a different interpretation (than a list change) of _permutated_: its wasPermutated may return true even on a replace - that's why some tests are throwing a IllegalStateException before the fix. The other way round: do we really want to introduce a singularity in selection handling after replace? The other selectionModels for virtualized controls try to keep the selectedItem/Index only. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Tue, 2 Jun 2020 21:01:05 GMT, Kevin Rushforth wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change isSortingInProgress to package scope
>
> 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.
Corrected in the next commit.
> 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.
Updated the doc in next commit, please take a look
-
PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Tue, 2 Jun 2020 21:08:27 GMT, Kevin Rushforth wrote: > The algorithm looks correct to me for sorting. How much regression testing > have you done for cases where rows or > columns are inserted? I have tested with a small TreeTableView of 15 rows of 3 levels and 3 columns. Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 columns ? - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Tue, 2 Jun 2020 21:00:27 GMT, Kevin Rushforth wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change isSortingInProgress to package scope
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
> line 1822:
>
>> 1821: public void sort() {
>> 1822: sortingInProgress = true;
>> 1823: final ObservableList> sortOrder =
>> getSortOrder();
>
> I presume that sorting cannot be called recursively?
The `sort()` is called only once for a `TreeTableView`. This method does not
perform the actual sorting but it is
performed by the sort policy(`DEFAULT_SORT_POLICY`). `DEFAULT_SORT_POLICY`
initiates the sorting by calling
`rootItem.sort()`, which results in call to `TreeItem.doSort()`.
`TreeItem.doSort()` makes the final call to sort the
children list, this method gets executed once for the root TreeItem and then
for each expanded child TreeItem.
-
PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
On Tue, 2 Jun 2020 18:02:50 GMT, Ambarish Rapte 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> 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
Re: [Rev 01] RFR: 8193800: TreeTableView selection changes on sorting
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/244/files - new: https://git.openjdk.java.net/jfx/pull/244/files/d6376dfc..44052f35 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/244/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/244/webrev.00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/244.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/244/head:pull/244 PR: https://git.openjdk.java.net/jfx/pull/244
