On Wed, 3 Jun 2020 13:45:12 GMT, Kevin Rushforth <k...@openjdk.org> 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
